Top Toolbar: Prevent the focus outline of the button from being cut off #54172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Package] Block editor
t-hamano
commented
Sep 5, 2023
| ? parseFloat( computedPinnedItemsStyle.width ) + 10 // 10 is the pinned items padding | ||
| ? parseFloat( computedPinnedItemsStyle.width ) |
There was a problem hiding this comment.
Padding is included in the width value, so it shouldn't be necessary in the first place.
t-hamano
force-pushed
the
fix/fixed-toolbar-cutoff
branch
from
f377768
to
d0c20a7
Compare
|
Size Change: -46 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
draganescu
reviewed
Sep 5, 2023
| overflow: hidden; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't understand the connection between the two changes. If the 10 px extra width solves the focus ring issue why do we need to remove this tweak? As far as I know this was added to prevent thr ugly jumping around when scrollbars appear.
There was a problem hiding this comment.
The scrollbars in the sidebar are defined elsewhere and are not affected by removing this CSS. This CSS affects the header. The following screencast might help you understand why this style must be removed.
We could take this scrollbar width into account when calculating the width of the fixed toolbar, but this is a bit tricky. See ChatGPT's answer for more details 😅: https://chat.openai.com/share/08933117-0064-4de0-ae71-747550c00c55
69cfa5fbf4f583a9a3b06352be25178b.mp4
draganescu
approved these changes
Sep 5, 2023
|
Thanks for the review, @draganescu! This problem is probably unique to WIndows, where the scrollbar has a physical width. I believe we can safely merge this PR, but if there are other issues I would be happy to address them in a follow-up! |
mikachan
added
[Type] Bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR prevents the focus outline of the device preview button from cutting off when the top toolbar is enabled.
Site Editor
Before
After
Post Editor
Before
After
Why?
Site Editor
The fixed toolbar dynamically has its own width, minus the total width of the other UI in the header, margins, etc. However, when the first button (device preview button) in the right header is focused, the outline has a width of 2px, so it is hidden under the top toolbar.
Post Editor
The Post Editor is a little more complicated. In #47177, the sidebar now always has the scrollbar to prevent sidebar UI shifting. At the same time,
scrollbar-gutter: stable;was added to align the header icons with the sidebar. This will push the right-hand header to the left by the width of the scrollbar. As a result, even if the top toolbar has the correct width, it will overlap by the width of this scrollbar.How?
Site Editor
When calculating the fixed toolbar width, I took the outline into account and subtracted 2px.
Post Editor
Removed
scrollbar-gutter:stablefrom the right header. This causes problems in Windows OS and Mac OS (where scrollbars are always set to appear), where the scrollbar has a physical width that prevents the header and sidebar icons from aligning.However, this approach also has the effect of removing unnecessary spacing on the right side of the header when the sidebar is not displayed.
So far I haven't found an approach that would ideally solve everything, so if anyone has any ideas, I'd love to hear them.
Testing Instructions
Note: I've noticed an issue with the widget editor where the top toolbar obscures the header on the right. I would like to address this issue in another PR.