Apply custom scroll style to fixed header block toolbar #57444
Conversation
t-hamano
added
[Type] Enhancement
| // Prevents padding from being applied to the left and right sides of the element. | ||
| scrollbar-gutter: auto; |
There was a problem hiding this comment.
The custom scrollbar mixin has a style called scrollbar-gutter: stable both-edges;. To prevent the button from being cut off due to this, we need to override it.
|
Size Change: +901 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Thank you for the ping, thank you for the PR! Let's definitely land a version of this; the use case described is valid, and it solves an issue present with the on-hover scrollbars where it isn't clear that there are more tools. That said, we'll need to fiddle with the margins and styles, so that the icons remain on a single row, rather than get pushed upwards, because right now this change affects not just Windows, but all browsers including Mac with on-hover scrollbars:
I wonder, while we're here, if we can even apply this overflow with a max-width, so that it also scrolls horizontally when the block toolbar is docked near the block. If you insert a list item, and apply text labels only, it's trivial to get the toolbar cut off:
I've added a red border here as I'm debugging some things. I'll dive in now and see if we can have some tricks that make the icons line up still, while keeping this scrollbar. |
|
An observation as I stumble upon it, looks like the post/page header-bar is now 64px tall, whereas the site editor header is 60px tall (as defined by $header-height). We actually want the header to eventually be 64px tall, as part of some updates we are making to the metrics, but at the moment the discrepancy seems a bug. CC: @youknowriad, I wonder if it has to do with some of the unification work? Edit: actually that's wrong. The web inspector measures 64px, so there's definitely something funky going on with elements not being quite the right size, but the rendered height is still 61px including the 1px border. |
|
Okay I think I have the start of a fix. I haven't pushed it yet, because I don't have immediately on hand an easy way to test in Windows, so I would appreciate you testing first. There's also a side effect I'll come back to. Here's the updated rule, starting at line 76: What it does is provide a fixed height to the toolbar section, which means the icons, now positioned with a top-padding rather than vertical centering, push the scrollbar below the row of icons, letting the inner scrollbar technically overlap the buttons a little bit. I combined that with having the resting state of the scrollbar be a light gray that goes darker on hover. Here's how it looks with debug colors:
Here's what it looks like with the default UI colors:
Depending on how this works in Windows, we will need to do a little followup. Because right now that fixed height, if not the padding, also affects the contextual block toolbar:
While we could distinguish between top and contextual toolbars in the CSS, it might be good to find a solution that works across both, because as noted there are cases, very commonly with text labels only mode, where the toolbar currently gets cropped and therefore unusable. So it'd be nice if we could a solution that works across both. Let me know what you think! |
|
Thank you for your great exploration, @jasmussen!
I think this is definitely an issue that should be improved. I have encountered many cases where a large number of plugins extend the block toolbar and hide it even when not in text label mode. This is a simple example to demonstrate it.
The challenge here is that we need logic to detect if the floating block toolbar hits the edge of the browser or the block sidebar and automatically stretches or shrinks its width. This is probably related to the Popover component, and may not be solved with CSS alone. Therefore, it might be better to leave this as a future improvement plan for now.
Indeed. First of all, I would like to apply the approach you suggested in this comment and report the test results again. |
This approach seems to work for Windows as well. However, it appears that some adjustments are needed in the non-fixed or mobile toolbars. 1e24792b47740641e0d5576c795257c8.mp4I think this PR needs further exploration, so I'll return it to the draft. I'll try different approaches, but if you have any ideas, feel free to push them. |
Right, good point, that makes me think it's probably best to look at separately.
Good catch.
My instincts would go towards applying a fixed height also to mobile, and even to contextual block toolbars. In the case of the mobile toolbar, it probably just lacks a fixed height at the moment. In the case of the contextual toolbar, it is likely it needs a local fixed-height, that takes precedence over the taller height. I'll take a quick look now, see if there's a small fix I can push. Otherwise, I'm heading AFK for a little bit very soon, and can look again the second week of January. I'd like to take the opportunity to wish you a happy new year. Thank you for the energy and care you brought to the project in 2023, your impact is felt! |
|
Oh I may have found a fix. There's a wrapper that appears to be attached only to the top-toolbar-embedded version, and if we apply the height and padding scoped to just that wrapper, it appears to thread the needle:
CC: @jeryj as I believe he worked on this as well. |
| height: $grid-unit-60; | ||
| top: 0; | ||
| display: inline-flex; | ||
| align-items: center; |
There was a problem hiding this comment.
Since this dot is a font, its height may change depending on the font-family and line-height. So I explicitly set it to the same height as the button and aligned it vertically and centered.
|
Thank you for your wonderful efforts, @jasmussen! In order to ship this PR, we mail need to adjust the border height and the dot position. I made a little adjustment in BeforePost Editor:
Site Editor:
Widget Editor:
AfterPost Editor:
Site Editor:
Widget Editor:
|
|
Looks good to me! |
From what I've researched, this might be difficult. The toolbar (
Simply adding
|
|
I wonder, what if the buttons are abs-positioned? They might be covered by the scrollbar, but would they force a scroll? |
Does this mean applying
|
|
Yes indeed, good catch. That might need a min-width. For now I was mostly curious if that aspect actually fixed the issue, meaning that it would not cause shifting when focused, and would afford the scrollbar where it was. If yes, I think we could probably find a reasonable min-width for it. In general the text-labels only mode needs separate work, both in terms of rephrasing labels, and managing font sizes, there should be room to get that right. |
I tried this approach right away. That is, apply an explicit width and Unfortunately, even if the button is absolutely positioned, toolbar shifting seems to occur if the button overlaps the scrollbar. Perhaps the toolbar shift might occur when an element visually overlaps the scrollbar, regardless of whether the element is relative or absolutely positioned 🤔 82e44f91438c27831200d805283cb7c8.mp4 |
|
Forgive the delay, I finally had time to come back to this. I think I've found a way forward. The key was realizing that the 20+20 != 48.
Essentially, the
Here's the same in Firefox:
I took the liberty of pushing the fix, but it should be easy to revert if I made an error. Let me know how this works for you. |
|
Thank you for exploring! Unfortunately, it did not work properly with the combination of Windows OS and Chrome. In the screencast below, a red border is applied to the container ( 5cc0178fdbe39164bc059591c521fff9.mp4Personally, I would like to at least fix this unsophisticated scrollbar in Windows with WP6.5. I think the current destination is one of the following, what do you think?
In either case, we might be able to try this problem again after changing the header height to 64px in the future. |
|
What happens if the 20px container is even smaller, say 8px tall? That said, I don't think it's problematic to move it upwards 6px if that fixes it, especially if we can limit it to only the top toolbar mode. As you say, we can then revisit. |
If I change the height of a container with a height of 20px to 8px, the mover button will move up. In order to align this, I was able to increase the 91e0b57bf850efa01f3975c37fb7dc6d.mp4 |
|
If we're comfortable with the code complexity, and if it's limited to the toolbar in top toolbar mode for now, I think it's fine to shift the movers upwards to address the scrolling issue, and then merge this. What do you think? |
I think so too. I reverted your c2de530 and merged the latest trunk again. What this PR ultimately achieves should be the behavior described in this comment. By the way, I'm thinking of making the headers more consistent overall. The issues I found are summarized in #58293. If you have any opinions, please comment. |
|
I think it's okay to ship this PR, what do you think?
Yes, I checked that PR. However, it seems that some components have reverted to 36px in the latest trunk. Perhaps some subsequent change caused the regression. |
If this scrollbar code is limited to the toolbar in top toolbar mode, and not near the block, yes, I think it's a substantial improvement over trunk, which looks like this: That's barely usable given how much things are shifting around. This PR fixes that well, and does not preclude further improvements. Thank you for your work and attention to detail here.
Ack, well thank you for being aware! |
Yes, this PR should have no effect when not in Top Toolbar mode. If there are any issues, I would be happy to address them with a follow-up PR. This was a difficult problem, but thank you for all your ideas and advice! |
…zy-hydration * origin/trunk: (47 commits) Interactivity API: Break up long hydration task in interactivity init (#58227) Add supports.interactivity to Query block (#58316) Font Library: Make notices more consistent (#58180) Fix Global styles text settings bleeding into placeholder component (#58303) Fix the position and size of the Options menu, (#57515) DataViews: Fix safari grid row height issue (#58302) Try a fix (#58282) Navigation Submenu Block: Make block name affect list view (#58296) Apply custom scroll style to fixed header block toolbar (#57444) Add support for transform and letter spacing controls in Global Styles > Typography > Elements (#58142) DataViews: Fix table view cell wrapper and BlockPreviews (#58062) Workflows: Add 'Technical Prototype' to the type-related labels list (#58163) Block Editor: Optimize the 'useBlockDisplayTitle' hook (#58250) Remove noahtallen from .wp-env codeowners (#58283) Global styles revisions: fix is-selected rules from affecting other areas of the editor (#58228) Try: Disable text selection for post content placeholder block. (#58169) Remove `template-only` mode from editor and edit-post packages (#57700) Refactored download/upload logic to support font faces with multiple src assets (#58216) Components: Expand theming support in COLORS (#58097) Implementing new UX for invoking rich text Link UI (#57986) ...
MaggieCabrera
added
the
[Feature] Site Editor
What?
This PR applies the custom scrolling style to the fixed block toolbar. I think this improves the appearance, especially on Windows.
Note
This PR was originally to simply introduce a custom scrollbar. However, after some discussion, we have made a CSS adjustment in all editor instances to prevent the toolbar from shifting when a scrollbar appears or when a button is focused.
Before (Chrome)
After (Chrome)
Why?
In the Chrome browser on Windows, the scrollbar has a physical width/height. So the top toolbar buttons are pushed far up and look unnatural to me.
Additionally, this toolbar actually contains two
overflowproperties:gutenberg/packages/edit-site/src/components/header-edit-mode/style.scss
Lines 206 to 208 in 83e1a2c
gutenberg/packages/block-editor/src/components/block-toolbar/style.scss
Lines 76 to 77 in 83e1a2c
In the case of the Site Editor, the
overfowproperty value of the upper container (.selected-block-tools-wrapper) isscrollinstead ofhidden, so when element in the container overflow, the two scroll bars overlap, making it look even more unnatural.How?
First, I changed the
overflowproperty of the upper container (.selected-block-tools-wrapper) fromscrolltohidden. This will eliminate double scrollbars.Additionally, to make this thick scrollbar appear more natural, I applied a
custom-scrollbars-on-hovermixin to the inner scroll container (.block-editor-block-toolbar). If an element overflows, a scrollbar will appear without the need for mouseover. Personally, I feel that this is easier to understand than the behavior of disappearing the scroll bar when the mouse is out.Testing Instructions
Screenshots or screencast
Note: This shows scrollbar behavior in WindowsOS and Chrome.
Before
before.mp4
After
after.mp4