Ensure the UI dialog instance exists/is valid in Drupal.dialog.resetSize.
Closes #3472624
Merge request reports
Activity
63 63 * @fires event:dialogContentResize 64 64 */ 65 65 function resetSize(event) { 66 // Ensure the UI dialog instance exists/is valid. 67 // If the dialog is closed very rapidly, the jQuery UI instance may have 68 // been destroyed, but debounce might still call this function within the 69 // setTimeout interval. 70 if (!event.data.$element.data('ui-dialog')) { 71 return; 72 } -
Calling the debounce immediately also seems to fix it:
debounce(resetSize, 20, true)
Whould that be a better solution?
Is there a reason why the debounce is not called immediately at the moment?
Could this also solve https://www.drupal.org/project/drupal/issues/2856047 which I also encountered in our CI (this is more frequent with lower height resolutions).Edited by Hervé Donner -
I noticed
resetSizecan be called with an emptyevent.datawhen resizing a window that contains a dialog containing a CKEditor field. Of course, we get:TypeError: Cannot read properties of undefined (reading '$element') at HTMLDocument.resetSize (http://127.0.0.1:8888/core/misc/dialog/dialog.position.js?v=11.0-dev:70:21) at later (http://127.0.0.1:8888/core/misc/debounce.js?v=11.0-dev:37:23)
This is reproductible in tests.
I then propose the following:
- call debounce immediately: I believe this should solve intermittent issues like https://www.drupal.org/project/drupal/issues/2856047
- harden function to check if
event.dataandevent.data.$elementare set.
added 45 commits
-
0394fd6d...79a49040 - 42 commits from branch
project:11.x - af4d0587 - Ensure the UI dialog instance exists/is valid in Drupal.dialog.resetSize.
- 76a07133 - Fix JS errors when resizing a window with a dialog containing a CKEditor field.
- d8947d9b - Call debounce immediately, prevents modal from moving around causing...
Toggle commit list-
0394fd6d...79a49040 - 42 commits from branch
188 223 // Press buttons in the dialog to ensure there are no AJAX errors. 189 224 $this->assertSession()->elementExists('css', '.ui-dialog-buttonpane')->pressButton('Hello world'); 190 225 $this->assertSession()->assertWaitOnAjaxRequest(); 191 $has_focus_text = $this->getSession()->evaluateScript('document.activeElement.textContent'); 192 $this->assertEquals('Do it', $has_focus_text); 193 226 $this->assertSession()->elementExists('css', '.ui-dialog-buttonpane')->pressButton('Preview'); 194 227 $this->assertSession()->assertWaitOnAjaxRequest(); 195 $has_focus_text = $this->getSession()->evaluateScript('document.activeElement.textContent'); 196 $this->assertEquals('Do it', $has_focus_text); 188 223 // Press buttons in the dialog to ensure there are no AJAX errors. 189 224 $this->assertSession()->elementExists('css', '.ui-dialog-buttonpane')->pressButton('Hello world'); 190 225 $this->assertSession()->assertWaitOnAjaxRequest(); 191 $has_focus_text = $this->getSession()->evaluateScript('document.activeElement.textContent'); 192 $this->assertEquals('Do it', $has_focus_text); 69 93 $this->assertNotNull($close_button); 70 94 $close_button->press(); 71 95 96 // Test opening and immediately closing modal a few times. 97 // Ensure no JS errors are thrown. 98 for ($i = 0; $i < 10; $i++) { 99 $this->getSession()->getPage()->clickLink('Link 1 (modal)'); 100 $this->assertSession()->waitForElementVisible('css', 'div.ui-dialog'); 101 // Use JS directly which is much faster than Element::find. 102 // We want to close it as fast as possible, within 20ms, corresponding to 103 // the debounce on Drupal.dialog.resetSize. 104 $this->getSession()->executeScript('document.querySelector(".ui-dialog button[title=\"Close\"]").click();'); 105 } 106 -
If we revert the actual fixes/changes in dialog.position.js, it does fail reliably, every time.
I just tested and got 10/10: Error: cannot call methods on dialog prior to initialization; attempted to call method 'option'
Sadly the "Test-only changes" pipeline doesn't work properly and seems to skip selenium tests: https://git.drupalcode.org/issue/drupal-3472624/-/jobs/3267868. Maybe I can create a new branch to prove the failures.
Edited by Hervé Donner -
So far I was not able to reproduce in the CI, but locally it fails correctly for me. I tested 2 different environments (local LAMP/selenium and ddev setup from https://github.com/justafish/ddev-drupal-core-dev), both fail as expected for the test-only branch.
69 93 $this->assertNotNull($close_button); 70 94 $close_button->press(); 71 95 96 // Test opening and immediately closing modal a few times. 97 // Ensure no JS errors are thrown. 98 for ($i = 0; $i < 10; $i++) { 99 $this->getSession()->getPage()->clickLink('Link 1 (modal)'); 63 63 * @fires event:dialogContentResize 64 64 */ 65 65 function resetSize(event) { 66 // Ensure the UI dialog instance exists/is valid. 67 if ( 68 !event.data || 69 !event.data.$element || 70 !event.data.$element.data('ui-dialog') changed this line in version 5 of the diff
mentioned in merge request !10069