Disable body-level scrolling when a dialog is open as a modal

What was the reason to create a patch in #90? It doesn't have tests, and more importantly - it's not working because if this line:

// Add body classes to indicate that the dialog is open.
body.addClass('drupal-dialog-open');

body should be prefixed with $

I'm hiding the patch anyway because it should not be applied.

Updating attributions

Tagging field UX as those efforts will likely increase the use of dialogs.

Re-roll #89 for 10.0.x

Utkarsh_33 → closed merge request !4114

The last submitted patch, 96: 2707291-96.patch, failed testing. View results →

I was not able to test this MR until I removed if condition from
`
if (scrollBarWidth > 0) {
document.body.style.paddingRight = `${bodyPad + scrollBarWidth}px`;
}
`
as I was getting scrollBarWidth == 0.

Hiding the patches as they aren't relevant anymore

Testing MR 4131
Confirmed the issue by opening up Media Library in the ckeditor and placing my cursor outside the modal and I can scroll
Applying the MR and that issue has gone away.

Seems a few committers have been on this thread but to me seems the changes for assertAnnounceContains were out of scope of this issue.

There's a regression with Android Chrome as a result of this change. Added screenshots for before and after.

It looks like the module mentioned in #66 is using https://github.com/willmcpo/body-scroll-lock. Maybe we should consider adopting that too?

Now that we are adding a new vendor asset. Think the issue summary could be updated to include that. Also do we need a change record that a new asset is being added?

Tested it for IOS in safari browser.The body level scrolling is locked for IOS.Attached the screenshots which i tested.

Tested it for android.The body level scrolling is locked for android also.Attached the screenshots which i tested.

Problem/Motivation

When a dialog is opened with the "modal" option, all other elements on the page are hypothetically disabled. Even though those other elements are masked, you can still scroll-through the modal to the body, which is awkward and not expected behavior. This can lead to the user being confused after closing a modal as the page has scrolled past where they initiated the modal originally.

This patch disables body-level scrolling when a dialog is opened as a modal, which should address this problem.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

For change record.

Latest diff:

Discussed with @catch on Slack to confirm that from release management standpoint it would be acceptable to use one of the libraries proposed in #110. Since we are not 100% confident about any of the libraries, @catch recommended that we add the library as an asset of the dialog library to reduce the risk of someone using this as an API.

Discussed with @bnjmnm to get his take on which of the libraries we should use from #110, and he recommended that we compare the more-current but less-used https://github.com/rick-liruixin/body-scroll-lock-upgrade against the original library and verify the changes are fine. This would help rule out issues that might have been already-discovered if it had more usage. Also, since the library doesn't break BC, it might provide us an option of going to one of the other forks if this one doesn't work out well.

Looked at https://github.com/rick-liruixin/body-scroll-lock-upgrade with @lauriii and concluded that we currently can't use that library since we can't import from esm files in Drupal and there is no UMD release provided. I filed an issue requesting a UMD file.

But for now we can probably just move forward with the original MR4131 that uses tua-body-scroll-lock.

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.

lauriii → closed merge request !4333

lauriii → closed merge request !4131

I don't know what is going on with the MR's since neither of them seems to have the right commits 🤔. I guess we need to start from the patch in #120 again?

Could the changes from the 2 MRs be documented to be added to the patch?

+++ b/core/core.libraries.yml
@@ -875,6 +876,16 @@ tabbable:
+tua-body-scroll-lock:
...
+    assets/vendor/tua-body-scroll-lock/tua-bsl.umd.js: { minified: true }

One of the remaining feedback here is that we need to move the asset from its own library to be loaded directly as part of drupal.dialog library.

#130 appears to be addressed

Posted feedback on the MR

All threads appear to be addressed so think this can be moved back to RTBC.

Adding dependency evaluation.

Tested the behavior with @penyaskito on iOS and Android to confirm that the latest solution works with both platforms.

lauriii → credited penyaskito → .

Tested the behavior on the latest version with @penyaskito on iOS and Android 👍

Great to finally solve this. Discussed additional testing with @lauriii at Drupal Dev Days but due to the fact this mostly affects mobile browsers only and specifically Safari, that adding JS testing is somewhat pointless for Chrome - we should revisit this if one day we have explicit mobile tests via Browserstack or similar.

Also discussed the copy-paste of assertAnnounceContains() which we should consider moving to WebAssert in a followup.

Committed and pushed 6cfd0670af to 11.x. Thanks!

Isn't this being backported to 10.1?

As this requires a new dependency, those are usually only added to minor releases, not patch releases. See https://www.drupal.org/about/core/policies/core-change-policies/allowed-... →

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

Published the change record. Should a usage example be added?

Sorry to be late to the issue here, but did we consider using overflow:clip here

We should be able to use the has selector and that to achieve this with a few lines of CSS

I made https://codepen.io/larowlan/pen/QWzMzgo for a demo - works in Chrome but unfortunately Firefox doesn't support :has yet.

I wonder if we should mark this library as internal/deprecated knowing that when Firefox supports has we can drop it?

In response to the previous comment from @larowlan, I conducted a preliminary test on a desktop setup (no mobile testing yet), utilizing Gin as the backend theme. I integrated the following CSS adjustments:

body:has(.ui-dialog) {
  overflow: clip;
  .ui-front {
    height: 90vh !important;
    min-height: 90vh;
  }
  .ui-dialog {
    overflow-y: scroll;
    overflow-x: hidden;
    height:98vh !important;
    margin-top: 1vh;
    margin-bottom: 1vh;
    display: flex;
    flex-direction: column;
  }
}

The results were perfect. Both the Media modals and the Layout Paragraph Modal functioned seamlessly. Kudos for these insights!

Since this wasn't reverted should anything be done @larowlan?

I think this could be explored in a followup but we should leave the original issue as fixed.

follow up: 📌 Remove tua-body-scroll-lock in favor of a CSS + polyfill solution Active

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

Patch for 10.2.x branch

re-roll for 10.3.x