-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try: Prevent scrolling content under an open overlay modal #49369
Conversation
Size Change: +13 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in ce55fe8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4532170546
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tested well for me and had the desired effect. It'd be worth having others chime in to see if anything unintended pops up.
Some real work usage notes in WordPress/wporg-mu-plugins#371 (comment) |
Pinging some folks on Slack to get some more tests |
Tested in Chrome/Safari on desktop and also IOS safari (on my phone). You can't see it but my finger is wagging up and down trying to activate the scroll bar!! ios-safari.mp4LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a clever idea. Unfortunately in testing it looks like switching to position: fixed
causes the layout to shift unexpectedly. You can see it at desktop viewport widths if you set the Navigation block's Overlay Menu to Always:
The other time this issue might appear is for themes that override the navigation modal so that it doesn't cover the entire viewport. Here's a mocked up example:
I used the above CSS to mockup the above screenshot:
.wp-block-navigation__responsive-container {
margin: 100px !important;
background-color: #fa9999 !important;
}
It's an interesting problem to solve, though, so I'll just ping a couple of folks who've been doing more work on the Navigation block lately in case they have other ideas: @getdave @scruffian
Update: I wonder if it'd resolve things (at least part of the way) if we also added a |
Good question. I'm not too sure what's causing it exactly, except that when using |
Interesting problem! The StackOverflow answer suggests setting top, bottom, left, right to 0 along with the fixed position. One problem with this approach is that, if the user is scrolled half way down the page and opening the nav modal from a sticky header, the page will be scrolled all the way back up, which might not be desirable either. The article above suggests adding a bit of JS as a workaround, which we could potentially add to the nav modal view script. Off the top of my head, would |
Thank you all for diving in here. I know that @iandunn has also had challenges with this implementation in WordPress/wporg-mu-plugins#371. It really is quite tricky to get right. If you find something that works, feel free to push directly to this branch, or fork off of it. |
I'm happy to help but I can't reproduce this issue on trunk with the xcode simulator (iPhone 14; iOS 16.2). Might this be happening only on another version of iOS? |
Not sure what the latest version is, but I tested it with the latest version (I freshly spun up a new Xcode just for this). It's possible it's some layouts, not all. That said, I can confirm that this is something that has worked, then broken, then worked on a back and forth basis with Mobile Safari 🙈 |
I also think this kind of thing is susceptible to being different on device vs in the simulator. It can be triggered by certain swipes that are not as easy to do with the mouse on the simulator. |
As far as my understanding, it should actually be as identical as is possible. But I don't have a device to test with unfortunately. |
It should be, but in my experience with previous efforts there can be subtle differences especially when interactions are involved. Rendering is usually ok. I will have another look at this on device vs in simulator next week. 👍 |
If it helps in debugging, the original issue as found on WordPress.org only seems to happen when the menu with a search box is opened & the iOS keyboard pops up. When the keyboard is closed, the |
Thanks @ryelle! I can reproduce the issue on the simulator by adding a Search block to the nav and focusing it inside the modal before scrolling. So the bad news is |
Closing as it doesn't seem the right fix. |
What?
When the navigation block modal overlay is open, a helper class is added to the
html
element, applyingoverflow: hidden;
. This is used to prevent the scrolling of content underneath the open modal overlay. I believe this used to also work as intended for Mobile Safari, but this appears to have stopped working in recent versions. GIF showing that you can swipe/scroll content "below":This PR tries to address that by adding
position: fixed;
to the open overlay. It appears to fix it in my testing:I believe this may have also been explored in the past, and avoided as it would also change the height of the HTML content, thus losing your scroll position in the use case where you've scrolled down a long way, and then open the modal overlay from a sticky top nav. Your scroll position in this case does reset, but that appears to also be an issue in trunk. Any alternative suggestions?
Testing Instructions
Insert a navigation block, set it to always collapse.
Test on a desktop, small viewport, but ideally also test on a physical iphone, or the Xcode simulator, and verify that you cannot scroll content under the open overlay.