Restore visual separator between mover buttons when show button label is on #57640
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
afercia
added
[Type] Bug
|
Size Change: +1.58 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Jan 8, 2024
There was a problem hiding this comment.
Thank you for the follow-up! I think this change makes sense, but since the .block-editor-block-mover__move-button-container selector already exists, how about changing it like this:
diff --git a/packages/block-editor/src/components/block-toolbar/style.scss b/packages/block-editor/src/components/block-toolbar/style.scss
index ae460faf5b..832988b416 100644
--- a/packages/block-editor/src/components/block-toolbar/style.scss
+++ b/packages/block-editor/src/components/block-toolbar/style.scss
@@ -203,6 +203,24 @@
.block-editor-block-mover .block-editor-block-mover__move-button-container {
width: auto;
+
+ @include break-small() {
+ position: relative;
+
+ &::before {
+ content: "";
+ display: block;
+ height: $border-width;
+ width: 100%;
+ background: $gray-900;
+ position: absolute;
+ top: 50%;
+ left: 50%;
+ // With Top toolbar on, this separator has a smaller width. Translating both
+ // X and Y axis allows to make the separator alwasy centered regardless of its width.
+ transform: translate(-50%);
+ }
+ }
}
.block-editor-block-mover.is-horizontal {
@@ -231,26 +249,6 @@
padding-right: $grid-unit-15;
}
- @include break-small() {
- .block-editor-block-mover__move-button-container {
- position: relative;
-
- &::before {
- content: "";
- display: block;
- height: $border-width;
- width: 100%;
- background: $gray-900;
- position: absolute;
- top: 50%;
- left: 50%;
- // With Top toolbar on, this separator has a smaller width. Translating both
- // X and Y axis allows to make the separator alwasy centered regardless of its width.
- transform: translate(-50%);
- }
- }
- }
-
.block-editor-block-contextual-toolbar .block-editor-block-mover.is-horizontal .block-editor-block-mover-button.blo
ck-editor-block-mover-button {
width: auto;
}
|
@t-hamano makes totally sense. Thanks for your review 🙏 |
t-hamano
reviewed
Jan 10, 2024
| @@ -145,14 +145,24 @@ $header-toolbar-min-width: 335px; | |||
| .block-editor-block-mover { | |||
| border-left: none; | |||
|
|
|||
| // Modified group borders | |||
There was a problem hiding this comment.
This comment doesn't seem to be related to the actual pseudo-element style, so how about deleting this comment?
| left: 50%; | ||
| // With Top toolbar on, this separator has a smaller width. Translating both | ||
| // X and Y axis allows to make the separator alwasy centered regardless of its width. | ||
| transform: translate(-50%); |
There was a problem hiding this comment.
Suggested change
| transform: translate(-50%); | |
| transform: translate(-50%, 0); | |
| margin-top: -$border-width * 0.5; |
I think translate(-50%); means translate(-50%, 0);. Strictly speaking, it is not vertically centered.
I tried changing it to translate(-50%, -50%) to align it vertically, but the border seems to bleed.
Instead, I tried using a negative margin to shift the border up by half the height value. This approach draws a solid border in my environment. Could you test if this approach works in your environment?
There was a problem hiding this comment.
I think translate(-50%); means translate(-50%, 0);. Strictly speaking, it is not vertically centered.
Right.
the border seems to bleed.
Are you on Windows or some GNU-Linux distro? What browser are you using? What display type and resolution?
I tried using a negative margin to shift the border up by half the height value. This approach draws a solid border in my environment.
The root problem is that the toolbar height is 48px. The mover buttons height is 24px. A vertically centered 1 pixel horizontal line would need to be placed at exactly 23.5 pixels. However, a physical pixel can't be split in two. Some operating systems / browsers / displays (retina resolution, sub-pixel rendering) may render that more nicely than others. The 'bleeding' depends on the ability to position an element to 'half a pixel'. In environments where a CSS pixel is actually more than one physical pixel that may work better. In your screenshot, the horizontal line is actually 2 pixels, apparently that's because your environment isn't able to render sub-pixels.
The top margin approach actually computes to margin-top: -0.5px;. In your environment it may preserve the height of the line to 1 pixel but I guess the line isn't vertically centered.
Environments that support true subpixel rendering won't trigger this issue. The only reliable way to avoid a greyish blurred line or its rendered height to be altered would be to make sure it is moved to a 'full pixel' e.g. by making the toolbar height an odd number.
Maybe translateZ(0) may help prevent the blurriness because it promotes the element to its own layer but there's no guarantee the line will be vertically centered. Also margin-top: -0.5px; doesn't guarantee the line will be vertically centered but I'd think that's batter than a blurred line.
For the records, this is how the translateY approach looks in my environment (zoomed in): no blurring or doubled height:
afercia
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
|
Flaky tests detected in b8af412. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7489952909
|
t-hamano
approved these changes
Jan 12, 2024
There was a problem hiding this comment.
LGTM!
the border seems to bleed.
Are you on Windows or some GNU-Linux distro? What browser are you using? What display type and resolution?
I am checking with Chrome browser on Windows OS. I have an external display connected to my laptop, and both have 1920 x 1080 resolution. I wasn't sure if these displays support sub-pixel rendering.
The top margin approach actually computes to
margin-top: -0.5px;. In your environment it may preserve the height of the line to 1 pixel but I guess the line isn't vertically centered.
So should we remove margin-top: -0.5px before merging this PR?
I think we should keep it. It works on environments that support true subpixel rendering. |
This was referenced Feb 14, 2024
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.
Follow-up to #49556
Part of an effort to remove all the CSS order properties from the codebase.
What?
While inspecting the CSS order properties, I noticed #49556 removed the visual separator between the Move Up and Move Down block toolbar buttons when the 'Show button text labels' preference is enabled.
From an usability and accessibility perspective thatn's nod ideal as there is no visual separation between the buttons. It's not clear where the buttons boundary is and they just look like a single button with some multi line text within it, much like the other buttons in the toolbar.
Screenshot:
Why?
There must be a clear separation between the two buttons.
How?
orderproperties.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Bock toolbar, zoomed in:
Top toolbar: