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.

Comments

samuel.mortenson created an issue. See original summary.

droplet’s picture

Status: Needs review » Needs work

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

samuel.mortenson’s picture

Thanks @droplet, I'll look into that. I found another bug in my patch where some modals were not calling closeDialog() when closing, which means that the body remained locked even after a modal was closed. You can see this by going into WYSIWYG, changing the text format to "Full HTML", and agreeing to the alert about unsafe markup.

We'll probably need to subscribe to the jQuery UI Dialog events globally (on $(window)) and add a class instead of inline CSS, which would let specific themes override our assumptions about what CSS rules are best for accomplishing this.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
1.89 KB

Here's a new patch which uses classes instead of inline CSS, and adds a position: fixed rule to body to prevent shifting.

droplet’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work
Issue tags: +CSS

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.

The last submitted patch, 4: drupal-dialog-modal-disable-scroll-2707291-4.patch, failed testing.

samuel.mortenson’s picture

Ah position: fixed was probably not needed then - I understand now that you meant shifting due to the scrollbar disappearing, which Bootstrap handles by adding an arbitrary margin when locking body down. I'm looking into solutions for this now.

Edit: See https://github.com/twbs/bootstrap/issues/9855 for the Bootstrap history of this

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
5.02 KB

This is going to make this issue harder to review, but the new patch prevents scrolling while dialogs are open as modals as well as preventing shifting of body elements due to the scrollbar disappearing. To accomplish this I copied code from https://github.com/twbs/bootstrap/blob/master/js/modal.js and https://davidwalsh.name/detect-scrollbar-width, as Bootstrap has iterated on this logic for awhile now.

I haven't specifically credited these sources in the patch, but am happy to do so if there's a standard comment in core for this. Also, if this added complexity is unwanted in core and people are happy with the body shifting when a modal is open, I can go back to the most simple patch (which is just the overflow: hidden rule).

droplet’s picture

Issue tags: +Usability

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.

Wim Leers’s picture

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

Bcwald’s picture

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.

droplet’s picture

@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 :)

Wim Leers’s picture

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!

nod_’s picture

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.

droplet’s picture

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.

yoroy’s picture

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

droplet’s picture

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 .

Bcwald’s picture

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.

yoroy’s picture

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.

Bcwald’s picture

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.

droplet’s picture

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.