fix change focus to focus-visible in navigation block css
#60429
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
fabiankaegy
added
[Type] Enhancement
fabiankaegy
requested review from
Mamaduka,
jasmussen,
afercia,
joedolson,
alexstine and
aaronrobertshaw
|
Size Change: +10 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
afercia
reviewed
Apr 4, 2024
There was a problem hiding this comment.
@fabiankaegy thanks for your ping to ask for a review.
I'm not sure we should change this so lightly.
- I'd think this would need a broader consideration as in: either WordPress should always uss
focus-visibleon the front end or it should not. I'm not sure using it only for the navigation overlay initial focus would be ideal. Instead, it should be consistent for all focus styles. - Personally, I don't like
focus-visiblethat much because it makes an assumption and then delegates to browser internal heuristics to determine when a pointing device or other devices are in use. The assumption is that users are either using the mouse or the keyboard. That's often a wrong assumption, as many useers do use a mix of devices. Personally, I use both mouse and keyboard often during the same flow.
Anyways, this change will not work for all themes. For xample, Twenty Twenty-Four sets a global style from its theme.json that has a higher specificity so that hte focus style is visible also when using the mouse:
:where(.wp-site-blocks *:focus){outline-width:2px;outline-style:solid}
I'd like to see this change be considered in a broader discussion. Could you please createa GitHub issue to allow for a broader discussion?
|
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. |
|
To clarify the intent of this change, it would make the focus style of the first item in the menu overlay (when a navigation block is set to use an overlay) not visible when using the mouse:
and still visible when using the keyboard:
To test this with Twenty Twenty-Four, the custom style set by the theme in Styles > Additional CSS needs to be removed: clear the textarea and save. Cc @joedolson |
|
@afercia Thanks for the thorough review :) From my perspective, I think I only partially agree with the fact that this should be a more holistic topic we discuss. To me there is a massive difference between a theme making a visual choice for how it wants to style its content vs. how the core navigation block styles its focus states by default. So I don't think making this change here wouldn't have to mean anything for the tt4 theme or any other theme in the theme directory. To me this is an area where this PR attempts to make the default behavior of the core block less opinionated to leave it up to themes how they want to style it. |
There was a problem hiding this comment.
Thanks for proposing this enhancement @fabiankaegy 👍
To me this is an area where this PR attempts to make the default behavior of the core block less opinionated to leave it up to themes how they want to style it.
I like the intent of this PR but I do think we might need to take a moment as @afercia suggests to consider a few things more broadly.
As part of the consistency angle to this, theme.json only supports the :focus pseudo selector and not :focus-visible. To me, it would make sense that our default styles and those supported via theme.json are consistent.
|
@aaronrobertshaw that is a very good point I hadn't considered :) makes sense to me 👍 |
@fabiankaegy Yes, personally I'm a little concerned about it as well. I'm really not sure the editor should provide its own custom focus styles for the front end. From an accessibility perspective, when it comes to focus style the best option is to just do nothing and let browsers do their job. I know contributors to this project have different opinions about what is responsibility of the editor and what is responsibility of themes. However, what I would really like to avoid is that the editor ends up using I would also like to remind everyone that the contributing guidelines of this project ask to create an issue for any non-trivial change to allow for broader discussion. That is not different from the process WordPress alwayse used on Trac. For a collaborative open source project, broader discussion is of fundamental importance. Alas, in this project I've seen many, too many, pull requests without an associated issue.
|
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?
Changes the styling of the Navigation block to use the
focus-visibleselector instead of thefocusselectorWhy?
When the navigation overlay opens the first item receives focus. As it should. However it is confusing to users why the element looks different than all the other elements.
focus-visibleis meant for this exact purpose. To style elements when they receive focus through the keyboard etc.How?
changing the instances of
focustofocus-visibleTesting Instructions
Ensure the navigation block still has proper focus states
Testing Instructions for Keyboard
Screenshots or screencast