Problem/Motivation

If you have a jQuery UI dialog modal (a dialog having the modal configuration set to TRUE) and a Off canvas rendered at the same time (i.e. the dialog is opened via the Off canvas), the modal is repositioned incorrectly when resizing the window.

Here is an example using Layout Builder and the contrib. module Layout Builder Modal:

(thanks to kevinfunk for providing the GIF in #3074302)

The expected behaviour here would be that the modal keeps being centered on window resize.

You can also reproduce this behaviour in Drupal core:

            <a href="/node/add/page" class="use-ajax" data-dialog-options='{"width": 500,"modal": true,"autoResize":true}' data-dialog-type="dialog">Dialog</a>

            <a href="/node" class="use-ajax" data-dialog-renderer="off_canvas" data-dialog-type="dialog">Off canvas</a>

If you only would open the Dialog, it would resize and the position would be as expected. But if you first open the off canvas, and then the dialog the same behaviour as with Layout Builder Modal.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#39 interdiff-37_39.txt652 bytesgauravvvv
#39 3075236-39.patch6.52 KBgauravvvv
#37 interdiff_33-37.txt642 bytesmrinalini9
#37 D-11-3075236-37.patch6.49 KBmrinalini9
#36 3075236-nr-bot.txt3.11 KBneeds-review-queue-bot
#35 D-11-3075236-33.patch6.51 KBgauravvvv
#34 interdiff-31-34.txt694 bytesgauravvvv
#34 d9.5-3075236-34.patch7.65 KBgauravvvv
#33 drupal.org_files_issues_2023-07-12_3075236-22-reroll.patch7.59 KBgauravvvv
#32 3075236-nr-bot.txt92 bytesneeds-review-queue-bot
#31 3075236-22-reroll.patch7.59 KBseanb
#30 3075236-nr-bot.txt85 bytesneeds-review-queue-bot
#27 3075236-27.patch7.57 KBricovandevin
#24 3075236-nr-bot.txt144 bytesneeds-review-queue-bot
#22 interdiff_16-22.txt762 bytesranjith_kumar_k_u
#22 3075236-22.patch7.54 KBranjith_kumar_k_u
#16 interdiff.txt587 bytesraman.b
#16 reroll_diff_12-16.txt3.49 KBraman.b
#16 3075236-16-reroll.patch7.28 KBraman.b
#16 3075236-16.patch7.29 KBraman.b
#16 3075236-16-test-only.patch5.85 KBraman.b
#15 3075236-15.patch7.52 KBseanb
#12 interdiff_10-12.txt1.81 KBjohnwebdev
#12 3075236-12.patch7.19 KBjohnwebdev
#10 interdiff_9-10.txt513 bytesjohnwebdev
#10 3075236-10.patch7.63 KBjohnwebdev
#9 interdiff_7-9.txt2.19 KBjohnwebdev
#9 3075236-9.patch7.75 KBjohnwebdev
#7 3075236-7.patch7.39 KBjohnwebdev
#4 3075236-4-FAIL.patch5.66 KBjohnwebdev
Aug-12-2019 08-55-28.gif11.16 MBjohnwebdev

Comments

johndevman created an issue. See original summary.

johnwebdev’s picture

johnwebdev commented
Issue summary: View changes
Related issues: +#3074302: Window Resize Positions Modal Top Right
johnwebdev’s picture

johnwebdev commented
Issue summary: View changes
johnwebdev’s picture

johnwebdev commented
Status: Active » Needs review
StatusFileSize
new 3075236-4-FAIL.patch5.66 KB

Here is a failing test. I'm thinking we should probably change the assertion just to ensure it's not repositioned on the top right corner on a resize instead of doing some guessing with where it's approximately is supposed to be using the left property. Nonetheless it's the start, and it shows the bug.

Status: Needs review » Needs work

The last submitted patch, 4: 3075236-4-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnwebdev’s picture

johnwebdev commented

Trying to debug and from what I can see is that Drupal.offCanvas.resetSize targets the dialog opened after the off canvas instead of the off canvas div. I'm not sure why that happens though.

It looks like the event listener stored does point to the correct element. For fun, I decided to remove the debounce() around the resize, and that seemed to resolve the issue.

johnwebdev’s picture

johnwebdev commented
Status: Needs work » Needs review
StatusFileSize
new 3075236-7.patch7.39 KB

Let's see if test passes when removing the debounce() function.

Status: Needs review » Needs work

The last submitted patch, 7: 3075236-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnwebdev’s picture

johnwebdev commented
Status: Needs work » Needs review
StatusFileSize
new 3075236-9.patch7.75 KB
new interdiff_7-9.txt2.19 KB

Reworked the test a bit.

johnwebdev’s picture

johnwebdev commented
StatusFileSize
new 3075236-10.patch7.63 KB
new interdiff_9-10.txt513 bytes

@group should still be dialog though :)

johnwebdev’s picture

johnwebdev commented
Status: Needs review » Needs work

Although this passes I believe we should look in to why this happens, and see if we can fix that. Removing the debounce would affect the performance.

johnwebdev’s picture

johnwebdev commented
Status: Needs work » Needs review
StatusFileSize
new 3075236-12.patch7.19 KB
new interdiff_10-12.txt1.81 KB

Okay, this brings back the debounce.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Version: 8.9.x-dev » 9.0.x-dev
StatusFileSize
new 3075236-15.patch7.52 KB

Reroll for Drupal 9.

raman.b’s picture

raman.b commented
Version: 9.0.x-dev » 9.2.x-dev
StatusFileSize
new 3075236-16-test-only.patch5.85 KB
new 3075236-16.patch7.29 KB
new 3075236-16-reroll.patch7.28 KB
new reroll_diff_12-16.txt3.49 KB
new interdiff.txt587 bytes

Re-rolling for 9.2.x

We also need to account for the 1px border added by .ui-widget.ui-widget-content

The last submitted patch, 16: 3075236-16-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

design.er’s picture

design.er commented

Patch #16 is working great for me on multiple projects with 9.1.5.
Thanks for taking care of that! :)

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 3075236-22.patch7.54 KB
new interdiff_16-22.txt762 bytes

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3075236-nr-bot.txt144 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

meanderix’s picture

meanderix commented
Related issues: +#3350972: [random test failure] Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest::testReloadWithNoSections()

This patch is failing after changes made in #3350972: [random test failure] Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest::testReloadWithNoSections() (Drupal 9.5.9). Are these two patches fixing the same thing?

Here's the patch that I think went into core:
https://www.drupal.org/files/issues/2023-04-10/3350972-9.4_9.5-16.patch

Version: 10.1.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, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ricovandevin’s picture

ricovandevin commented
StatusFileSize
new 3075236-27.patch7.57 KB

Reroll

vlad.dancer’s picture

vlad.dancer
Primary language Ukrainian
Location Kyiv
commented
Status: Needs work » Needs review
vlad.dancer’s picture

vlad.dancer
Primary language Ukrainian
Location Kyiv
commented

Patch from #27 applies for 9.5.10. Thank you @ricovandevin.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3075236-nr-bot.txt85 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Needs work » Needs review
StatusFileSize
new 3075236-22-reroll.patch7.59 KB

Patch in #27 failed to apply. Here is another attempt for a reroll of #22 for 9.5.10.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3075236-nr-bot.txt92 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new drupal.org_files_issues_2023-07-12_3075236-22-reroll.patch7.59 KB

I have re-rolled patch #31, for D11. Not adding interdiff as the patch doesn't applies anymore to d11.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new d9.5-3075236-34.patch7.65 KB
new interdiff-31-34.txt694 bytes

Added patch for 9.5 along with interdiff with #31.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new D-11-3075236-33.patch6.51 KB

Wrong patch uploaded in #33, added the correct patch for d11.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3075236-nr-bot.txt3.11 KB

The 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 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.

mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review
StatusFileSize
new D-11-3075236-37.patch6.49 KB
new interdiff_33-37.txt642 bytes

Fixing custom command failure issues in #35, please review it.

Thanks & Regards,
Mrinalini

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

CC failure

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3075236-39.patch6.52 KB
new interdiff-37_39.txt652 bytes

Tried fixing CC failures, attached interdiff for same

Status: Needs review » Needs work

The last submitted patch, 39: 3075236-39.patch, failed testing. View results

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.