Block mover button: do not show focus styles on pointer interactions #45126
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
ciampo
added
[Type] Enhancement
|
|
|
Size Change: +8 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
try/block-mover-button-focus-styles
branch
from
ad127b7 to
6b9bf2a
Compare
ciampo
requested review from
talldan,
mirka,
mtias,
jasmussen,
youknowriad and
chad1008
This PR seems to apply to all toolbar button styles, which does make things more consistent. Is that on purpose? |
|
What's the guideline for the new focus-visible pseudo selectors? Should we use them consistently for all our focus styles? Also, it's actually already supported in all the browsers we do support, so technically we could avoid the "supports" fallback or you're just leaning on the safe side here. |
Great catch, I had actually meant to update the title / description but clearly forgot.
It is for now, but it definitely needs approval from folks like @mtias / @jasmussen — it's hard to draw the line of where the new focus styles should apply: just the block movers? All buttons in the toolbar? What about the dropdown menus originating from the toolbar? And what about the list view, and the remaining buttons in the list view? The more we spread out these styles, the more consistent the UI looks — but that means introducing even more style overrides and more technical debt. |
I'd love to, but that would mean to refactor the I think that the spirit of Matias' request was to just apply this change to the block movers.
From what I know, se support the latest 2 version of evergreen browsers — support for |
ciampo
force-pushed
the
try/block-mover-button-focus-styles
branch
from
6b9bf2a to
41f76b2
Compare
With this in mind, I've pushed a change that apply the new |
|
@ciampo I think the last version of Safari is Safari 16 but it probably depends on the OS. |
I think I know where the misunderstanding lies — I was interpreting it the word "version" as "major version", and therefore I thought that we had to support Safari 15 and 16 😅 But looking at browserlist's website, the current "last 2 versions" for Safari are
and even checking for "> 1%" usage doesn't show any Safari version before
Which means that I can safely remove the fallback. Sorry for the confusion! |
mirka
reviewed
Oct 20, 2022
Comment on lines
70
to
75
| &:not(.block-editor-block-mover-button):focus::before { | ||
| @include block-toolbar-button-style__focus(); | ||
| } | ||
|
|
||
| // Focus style for block mover buttons (using :focus-visible instead of :focus) | ||
| &.block-editor-block-mover-button:focus-visible::before { |
There was a problem hiding this comment.
Do you prefer that these hacks live here instead of packages/block-editor/src/components/block-mover/style.scss?
I think I prefer moving it to block-mover unless it's likely that we're defaulting to focus-visible for the rest of the toolbar buttons soon. @wordpress/components shouldn't contain downstream overrides, even if that means dirtier code downstream (e.g. additional specificity hacks).
There was a problem hiding this comment.
I attempted to move styles to packages/block-editor/src/components/block-mover/style.scss as you suggested:
- I still had to leave those same styles in place in
packages/components/src/toolbar/style.scssbecause they are applied to the rest of toolbar button (using:focus) - I had to add an override in the block mover button styles to remove the
:focusstyles applied by the block toolbar - I had to leave some minor overrides for the size/position of the
::beforepseudo element in the list view
6c17fae — WDYT ?
There was a problem hiding this comment.
Whoa, talk about leaving things cleaner than you found them 👏 Great refactor!
ciampo
commented
Oct 20, 2022
| @@ -134,7 +134,7 @@ function ListViewBlock( { | |||
| const { isTreeGridMounted, expand, collapse } = useListViewContext(); | |||
|
|
|||
| const hasSiblings = siblingBlockCount > 0; | |||
| const hasRenderedMovers = showBlockMovers && hasSiblings; | |||
| const hasRenderedMovers = true && hasSiblings; | |||
There was a problem hiding this comment.
I didn't intend to commit this line, but we can keep it for now to make it easier to review the block mover button in the list view. I will remove this line before merging
mirka
approved these changes
Oct 21, 2022
ciampo
force-pushed
the
try/block-mover-button-focus-styles
branch
from
6c17fae to
98d8a5b
Compare
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?
Tweak mover button styles in both the block toolbar and the list view, so that the button's focus styles are not shown as the results of a user click.
Why?
More polished button styles when interacting with a pointer.
How?
By using the
:focus-visibleCSS pseudoselector, which lets the browser determine via heuristics if the focus should be made evident on the element — in practice, it allows for focus styles to be shown only on keyboard interaction (and not as a result of pointer clicks).packages/block-editor/src/components/block-mover/style.scsspackages/components/src/toolbar/style.scssbecause they are applied to the rest of toolbar button (using:focusinstead of:focus-visible):focusstyles applied by the block toolbar::beforepseudo element in the list viewNote: the correct way of making this change would be by changing styles directly in the
Buttoncomponent. This PR instead resorts to applying style overrides directly in the block editor — this is a sub-optimal approach, which introduces technical debt and fragmentation, making it actually harder to work on theButtoncomponent in the future. This approach is therefore not reccommended.Testing Instructions
Block mover buttons in the toolbar:
Block mover buttons in the tree list:
hasRenderedMoversvariable totrue)Screenshots or screencast
Before (
trunk): focus ring appears on both pointer and keyboard interactionsBlock toolbar:
block-movers-trunk-block-toolbar.mp4
List view:
block-movers-trunk-block-list-view.mp4
After (this PR): focus ring only shows on keyboard interactions
Block toolbar:
block-movers-PR-block-toolbar.mp4
List view:
block-movers-PR-block-list-view.mp4