Problem/Motivation
I'm seeing this error in our project CI with drupal dialogs:
Error: cannot call methods on dialog prior to initialization; attempted to call method 'option'
This happens, when opening and immediately closing a modal (very fast).
Steps to reproduce
It's quite easy to reproduce with any modal and some JS code:
- click button to open modal,
- wait for .ui-dialog element to exist/be visible,
- and immediately close modal with cancel/close button.
The MR contains a test to reproduce.
If needed here is a sample JS code:
function waitForElement(selector) {
return new Promise(resolve => {
if (document.querySelector(selector)) {
return resolve(document.querySelector(selector));
}
const observer = new MutationObserver(mutations => {
if (document.querySelector(selector)) {
observer.disconnect();
resolve(document.querySelector(selector));
}
});
observer.observe(document.body, {
childList: true,
subtree: true
});
});
}
document.querySelector('{open modal button selector}').click();
waitForElement('.ui-dialog').then((dialog) => {
dialog.querySelector('{close button selector}').click();
});Proposed resolution
It seems that in such cases, Drupal.dialog.resetSize is called from debounce after the jquery dialog instance has been destroyed. This happens within 20ms (debounce/setTimeout interval).
Add a check ensuring we have a valid jquery dialog instance at the top of Drupal.dialog.resetSize.
Also, per #25
dialog:beforeclose unbinds the resize handlers, but doesn't stop a already-scheduled debounced call. When the element is removed the jQuery UI instance is destroyed and the call tries to debounce on the destroyed dialog.
The fix adds a cancel() method to debounce.js and uses it when the dialog is closed, and adds test coverage.
Remaining tasks
?
User interface changes
None
Introduced terminology
None
API changes
None
Data model changes
None
Release notes snippet
None
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 3472624-nr-bot.txt | 1.05 KB | needs-review-queue-bot |
Issue fork drupal-3472624
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:
Comments
Comment #3
herved commentedComment #4
herved commentedComment #5
herved commentedMoving to needs review, thanks.
PS: It seems nightwatch has intermittent failures these days, phpunit as well
Comment #6
herved commentedLinking #3356667: Error: Cannot read properties of undefined (reading 'settings') with dialog.position.js as possibly related (but not quite).
Comment #7
smustgrave commentedMay be able to help decide the solution.
Comment #8
herved commentedI believe #2856047: Avoid random failures in JavascriptTestBase when testing functionality in a dialog is also related, as I mentioned in #6.
Comment #9
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #10
smustgrave commentedFalse positive
Comment #11
saidatomGreat! It looks good, moving to RTBC.
Comment #12
dxvargas commentedI just want to confirm the RTBC.
The new version of the MR, after the last two commits managed to fix the problems in the pipeline I had before.
Comment #13
claudiu.cristeaComment #14
nod_I can't seem to replicate the test failures. When I run the tests without the immediate and early return fix from the dialog.position.js file it's still green.
Comment #16
joseph.olstadWhile this patch deals with dialog
I was hoping it would also fix resizable
I'm seeing this one:
Using a theme (not claro) for a node form.
This worked up until Drupal 10.4.x
In Drupal 11 there's a js error related to the media dialog.
Comment #17
grimreaperHi,
I have the same problem on a D10.4, with CKE5 in Layout Builder offcanvas, on a text format with the drupal media and/or the icon (from ui_icons) plugins.
When I click on the media insert button or the icon insert button, the error is immediately present in the browser's console.
Comment #18
herved commented#17 Hi @grimreaper,
Yes I also noticed these errors get triggered with CKeditor: https://git.drupalcode.org/project/drupal/-/merge_requests/9440#note_398735
Is this MR here fixing your issue?
There is also #3356667: Error: Cannot read properties of undefined (reading 'settings') with dialog.position.js which relates closely to this, maybe the patch there is is the proper fix?
Comment #19
grimreaperHi @herved,
I tried the MR changes without success.
But those errors were not blocking me it was something else related to dialogs which I had not been able to identify.
Like events not triggerred or incorectly triggered (not the right element, stuff like that).
So I pass to something else for until now.
Comment #20
goz commentedHi, i have the same issue using ui_suite_bootstrap + layout_paragraphs + mercury_editor
In node edition with paragraph layout, start a new paragraph, choose a component (+ button), add your section paragraph in the first dialog, second dialog opened and error is logged in console
The MR does not solve this.
Comment #22
albeorte commentedThe current MR is not solving the problem.
Comment #23
longwaveThis has got much worse since #3515403: Update to selenium/standalone-chrome:133, affecting multiple JS tests, so raising to critical. Removing subsystem maintainer tag for now as I don't think we need it, we just need to find a solution.
Comment #24
longwaveComment #25
longwaveDiagnosis and fix assisted by Claude Code.
dialog:beforecloseunbinds the resize handlers, but doesn't stop a already-scheduleddebouncedcall. When the element is removed the jQuery UI instance is destroyed and the call tries to debounce on the destroyed dialog.The fix adds a
cancel()method to debounce.js and uses it when the dialog is closed, and adds test coverage.Comment #27
godotislateRepeat test (500x) of CKEditor5AllowedTagsTest::testFullHtml, which was randomly failing quite often: https://git.drupalcode.org/project/drupal/-/jobs/11377696. Passed about 350 times without failure before the job timed out.
Test only https://git.drupalcode.org/project/drupal/-/jobs/11377699 fails as expected.
lgtm!
Will be great to get this in because core FJ tests are failing randomly quite a lot.
Comment #28
quietone commentedChanging title to be a description of what is being fixed or improved per https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-issue/list-of-issue-fields#s-title
Comment #29
godotislateComment #30
alexpottWe need a change record to tell everyone about debounce.cancel and when to use it. We also should check if there are other implementations that could use it in core.
Comment #31
godotislatePer @longwave's MR comment, I think it's OK to go with the current solution.
Added a CR for
cancel: https://www.drupal.org/node/3619513. It could use a review. I think it'd be nice for the CR to have an example of use, but I couldn't come up with one that is relatively simple.Comment #32
claudiu.cristeaRemark has a reply and a CR has been added. Back to RTBC
Comment #33
alexpottCommitted and pushed d0904d4ace3 to main and f0cc91d7ec9 to 11.x. Thanks!
This feels worth backporting to 11.x