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)
- On a clean Drupal 8 install, go to admin/structure/block
- Click on one of the blocks to place (or go to another page and open a modal)
- Ensure that the entire modal is not visible in your browser window
- Try to scroll down to click Save
- Scrolling doesn't work, you have to refresh the page
- Same problem in admin/config/development/sync page
- Same problem in node/add when trying to insert an image with WYSIWYG editor
After patch
- Apply patch
- Do the same
- Scrolling works
Related Issues
#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
Comment | File | Size | Author |
---|---|---|---|
#18 | core-js-resiez-2095225-17.patch | 3.81 KB | nod_ |
#15 | core-js-resiez-2095225-15.patch | 4.23 KB | nod_ |
#13 | core-js-resiez-2095225-12.patch | 3.81 KB | nod_ |
#12 | core-js-resiez-2095225-12.patch | 4.53 KB | nod_ |
#11 | Synchronize configuration drupal8.localhost.png | 50.94 KB | pameeela |
Comments
Comment #1
pameeela CreditAttribution: pameeela commentedComment #2
meba CreditAttribution: meba commentedTo 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?
Comment #3
nod_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.
Comment #4
rteijeiro CreditAttribution: rteijeiro commentedPatch applies well and it fixes the problem so let's wait for testbot and RTBC! :)
Comment #5
pameeela CreditAttribution: pameeela commented@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.
Comment #6
Wim LeersThis is definitely not RTBC. This is not fundamentally broken, because it works fine here:
node/add/article
.It'll look like this:
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.
Comment #7
pameeela CreditAttribution: pameeela commentedIt'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.
Comment #8
pameeela CreditAttribution: pameeela commentedChanging 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?
Comment #9
nod_ok, deeper bug with the ajax dialog integration.
Comment #10
nod_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.
Comment #11
pameeela CreditAttribution: pameeela commentedPatch 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.
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 :)
Comment #12
nod_lol I hate this code.
It was a scope issue.
Comment #13
nod_wrong patch.
Comment #14
pameeela CreditAttribution: pameeela commentedPatch 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.
Comment #15
nod_BAM!
Comment #16
nod_double BAM!
Comment #18
nod_Triple BAM!
forgot to clean up my git repo before diffin'
Comment #19
rteijeiro CreditAttribution: rteijeiro commentedPatch applies well and seems to fix the problem. Let's see what @pameeela thinks and go for RTBC!
Comment #20
pameeela CreditAttribution: pameeela commentedIt 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.
Comment #21
webchickOMG THIS IS MY FAVOURITE PATCH IN THE WORLD EVER!!!
Committed and pushed to 8.x. :D
Comment #22
nod_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.
Comment #23.0
(not verified) CreditAttribution: commentedAdded other pages where you can reproduce the problem.