Updated: Comment #0

Problem/Motivation

When a modal is open and the entire modal isn't visible on the screen, you can't scroll down to access the Save button.

There are a few other issues going with modal updates, however this is patch is simply to fix the bug with scrolling.

Proposed resolution

Patch from nod_ #9 in #2067263 fixes this bug.

Steps to test:
Before patch (using Chrome, haven't replicated in other browsers)

  1. On a clean Drupal 8 install, go to admin/structure/block
  2. Click on one of the blocks to place (or go to another page and open a modal)
  3. Ensure that the entire modal is not visible in your browser window
  4. Try to scroll down to click Save
  5. Scrolling doesn't work, you have to refresh the page
  6. Same problem in admin/config/development/sync page
  7. Same problem in node/add when trying to insert an image with WYSIWYG editor

After patch

  1. Apply patch
  2. Do the same
  3. Scrolling works

#2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement
#2072037: Drupal dialog modal background z-index is set too low to reliably occlude core UI components

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pameeela’s picture

Status: Active » Needs review
meba’s picture

Status: Needs review » Needs work

To be clear, this happens on windows that are not tall enough to fit the modal, correct? I don't think the patch is a full fix - on windows like that the window then does not fit. Maybe we need to calculate the height of the window and height of the modal and only disable autoResize if the modal is bigger than window?

nod_’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

After reviewing where autoresize is used, turned out it gets in the way more than it helps and the initial justification to get it in is not convincing #1879120-75: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms.

Patch takes out autoresize and makes dialogs works everywhere.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies well and it fixes the problem so let's wait for testbot and RTBC! :)

pameeela’s picture

@meba that may be a cleaner solution but for now I think it's critical to get a fix in. Maybe you can create a follow up issue to implement that?

Tested the patch and it resolves the issue of not being able to access the entire modal.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
75.3 KB

This is definitely not RTBC. This is not fundamentally broken, because it works fine here:

  1. Resize your browser to be e.g. 200 px high.
  2. Go to node/add/article.
  3. Click the "add image" button.

It'll look like this:
Screen Shot 2013-09-22 at 17.18.41.png

So we should look into why it breaks on admin/structure/block.


Removing autoResize, like #5 does, is problematic too, because now you can scroll your dialog away while your cursor is on the black "jQuery UI overlay" (not the Drupal overlay) that sits between the Drupal site and the jQuery UI Dialog. That's extremely weird.

You should never be able to scroll away a dialog. You should be able to scroll the contents of the dialog, not the dialog itself.

pameeela’s picture

It's also broken at admin/config/development/sync when you click view difference, if there is more content than fits the screen. You can scroll down to view it all but it immediately jumps back to the top.

pameeela’s picture

Priority: Normal » Major

Changing this to major. It stops me saving blocks settings on a 13-inch laptop, seems a pretty wide use case.

@Wim is it not possible to get this fix in as a workaround with the understanding that a better approach is needed afterward?

nod_’s picture

Assigned: Unassigned » nod_

ok, deeper bug with the ajax dialog integration.

nod_’s picture

Assigned: nod_ » Unassigned
Status: Needs work » Needs review
FileSize
3.3 KB

So many fails, so little lines.

When using data-dialog-options, the ajax framework wasn't putting the default dialog options either.

Still not happy with the code but now it works at least.

pameeela’s picture

Patch fixes the bug so you can scroll. But now the modal opens with the bottom visible rather than starting from the top, though you can scroll up. Screenshot attached is what you get when you click in view differences.

Synchronize configuration   drupal8.localhost.png

Not sure what to mark this one as we've still got the page scrolling behaviour. I think I'll leave it for more feedback :)

nod_’s picture

lol I hate this code.

It was a scope issue.

nod_’s picture

wrong patch.

pameeela’s picture

Status: Needs review » Needs work

Patch fixes the bug at admin/structure/block and admin/config/development/sync and the scrolling is inside the modal.

But there's a regression at node/add/article when inserting an image via WYSIWYG, it scrolls the whole page rather than just the modal.

nod_’s picture

BAM!

nod_’s picture

Status: Needs work » Needs review

double BAM!

Status: Needs review » Needs work

The last submitted patch, core-js-resiez-2095225-15.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
3.81 KB

Triple BAM!

forgot to clean up my git repo before diffin'

rteijeiro’s picture

Patch applies well and seems to fix the problem. Let's see what @pameeela thinks and go for RTBC!

pameeela’s picture

Status: Needs review » Reviewed & tested by the community

It works! It works!

Tested at:

admin/structure/block
admin/config/development/sync (with a long diff)
node/add/article (when inserting an image via WYSIWYG)

and the scrolling is within in the modal.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

OMG THIS IS MY FAVOURITE PATCH IN THE WORLD EVER!!!

Committed and pushed to 8.x. :D

nod_’s picture

omg, fastest js commit ever by webchick \o/

Check out #787896-46: Add a link so that administrators can return to their most recently visited non-admin page too, it will end up making people stop bitching about the overlay.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Added other pages where you can reproduce the problem.