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

You do need some padding to left (or right) to avoid element shifting. Check bootstrap's modal as example :)

Thanks.

+++ b/core/misc/dialog/dialog.css
@@ -0,0 +1,10 @@
+  position: fixed;

but this trigger `body` scrolls back to the top.

were not calling closeDialog() when closing

Yeah, that's a case. We created our implement in Drupal.dialog and that's too easy to be overridden.

Great! I have a quick testing and it works as expected. I will run through the code review soon. and hope somebody able to have a 2nd review also :)

+++ b/core/misc/dialog/dialog.js
@@ -88,12 +104,48 @@
+      // Workaround for missing window.innerWidth in IE8.

We don't support IE8 in D8.

Is it not enough to just cancel the bubbling of scroll events on the dialog?

Tested in administration theme from dialogs prompted from both WYSIWYG, Views, and fields triggering a dialog. I also tested it on the frontend where the theme is calling the jquery UI Dialog, both worked as intended. I also tested manually closing and 'esc' key closing.

@Wim,

I'm afraid not. You can't cancel `document` level scroll event. You can prevent mouseEvent, or touchEvent but can't disable native scrollbar scroll interactive.

You may able to set:

window.scrollTo(0, y);

But it leads to performance & flickering issue I believed.

Alternative way is set `body` to position: absolute; (It's slightly better than position: fixed;.) However, it really works for a pre-designed site only. Not scalable for a framework. eg. Facebook uses that technique. For all other sites, we can't predict how the body & inner elements styling, that would trigger more elements shifting & reflow.

I hope I'm wrong because that would be less code :)

I figured that what I said in #10 would not be possible, because such an obvious, simple solution would of course have been tried first. Just wanted to double-check. Thanks!

Adding #2672344: [UX] Open as Modal should not able to interact with other elements as related

I don't agree with the issue.

The <dialog> spec doesn't say scrolling should be disabled when a modal is open. And so far neither firefox nor chrome prevent scroll on modal open: http://demo.agektmr.com/dialog/ Since the dialog element was intended are more or less a polyfill for this element, I don't think we should add code for this.

Sounds right by reading the SPECS. Anyway, only Chrome supported `dialog`.

So now, the question is "Do we need it for Drupal? and any UX problems by this change?"

It's all about how we code and we think.

A. We can hook it into `dialog.beforecreate` event. In this way, we are changing other elements in the standard way, not the standard dialog itself.
B. We can think that Drupal.dialog is our custom API for dialog than a polyfill, we doing extra scripting before trigger `show` event. We doing that now, dialog.postion.js is an example. ( we also changing the A tag, Submit buttons ..etc ).

** the main point is all our changes should be adaptable to HTML 5.1+ standard in future.

I don't see usability reasons to prevent scrolling in the underlying page.

There's one little problem is,

in macbook like devices, that's no visible scrollbar. When you are using touchpad to scroll it in non-scrollable area, the bg will be moved. after closing the modal, you will get a different screen. that may confusing users if their actions inside the modal changed the main screen contents.

By HTML SPEC, dialog scrolling with the page by default .

I recommend trying the patch, it addresses the scroll bar issue.

Also, I recommend this be added as an option in appearance/settings (global settings) for the site administration to choose if they want to disable scroll or not. In most cases a scrolling background makes no logical sense. If the background page is greyed out, it means it's inaccessible to the user until an action is taken.

The patch works fine, and it does feel more natural to not have scrolling in the background. We do not want to make this a setting, that would just add clutter to the admin interface for a very minor feature that will be hard to explain what it even does. We should come to a decision and act on it. It seems better to me to follow relevant specs than fix a potential usability problem that we don't even know it exists.

fair point on the checkbox being confusing.

I am having a hard time understanding how it's not a usability problem. The modal applies an overlay over the entire site outside it's contents, making anything behind it unusable. having something scroll behind suggests that actions can be taken on the page outside the modal (like browsing the page, for example) which is not true.

Recall for UX feedback before my code reviews and improvement. Thanks

This does not patch clean against 8.6.x-dev.

Updated patch for 8.6

Here's the updated patch for 8.6 based on @samuel.mortenson changes.

Also to note, I just figured that there is a bug on iOS that makes the body scrollable when the keyboard is opened - in a case when we have a form inside the modal. Somehow, in this case, the fix with overflow: hidden does not apply. I didn't find a proper solution to this yet.

We can't just set "overflow: hidden;" for body tag, because it doesn't work on Safari browser on all devices (iphone, ipad, mac, whatever) -> for such devices "position: fixed;" works good.
So need to think how to implement only one fix, but globally. This is common issue with dialog popups, for now to fix such issues i use additional js that calculates offset between top of page and top of your current window position, then "position: fixed;" for body tag + "top" property with negative value, like

<body class="is-path-frontpage no-overflow" style="top: -578px;">

and

body.no-overflow {
  position: fixed; 
}

But need to be careful when you are under admin user and you have toolbar on the top + default padding-top for body coz this toolbar. So need to find simple solution for all cases.

@sasanikolic thnx! #31 works fine for me against 8.6.2

https://github.com/willmcpo/body-scroll-lock works perfect in all browsers
How we can put it in drupal?

Enjoy it, my friend:
https://www.drupal.org/project/body_scroll_lock

These all fixes are done with this module ( https://www.drupal.org/project/modal_page ) except ipad, iphone.
replace this

body.modal-open { 
overflow:hidden; 
}

with

body.modal-open { 
position:fixed; 
}

will fix the issue.

Added a patch which pointed #42.

Re-rolled for 9.2.x.

Fixed custom command failures in #51 patch, thanks.

Tried to solve the custom command failures.

Tried to fix the custom command failure

another try

Confirming patch in #51 (misnamed as #49) applies and still resolves this issue in 9.2.0.

Tried to solve the command failures.

Tried solving command failures again

Re-rolled patch #61, Attched interdiff for same. Added missing dialog.es6.js file and re compiled. please review

Ignore above patch, it consists yarn update changes.

We have a css property that can help because this seems like a lot of code.

https://css-tricks.com/almanac/properties/o/overscroll-behavior/ it doesn't work on safari thought. I would try to go this way and find a safari-specific polyfill.

@nod_

both are diff cases I think. CSS one is prevent accidental scroll, this patch is prevent intended scroll (you have no way to scroll the body anymore) also.

both may also be needed.

(and needs mobile touching testings)

also,

even more code is needed
https://www.drupal.org/project/body_scroll_lock

Fix for jenkins build

Re #64

https://css-tricks.com/almanac/properties/o/overscroll-behavior/ it doesn't work on safari thought. I would try to go this way and find a safari-specific polyfill.

Update:
Safari is green now from v16 on!

Shouldn't we at last add a body class, indicating the modal is open and go on with everything else then? I guess that would be helpful in many projects and perhaps we can already agree on that? (This is open since 2016 ...)

Adding a body class would be great. That's not intrusive and still allows each site to have a fixed background with minimal effort.

#68 since I had to ask about the error it stands to reason that others here probably would have to as well. It's not a very well worded error.

'/var/www/html/core/misc/dialog/dialog.es6.js' is not updated.

effectively means: dialog.js should match the output of yarn build dialog.es6.js

Also, #67 and #68 are missing the new css file.

#64 I've done some testing and sadly overscroll-behavior doesn't completely block the body from scrolling AND it ONLY works if there's a scrollbar inside the modal /sadface

A colleague suggested the new inert attribute but that only blocks clicks, not scrolling.

Here's a patch against 9.4.x.

I can't provide an interdiff because I used bits and pieces of different prior patches and effectively rewrote it by hand. The .js file is now properly compiled from the .es6.js file.

I found the 'position: fixed' CSS reset the scroll position of the body so I used 'overflow: hidden' instead.

Scrollbar measurement doesn't work yet.

This should do the trick. I note that it leaves a strip of transparent space in place of the scrollbar. Not sure if that's the best thing to do.

Forgot to remove the unnecessary CSS. Third time's the charm?

Really don't need a library just for one line of CSS.

Argh, apologies for the spam, found another issue with the patch. Overflow style should only be applied for modals, not all dialogs.

Manual testing of the first failure shows it's working fine. I think there may be a quirk in how the test driver looks for visible content?

Asked around in #bugsmash on Slack and @ambermatz and @mstrelan both agree with me that this is a usability bug, not just a feature request.

Discovered a small issue with removing the body padding style.

Thanks to @mstrelan for the chat on slack where they pointed out that getText() is intended only for visible text as per the documentation

java.lang.String getText()
Get the visible (i.e. not hidden by CSS) text of this element, including sub-elements.

This means that it should not be used for the live-announce system which is where these tests are failing.

For illustration purposes I'm uploading a patch which passes those tests locally.

I assume this is a big enough scope creep to warrant a separate issue so I'm going to go make one now and then postpone this issue on it.

Hahaha, of course. As soon as I finish with that comment I discover via #3087213: Create a test trait for assertAnnounceContains and assertAnnounceNotContains that there's already a standard for assertAnnounceContains.

New patch uploaded which alters the failing tests to use that instead.

I'm no longer sure this needs a separate issue for the test updates.

#86 patch re-rolled for core 9.5.x.

Patch #86 re-rolled again for core 9.5.x, forgot to add tests.

For 9.5.x going off what #51 created.

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.

Others seeing this: Dont apply #90
As said by Matroskeen: It breaks dialogs.

Uncaught ReferenceError: body is not defined

Updating attributions

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

Re-roll #89 for 10.0.x

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?

Edit: Looks like the that library is abandoned. We could consider adopting one of the forks, e.g. https://github.com/tuax/tua-body-scroll-lock or https://github.com/rick-liruixin/body-scroll-lock-upgrade. It seems a fairly small UI library so even if we had to fork it ourself, wouldn't probably be the worst thing.

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.

Updated issue summary.

For change record.

Latest diff:

[Ah shucks, the latest diff doesn't apply to Drupal 10 :(]

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, there is no UMD release provided, and bundling it ourselves to make it compatible feels like overkill for this issue. 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.

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.

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

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;
}
body  .ui-front {
  height: 90vh !important;
  min-height: 90vh;
}
 body .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: #3388408: Remove tua-body-scroll-lock in favor of a CSS :has()

Patch for 10.2.x branch

re-roll for 10.3.x

This seems to be "fixed" in 10.4. Can someone suggest how to revert this back to the way it was in D9 (despite everyone saying no one would want to scroll the background when a modal is open, my client is pretty annoyed this is now broken).

@liquidcms A true revert would either be a patch or overriding the two JS files updated but the quickest way to go back to the earlier behavior is probably to disable the scroll lock immediately after it is activated.

in a custom module add a jQuery event listener on 'dialog:aftercreate', which is triggered immediately after the scroll lock is added.

In the callback, run the same code that is run when the modal dialog closes

const $scroll = $element.find('.scroll');
    if ($scroll.length) {
      bodyScrollLock.unlock($scroll.get(0));
    }

This might not be it line-for-line, I didn't actually try it, but this approach should work - fortunately an event is dispatched right after the scroll lock so it's quite available for disabling.

(function ($, Drupal, bodyScrollLock) {
  'use strict';
  Drupal.behaviors.foo = {
    attach: function(context, settings) {
      $(window).on('dialog:aftercreate', function(event, dialog, $element, settings) {
        bodyScrollLock.unlock($element.get(0));
      });
    }
  }
})(jQuery, Drupal, bodyScrollLock);

You can disable it like this. In our experience having this enabled has created major issues in IOS with users unable to scroll dialogs that are larger than the viewport. It might be specific to our use case and we have not look at it beyond this yet.