-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Top toolbar: fix half a pixel artifacting of the bottom border. #62225
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. |
Size Change: +16 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
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.
This issue probably depends on the computer and display, but on my setup the bottom border was completely hidden.
Perhaps it would be more reasonable to subtract 1px from this value?
height: $header-height; |
Thanks for the suggestion. I tried it, and it looked as if it worked so I committed it. But on closer inspection, that causes the buttons to be offset by a little bit: Still closer investigation surfaced that this is only an issue in distraction free mode, where the toolbar is positioned and animated using translate. This particular aspect makes it subject to subpixel rounding math, which is probably what's causing this glitch in the first place. Another option is removing the white background, what do you think? For now, I've restored the previous fix, which I don't love, but does make things still line up: |
This approach also seems reasonable. By adding gutenberg/packages/editor/src/components/collapsible-block-toolbar/style.scss Lines 8 to 9 in da98bdb
I'm sure @jeryj know a lot about toolbars, so if you have any good ideas please let me know 🙏 |
If |
That fix seems to work well! The only question remaining, is probably one for @draganescu and related to how 3rd party plugins in some cases inject buttons there, such as custom pattern insterter tools. I vaguelly recall us adding some features to the top toolbar mode that covered those buttons unless manually collapsed. Would that regress if we remove the background color? Anyone recall a plugin that still does this, or was the behavior for how those buttons are forcefully added changed? |
@@ -10,6 +10,7 @@ | |||
|
|||
&.is-unstyled { | |||
border: none; | |||
background: transparent; |
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.
The white background doesn't originally exist in the Toolbar
component itself. It exists in the BlockToolbar
component, a block that extends the Toolbar
component.
background-color: $white; |
And I think the white background only needs to be overridden when this block toolbar is rendered in the header, so I think adding it in the place suggested in this comment might be a good idea.
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.
Thank you for your patience. Moved it there, and it still seems to work for me.
c4441e8
to
e5885cd
Compare
e5885cd
to
fb1e226
Compare
Flaky tests detected in fb1e226. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9366246617
|
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.
LGTM!
Nit: Since this PR did not ultimately change anything in the components
package, the CHANGELOG entry can be removed.
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.
Tested this as well and the borders look perfect!
The original implementation of the top toolbar was a |
…dd/on-async-directives * 'trunk' of https://github.com/WordPress/gutenberg: (72 commits) Top toolbar: fix half a pixel artifacting of the bottom border. (#62225) Fix UI order for theme.json spacing sizes (#62199) Chore: Simplify a padding style on global styles. (#62291) Editor: Avoid remounts of `DocumentBar` (#62214) Add `default-spacing-sizes` and `default-font-sizes` options for classic themes (#62252) Editor: Cleanup styles and classnames (#62237) Scripts: Pin the @wordpress/scripts version to a version supported by WP 6.5 (#62234) Documentation: Better changelogs for the JSX transform upgrade (#62265) Chore: Simplify a padding style on dataviews. (#62276) MediaUpload: Remove dialog markup on close (#62168) Tabs: Prevent accidental overflow in indicator (#61979) Make edit site pagination buttons accessibly disabled. (#62267) Fix: Remove unused code from dataviews styles. (#62275) Re-enable React StrictMode (#61943) Inserter: Return the same items when the state and parameters don't change (#62263) Update instances of text-wrap: pretty to fall back to balance (#62233) Update: Slotfill documentation samples (links, code, and rephrase). (#62271) Try: Fix mover positioning. (#62226) [Mobile] - Image corrector - Check the path extension is a valid one (#62190) Update: Block styles documentation. ...
Thanks for the merge and input + reviews. |
* Top toolbar: fix half a pixel artifacting of the bottom border. * Address feedback. * Restore previous fix after all. * Use transparent background color. * Update changelog. * Move the rule to the right place. * Remove entry from changelog. Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org>
* Top toolbar: fix half a pixel artifacting of the bottom border. * Address feedback. * Restore previous fix after all. * Use transparent background color. * Update changelog. * Move the rule to the right place. * Remove entry from changelog. Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org>
…Press#62225) * Top toolbar: fix half a pixel artifacting of the bottom border. * Address feedback. * Restore previous fix after all. * Use transparent background color. * Update changelog. * Move the rule to the right place. * Remove entry from changelog. Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org>
This was cherry-picked to the wp/6.6 branch. |
What?
In distraction free mode, or with the top toolbar enabled, there's a half pixel white artifact below the toolbar which causes the bottom border to not be coherent:
![broken bottom border](https://private-user-images.githubusercontent.com/1204802/336036042-200afcb7-3952-4a9c-889d-c34d7f1ffb84.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjUyMjQxMTYsIm5iZiI6MTcyNTIyMzgxNiwicGF0aCI6Ii8xMjA0ODAyLzMzNjAzNjA0Mi0yMDBhZmNiNy0zOTUyLTRhOWMtODg5ZC1jMzRkN2YxZmZiODQucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDFUMjA1MDE2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9OTYxY2U2NGQ4ZGU1NjMxNDY4NzRlODAxNDgzZjI2NDUyYWQ1MGY1MzA4ZmNjY2IzMGY3YTUxM2MyMWU4ZjUwZiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.rzjgVWeu8tyZK6S3KrMGz97poHk-76S6MtkSdxpquro)
This PR fixes that by applying a max height, with no discernible side effects: