-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Wrap overflow: auto in media query for smaller screens only #66176
Wrap overflow: auto in media query for smaller screens only #66176
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Probably could use some more cross-device and browser size testing. |
This also addresses #66156 |
Hello @colorful-tones, I have tested this PR using the Gutenberg PR reviewer, as well as in my local Chrome and Firefox browsers across various screen sizes, and found that it resolves the issue perfectly. I’m also attaching a video of the testing for your reference. Thanks! 66176-chrome.mp4 |
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.
Thanks for the PR!
I remember having a similar problem a few times in the past 😅
It works as expected on Windows OS and Firefox/Chrome.
One thing I noticed is that overflow:auto
itself causes double scrolling in the mobile layout.
Below is a video on trunk. Note that you need to shift the screen to the right after entering a keyword for search:
8267d3ec8bf83b181fe5cf6ea6742907.mp4
It's not an issue with this PR, but I think we need an approach that doesn't rely on overflow:auto
in the future.
I'll also ping @stokesman and @talldan, who have been involved with this issue in the past.
I tested this PR, I don't think it's the right approach - the removed
This was a fix for the device previews third scrollbar in 6.5 - #62952. The The problem that occurs when searching for blocks has, in the past, been triggered by visually hidden text that overflows outside the inserter sidebar (see #44853, and I'm pretty there was also a fix in 6.5 as well). I'm not sure if it's the same issue that we're seeing now though, the fixes from previous releases still seem to be there. |
The main issue I was trying to fix on that PR is that the "height" prop of the block canvas should actually work (you can confirm this by running the Box story in storybook) and that the "width" of the canvas is 100%. I do remember removing that flex but I'm not sure about the reason, I thought it was breaking something but after a quick manual check, I'm not sure if it breaks anything. So we could consider restoring it and just checking the height/width of the Box story. |
I pushed up another change that addresses this.
Yes, I remember staring at that exact line of code |
After the latest change, the PR doesn't seem to be fixing either of the bugs unfortunately. I now get the extra scrollbars again when testing. I think it might be worth looking to reinstate the I did some further digging into the issue with the inserter and it looks like it's caused by this bit of VisuallyHidden text: gutenberg/packages/block-editor/src/components/inserter/search-results.js Lines 186 to 190 in d330f30
I'm not sure why it suddenIy caused an issue (the code has been there for a while), perhaps some changes to the styles. I put together a PR to fix it - #66229. |
Yes, or otherwise find another way to fix #66156. |
Now that #66229 has been merged, can we close this PR and the following issue? |
What?
Addresses #66161 and #66156
Why?
The appearance of double vertical scrollbars was occurring in certain instances of the interface where the viewport height was small. It seemed to be triggered by the
overflow: auto
, which had a previous code comment indicating that theoverflow: auto
was implemented to circumvent issues with mobile Safari. Based on the commentary it seems we would want theoverflow: auto
to only impact smaller horizontal screens and not smaller vertical screens.How?
By wrapping the
overflow: auto
in a media query:@media (max-width: #{ ($break-medium - 1) })
we make sure the overflow is only applied to smaller horizontal screens.Testing Instructions
The original bug reported in #66161 impacts small vertical screens.
Screenshots or screencast
double-scrollbar-before.mp4
double-scrollbar-after.mp4