Problem/Motivation

When using the media browser popup, it is possible to trigger 

react-dom.min.js?v=16.8.4:121 TypeError: Cannot read properties of null (reading 'style')
    at dialog:aftercreate (toolbar.js?v=10.2.0:211:24)
    at dispatch (jquery.min.js?v=3.7.1:2:40035)
    at v.handle (jquery.min.js?v=3.7.1:2:38006)
    at Object.trigger (jquery.min.js?v=3.7.1:2:70124)
    at jquery.min.js?v=3.7.1:2:70726
    at Function.each (jquery.min.js?v=3.7.1:2:3129)
    at ce.fn.init.each (jquery.min.js?v=3.7.1:2:1594)
    at ce.fn.init.trigger (jquery.min.js?v=3.7.1:2:70701)
    at openDialog (dialog.js?v=10.2.0:83:17)
    at dialog.showModal (dialog.js?v=10.2.0:101:7)

when the media browser is rendered as a modal. For example using a combination of Drupal 10.2, Gutenberg v3 and the media browser can trigger it. (It is possible to trigger this issue from other combinations of modules that use the media library popup dialog ui as well)

Steps to reproduce

To reproduce w/the gutenberg editor (it is reproducible from most ways to get to the media browser dialog popup UI)

Insert an image into the node body in gutenberg, click replace on the image in the gui, and hit open media library.

The console will emit errors like the above and 

toolbar.js?v=10.2.0:230 Uncaught TypeError: Cannot read properties of null (reading 'style')
    at dialog:beforeclose (toolbar.js?v=10.2.0:230:24)
    at dispatch (jquery.min.js?v=3.7.1:2:40035)
    at v.handle (jquery.min.js?v=3.7.1:2:38006)
    at Object.trigger (jquery.min.js?v=3.7.1:2:70124)
    at jquery.min.js?v=3.7.1:2:70726
    at Function.each (jquery.min.js?v=3.7.1:2:3129)
    at ce.fn.init.each (jquery.min.js?v=3.7.1:2:1594)
    at ce.fn.init.trigger (jquery.min.js?v=3.7.1:2:70701)
    at Object.closeDialog [as close] (dialog.js?v=10.2.0:87:17)
    at Object.close (dialog.js?v=10.2.0:31:35)

The dialog will be blank and there is no way to insert anything from the media library.

The error occurs because toolbar.js assumes that the toolbar is always part of the scope when the scope is actually limited in the context of the media browser popup dialog (I think...?)

Proposed resolution

The fix is pretty easy -- in toolbar.js check to see that toolbarBar is a thing before trying to manipulate it. There is no error handling if document.getElementByID('toolbar-bar'); returns null in this scope.

Apply the attached patch to work around it.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#27 fix_toolbar_null_handling-3409505-27.patch2.04 KBsakiland
#5 2023-12-21 12-03-11.mp415.61 MBdarko_antunovic
#2 fix-toolbarjs-null-handling.patch1011 bytesmalik.kotob
fix-toolbarjs-null-handling.patch1.19 KBwendell

Issue fork drupal-3409505

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

wendell created an issue. See original summary.

malik.kotob’s picture

malik.kotob commented
Status: Active » Needs review
StatusFileSize
new fix-toolbarjs-null-handling.patch1011 bytes

I ran into this same issue! The patch as-is didn't quite work, but not because the code itself wasn't sound. It seems the patch formatting itself had an issue? Here's the patch I used (the code was borrowed @wendell in the original patch). Note that I was able to reproduce this when adding an inline block via Layout Builder.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Can we get a test showing the issue?

JJLems’s picture

JJLems commented

I encountered a similar issue while using the Gin Toolbar module. This module modifies the toolbar's ID to gin-toolbar-bar, making it untargetable in toolbar.js.

darko_antunovic’s picture

darko_antunovic
He/him
Primary language Serbian
commented
StatusFileSize
new 2023-12-21 12-03-11.mp415.61 MB

I am attaching a video recording of the behaviour that I am encountering.

klemendev’s picture

klemendev commented

This also happens with Bootstrap's modal dialogs in combination with ajax_comments when deleting a comment

alina.basarabeanu’s picture

alina.basarabeanu commented

The patch fixed the Ajax error on Drupal 10.2, the Gin theme 8.x-3.0-rc8 and the Gin Layout Builder 1.0.0-rc5.
As mentioned above the Gin Layout Builder modified the ID for the secondary toolbar to "gin-secondary-toolbar" and the code from the toolbar.js fails to find the element in the DOM.
It is always safer to check if the element exists before applying a style.
We will use the patch from #2 to fix the issue.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Title: dialog:aftercreate cannot read properties of null (toolbar.js) » Uncaught TypeError: Cannot read properties of null (reading 'style') (toolbar.js)
tyler36’s picture

tyler36 commented

Patch #2 fixes issue for me with Drupal 10.2.1, the Gin theme 8.x-3.0-rc8 and the Gin Layout Builder 1.0.0-rc5.

Thank you.

evilargest’s picture

evilargest commented

#2 works for me with Drupal 10.2.2 and Gin 8.x-3.0-rc7. Thanks.

tyler36’s picture

tyler36 commented
Status: Needs work » Reviewed & tested by the community
cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work

A maintainer asked for tests.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented

The test is hard to make because it happens only with the conjunction of gin/gin toolbar because gin removes some classes and toolbar.js does not have a proper check whether element exists (which should be a good practice anyways). Gin Toolbar has 30k+ installs, pretty big impact imho

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Priority: Normal » Major

#2 also resolves this with Gin and Layout Builder. I'm bumping to Major is it makes LB un-usable. A test may be tricky since this seems to only be surfaced by Contrib modules...

mlncn’s picture

mlncn commented

Running into this in putting a Drupal.dialog (not ajax) in a contrib module.

This patch fixes it for me, but the fact that adding a dialog in my module triggered this problem in toolbar makes me wonder if i am not doing something to scope my dialog correctly— or if dialog has a bigger bug?

mindaugasd’s picture

mindaugasd commented

My error was not mentioned toolbarBar is null, but search still worked fine to find this issue.

Patch fixed the error after clearing Drupal and Browser caches.

Interesting fact: custom link dialog (https://www.drupal.org/docs/develop/drupal-apis/ajax-api/ajax-dialog-boxes) + bootstrap theme + css + js keeps braking (in multiple ways per upgrade) every time I update Drupal for years.

klemendev’s picture

klemendev commented

I will try to test if this is the case for this issue too when I get some time: https://www.drupal.org/project/ajax_comments/issues/3412943

It is also custom link dialog + bootstrap theme + css + js

Version: 10.2.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented

Have the same issue as #14 and the patch solves the issue. Thanks!
We need to figure out how to have tests for this.

klemendev’s picture

klemendev commented

I can confirm patch #2 fixes https://www.drupal.org/project/ajax_comments/issues/3412943 to the level buttons on the dialog work and no JS errors are printed, however, the look (CSS styling) of the popup dialog is still broken

klemendev’s picture

klemendev commented

I think the issue is deeper as this patch only fixes the consequence, but not the reason why styles are not properly passed to AJAX dialogs/popups

sirclickalot’s picture

sirclickalot
Primary language English
Location Somerset
commented

I would just like to raise the question that this issue is marked as Version: 11.x-dev but that it is definitely an issue for Drupal 10.2.3 too.
I think my statement is backed up by this issue.

Thanks all, if I'm wrong then please someone put me right!

klemendev’s picture

klemendev commented

It seems multiple modules as well as the core itself were affected by some change in 10.2 that caused this. This issue prevents many websites from updating to 10.2, while 10.1 EOL is getting closer and closer (17. 6. 2024)

lukas.fischer’s picture

lukas.fischer commented

I just successfully tested https://www.drupal.org/files/issues/2023-12-20/fix-toolbarjs-null-handli... on 10.2.4

sakiland made their first commit to this issue’s fork.

sakiland opened merge request !7333

sakiland’s picture

sakiland commented
StatusFileSize
new fix_toolbar_null_handling-3409505-27.patch2.04 KB

I've just created MR with implemented @wendell and @malik.kotob work and additional check.

Here's also patch for this

klemendev’s picture

klemendev commented

Is this needs work, or needs review?

The last patch seems to fix the ajax part of the problem I had

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
Issue tags: -Needs tests +needs no tests

Following #2972776: [policy, no patch] Better scoping for bug fix test coverage I don't think tests are required here.

The issue has clear steps to reproduce, and the fix is trivial - just a guard against NULL. It has already been verified with manual testing by people in this issue. It is self-contained to the toolbar JS, doesn't require any new code, a regression will only leave us back where we started, and adding test coverage is tricky without jumping through a lot of hoops.

The patch looks good to me, marking NR for someone to confirm I have applied the new testing gate correctly above.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

A b/c break was inadvertently added by https://git.drupalcode.org/project/drupal/-/commit/6a43b511df9e26aa5abf0... when jquery was swapped out for vanilla JS.

Previously, jQuery was more tolerant of an element not found condition. The vanilla JS results in an uncaught exception.

I was going to remove the needs tests tag, but Dave beat me to it :D

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Oh, if the break was in that issue then I'm even more confident from reading the diff there. Happy to mark this RTBC and let another committer confirm.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Uncaught TypeError: Cannot read properties of null (reading 'style') (toolbar.js) » [regression] Uncaught TypeError: Cannot read properties of null (reading 'style') (toolbar.js)

Also adding the regression title tag given that this used to work and we broke it in 10.2.

  • nod_ committed 41943e87 on 11.x 
    Issue #3409505 by sakiland, wendell, malik.kotob, darko_antunovic,...

  • nod_ committed d51837f8 on 10.3.x 
    Issue #3409505 by sakiland, wendell, malik.kotob, darko_antunovic,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 41943e8 and pushed to 11.x. Thanks!

nod_ closed merge request !7333

  • longwave committed e4949440 on 10.2.x authored by nod_ 
    Issue #3409505 by sakiland, wendell, malik.kotob, darko_antunovic,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 10.3.x-dev » 10.2.x-dev

This is eligible for a bug fix release in 10.2.x, cherry-picked it back there as well.

  • nod_ committed 41943e87 on 11.0.x 
    Issue #3409505 by sakiland, wendell, malik.kotob, darko_antunovic,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.