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

Performances: Avoid UI shifting when selecting blocks #47177

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

corentin-gautier
Copy link
Contributor

@corentin-gautier corentin-gautier commented Jan 16, 2023

What?

Force the use of overflow scroll on scrollable areas to avoid UI shifting/resizing when getting a scrollbar

Why?

When the content of a scrollable area changes and overflow, the whole page resizes triggering massive repaint of the page

How

Force overflow: scroll on both .interface-interface-skeleton__content and .interface-interface-skeleton__sidebar

Fixes: #45386
Fixes: #23427

Testing Instructions

  1. Open a page
  2. collapse/expand panels in the sidebar to create overflow
  3. the blocks container should not change its size because of the sidebar scrollbar appearing

Before

Screen.Recording.2023-01-16.at.10.35.32.mov

After

Screen.Recording.2023-01-16.at.10.36.13.mov

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @corentin-gautier , thank you for working on this one!

I left some technical feedback, but I'll @jasmussen and @youknowriad give more feedback on this one

@@ -99,6 +99,10 @@ body.block-editor-page {

@include wordpress-admin-schemes();

.interface-interface-skeleton__content {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding this override, could we see if we can rework the original overscroll: auto prop in the interface package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken the site editor now uses an iframe, meaning the overflow auto here isn't particularly useful since it's the iframe that now handles the scroll

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which also means we can't set it to scroll (it would result in a double scrollbar)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!

What I'm trying to say is, we couldn't set those styles for .interface-interface-skeleton__content and .interface-interface-skeleton__header (at the moment scrollbar-gutten and overflow) directly at the source (in the @wordpress/interface package), instead of overriding them specifically for the @wordpress/edit-post package.

@jasmussen
Copy link
Contributor

Thank you for the PR! This particular issue bugs me a ton as well and I really wish there was a better way to handle it. The main problem is the inspector, which shifts so often when opening/closing panels, or selecting different blocks, it can really become quite jumpy.

But at the same time, forcing the scrollbar track to be visible always, it provides a somewhat more clunky interface for the default case. Here's the before state, with or without auto-hiding MacOS scrollbars:

before

Here's the after-state with auto-hiding scrollbars:

after - auto hiding scrollbars

More to the point, here's the after-state with permanently visible scrollbars:

after, no auto hiding scrollbars

While I understand that Windows 11 also defaults to auto-hiding scrollbars, I suspect that for the majority of Windows users on Windows 10, this will be quite a shift. Mainly what bugs me about this is that it makes the icons no longer align: if you visually track a vertical line from the top toolbar ellipsis icon and downwards, things don't line up.

While it does come down to choosing a tradeoff, I wonder if there are some more modern scrollbar CSS properties we can explore first, in a progressive-enhancing way. For example, there's a scrollbar-gutter property here that could be interesting. If we combine that with a supports, we could potentially adjust paddings and margins when the feature is supported, so as to ensure things still line up when the gutter is reserved. Something like:

@supports (scrollbar-gutter: stable) {
	scrollbar-gutter: stable;
}

The site editor is additionall exploring a scrollbar behavior that's codepenned out here: https://codepen.io/joen/pen/rNKRWdx?editors=1100 — I wonder if there's something we could leverage just for the inspector. What do you think?

@corentin-gautier
Copy link
Contributor Author

corentin-gautier commented Jan 27, 2023

@jasmussen Here's an exemple with :

  • .interface-interface-skeleton__content with scrollbar-gutter stable, overflow auto
  • .interface-interface-skeleton__sidebar with overflow scroll
  • .interface-interface-skeleton__header with scrollbar-gutter stable, overflow auto to match the sidebar icons alignement

Screenshot 2023-01-27 at 14 03 04

Screenshot 2023-01-27 at 14 00 27

This would only happen when the user explicitly chooses to always show the scrollbars

Screen.Recording.2023-01-27.at.14.06.33.mov

@jasmussen
Copy link
Contributor

Thanks for your responsiveness and demo videos, very helpful in reviewing 🙏

To be clear: some of this we've been through in the past and ended up back on what's shipping in trunk. So wherever we go, it would be good to boost the confidence. CC: @WordPress/gutenberg-design

Personally it does bug me that there's a jump when I have scrollbars showing always. But to be clear, this bugs me only in the inspector, not in the editing canvas. So I think to keep things focused, we should probably not touch the editor canvas at this point and focus only on the inspector, and that's where I think the scrollbar gutter could be interesting.

@jameskoster
Copy link
Contributor

I think it would be nice to eliminate the jump, and this seems like a low-risk tweak that is easy to revert. 👍 from me!

@paaljoachim
Copy link
Contributor

I too would like to test this out! I agree with James.

@corentin-gautier corentin-gautier force-pushed the fix/avoid-ui-shift branch 2 times, most recently from ac23b3e to 4f67e47 Compare January 31, 2023 12:44
@corentin-gautier
Copy link
Contributor Author

corentin-gautier commented Feb 6, 2023

Thanks for your responsiveness and demo videos, very helpful in reviewing 🙏

To be clear: some of this we've been through in the past and ended up back on what's shipping in trunk. So wherever we go, it would be good to boost the confidence. CC: @WordPress/gutenberg-design

Personally it does bug me that there's a jump when I have scrollbars showing always. But to be clear, this bugs me only in the inspector, not in the editing canvas. So I think to keep things focused, we should probably not touch the editor canvas at this point and focus only on the inspector, and that's where I think the scrollbar gutter could be interesting.

This PR only changes the behaviour of the post editor 👍
The current behaviour in trunk is indeed not ideal at all for the amount of style recalculation it triggers as well as the visual shifting of the UI

@jasmussen
Copy link
Contributor

This PR only changes the behaviour of the post editor 👍

Just to be sure I understand you correctly, this changes the scrollbar behavior both for the editor canvas and the block inspector, in the post editor, correct?

If yes, then I'd still split that into two PRs, one PR for just the inspector, one PR for just the editor canvas. The former can land quickly, the latter actually needs a little thought, because it can be argued that the editor canvas should be WYSIWYG, including the behavior of the scrollbar. That is, a particular theme might have a curious scrollbar behavior on the frontend, and this should probably be reflected in the editor as well. An extreme example that comes to mind is an older theme called "Shelf", which scrolled horizontally.

@corentin-gautier corentin-gautier force-pushed the fix/avoid-ui-shift branch 2 times, most recently from 791ae0a to 7d7e1c9 Compare February 13, 2023 09:59
@corentin-gautier
Copy link
Contributor Author

corentin-gautier commented Feb 13, 2023

@jasmussen I reverted the change on the editor canvas so this PR only changes the behaviour of the sidebar. Still It would be nice to quickly land the change on the editor canvas since it avoids a lot of browser repaint and it's a very small change

@jasmussen
Copy link
Contributor

Thank you! Let's get some reviews on this. @WordPress/gutenberg-design

@paaljoachim
Copy link
Contributor

Testing.
Mac OSX 12.6.3
Using WordPress 6.2 beta 2.
WordPress Beta Tester and Yoast SEO plugins are active.
Browser: Brave.
NB. I downloaded to test the PR Gutenberg build but wanted to pretest first. This is the pretest.

I have been exploring and it seems sometimes the shift occurs and sometimes not. I made sure to make the page very small so that I could see the page scrollbar and the sidebar scrollbar.

1- A shift occurs in the beginning when opening panels and then suddenly the shift stops.

In Brave.
https://user-images.githubusercontent.com/5323259/220359385-d7e1c29f-d941-4d8b-98e4-864620d0f4d7.mp4

In Safari.

No.shift.in.Safari.mp4

What am I missing? Is this problem in Windows but not on the Mac?

@corentin-gautier
Copy link
Contributor Author

@paaljoachim in the second half of the brave video the sidebar content is high enough to always overflow (you left some panels open), that's why you're not getting a shifting UI anymore (the scrollbar doesn't hide), in the safari video the content is always overflowing because the content it too big so the scrollbar is always needed as well. The problem only appears when you don't have overflow because the sidebar content is too small to trigger overflow and then opening a panel triggers it.

@jasmussen
Copy link
Contributor

Thanks for all the energy. I took it for a spin and have a lot of GIFs and screenshots to go through.

First off, here's how it looks on a Mac if you have auto hiding scrollbars. This GIF shows both the before and after, because they are the same:

post editor, auto hiding scrollbars, before and after (they are the same)

One note I realized as I was testing this is that we would probably need the scrollbar code to be present also in the site editor. Currently it's just the default overflow behavior:
Screenshot 2023-02-28 at 13 20 31

Screenshot 2023-02-28 at 13 20 15

Then I proceeded to test how this new behavior exists with always visible scrollbars. Here's a screenshot:

always-visible-scrollbars

Note in the above screenshot how in the top right corner, the top toolbar is suddenly vertically scrolling. This appears to be a bug worth looking into. Latest mac/chrome.

Here's a GIF showing how the scrollbar-gutter reserves space for the scrollbar, even when there's no content to scroll:

post editor, visible scrollbars, after

It's easiest to see the behavior when you compare with trunk. Here's how it looks today, notice how the scrollbar pushes the inspector leftwards when scrollable content appears:

post editor, visible scrollbars, before

So that's the main review. But as I tested this, I realized that for people who have always visible scrollbars, this extra space that's reserved for the scrollbar will essentially be misaligning the layout, optimizing for long inspectors. Note here how the top toolbar ellipsis menu, and the X icon are misaligned:

misaligned icons

This isn't ideal for most cases, where we are actively cleaning up and simplifying inspectors to be shorter and simpler. So this opens a few questions:

  • Is this fine? After all, if you have visible scrollbars, it will be a less jarring experience when inspectors grow long.
  • Or, are we optimizing for long inspectors when we should be optimizing for short ones?
  • Can we enable this behavior only for smaller screens, such as @media only screen and ( max-height: 900px )?

Alternately, are there other progressively enhancing CSS properties we can use to make sure the inspector doesn't move leftward, but things still line up? It appears that overflow: overlay; is an unofficial extension. But another option is to look into a custom scrollbar for the inspector. With such a one as shown, we would in most cases be able to control the width of the scrollbar, making sure things line up even when space is reserved. The one linked also has a faux show/hide behavior, which could be interesting. But that part might not even be needed for the inspector.

Thanks for all your energy on this one. What do you think?

@corentin-gautier
Copy link
Contributor Author

@jasmussen Thanks for your feedback

as you can see in the video here #47177 (comment) their shouldn't be misaligned icons and the top toolbar shouldn't have a scrollbar, so I'm not sure what's happening (I'm also using chrome on a mac)

Here's how it look in my current test env :

Screenshot 2023-03-09 at 13 22 11

Screenshot 2023-03-09 at 13 22 20

@jasmussen
Copy link
Contributor

Took this for another fresh spin. Before, or with auto hiding scrollbars:

Screenshot 2023-03-10 at 08 01 11

After, with always visible scrollbars:
Screenshot 2023-03-10 at 08 01 49

As you can see, I'm still seeing the scrollbar in the header. This appears to be the responsinble code:

Screenshot 2023-03-10 at 08 02 08

If you change that overflow to hidden, it looks right. With scrollbar in the inspector:

Screenshot 2023-03-10 at 08 02 14

With no content to scroll, but space still reserved for the scrollbar:

Screenshot 2023-03-10 at 08 02 23

The scrollbar-gutter: stable as applied to the header toolbar is a neat trick, as it invisibly reserves space there to the right, as you note, making the icons line up.

Furthermore, if you use the auto-hiding macos scrollbars, in both places the reserved space collapses and looks as it does today:

Screenshot 2023-03-10 at 08 04 17

To me, this threads the needle!

  • When you have always visible scrollbars, space is reserved so you avoid UI shifts.
  • Icons still line up.
  • When you have auto hiding scrollbars, reserved space is collapsed, ensuring icons line up and things look as before.

I still think we should eventually look into something like https://codepen.io/joen/pen/rNKRWdx?editors=1100 as a customized scrollbar for compact spaces, but in the mean time, this feels like a good step forward, with minimal downsides. It's also a PR I think @ellatrix will appreciate.

So as next steps, can you please apply overflow: hidden; to the header container as mentioned, and then let's ask @WordPress/gutenberg-design for eyes again. Thank you!

Force the use off overflow scroll on scrollable areas to avoid UI shifting/resizing when getting a scrollbar
@corentin-gautier
Copy link
Contributor Author

@jasmussen done ;)

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try this.

Can you add a small CSS comment in context of the new values, to explain what they do? Something like

// The scrollbar-gutter property ensures space is reserved for the scrollbar to appear,
// when scrollbars are set to be always visible.

@corentin-gautier
Copy link
Contributor Author

@jasmussen comment added ;)

@jasmussen
Copy link
Contributor

Are you able to press the green button and merge? Or do you need me to do that?

@corentin-gautier
Copy link
Contributor Author

@jasmussen I do not have that kind of power :)

@ciampo ciampo merged commit a4c1edc into WordPress:trunk Mar 21, 2023
@ciampo
Copy link
Contributor

ciampo commented Mar 21, 2023

Merged for you :) Thank you again @corentin-gautier for your help and patience in tweaking your proposed solution!

@github-actions github-actions bot added this to the Gutenberg 15.5 milestone Mar 21, 2023
@tyxla
Copy link
Member

tyxla commented Mar 21, 2023

Nice one, thanks @corentin-gautier 🙌

@StevenDufresne
Copy link
Contributor

Noting that a wp.org plugin added a dropdown menu to the header and since the overflow is hidden that component doesn't work anymore. Not certain if this is a common use case but hiding any overflow in the .edit-post-header may not be a great strategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Edit Post /packages/edit-post [Package] Interface /packages/interface [Type] Performance Related to performance efforts
Projects
None yet
7 participants