Skip to content
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

Conversation

colorful-tones
Copy link
Member

@colorful-tones colorful-tones commented Oct 16, 2024

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 the overflow: auto was implemented to circumvent issues with mobile Safari. Based on the commentary it seems we would want the overflow: 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.

  1. Open the block editor on a desktop device with a small vertical height.
  2. Open the block inserter sidebar and note the right-most vertical scrollbar(s)
  3. Begin typing to find a block and note the right-most vertical scrollbar(s)

Screenshots or screencast

Before After
double-scrollbar-before.mp4
double-scrollbar-after.mp4

@colorful-tones colorful-tones marked this pull request as ready for review October 16, 2024 16:25
Copy link

github-actions bot commented Oct 16, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: colorful-tones <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: dhruvang21 <[email protected]>
Co-authored-by: talldan <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: stokesman <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@colorful-tones
Copy link
Member Author

Probably could use some more cross-device and browser size testing.

@colorful-tones
Copy link
Member Author

This also addresses #66156

@dhruvang21
Copy link
Contributor

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

Copy link
Contributor

@t-hamano t-hamano left a 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.

@talldan
Copy link
Contributor

talldan commented Oct 17, 2024

I tested this PR, I don't think it's the right approach - the removed overflow: auto makes the Code Editor mode unscrollable. I have sympathies as trying to navigate through these styles is very challenging.


I'll also ping @stokesman and @talldan, who have been involved with this issue in the past.

This was a fix for the device previews third scrollbar in 6.5 - #62952. The display: flex that was added there to fix things was removed by this PR - #65977 (cc @youknowriad). Adding it back again does seem to fix the issue again, though maybe it also regresses something that Riad was trying to fix. I should've added a comment above the line in my original PR to explain what it was for.

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.

@youknowriad
Copy link
Contributor

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.

@colorful-tones
Copy link
Member Author

the removed overflow: auto makes the Code Editor mode unscrollable.

I pushed up another change that addresses this.

I have sympathies as trying to navigate through these styles is very challenging.

Yes, I remember staring at that exact line of code overflow: auto during the WP 6.5 release and trying to fix a layout issue. It has been haunting me, and surfacing again and again.

@talldan
Copy link
Contributor

talldan commented Oct 18, 2024

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 display: flex if we can be sure it doesn't regress the story mentioned by Riad.

I did some further digging into the issue with the inserter and it looks like it's caused by this bit of VisuallyHidden text:

<InserterPanel
title={
<VisuallyHidden>{ __( 'Block patterns' ) }</VisuallyHidden>
}
>

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.

@stokesman
Copy link
Contributor

I think it might be worth looking to reinstate the display: flex if we can be sure it doesn't regress the story mentioned by Riad.

Yes, or otherwise find another way to fix #66156.

@t-hamano
Copy link
Contributor

Now that #66229 has been merged, can we close this PR and the following issue?

@stokesman
Copy link
Contributor

Now that #66229 has been merged, can we close this PR and the following issue?

I still see #66156 in trunk and in the wp/6.7 branch so it appears yet to be resolved. I’ve opened up #66494 aimed to fix it.

@t-hamano
Copy link
Contributor

I think the problem this PR is trying to solve is solved by #66229 and #66494.

@t-hamano t-hamano closed this Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Interface /packages/interface [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants