Item: Unify focus style and add default font styles #52495
Merged
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
changed the title
Item component rendered as anchor elementItem component rendered as anchor element
t-hamano
added
[Focus] Accessibility (a11y)
|
Size Change: -56 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
t-hamano
commented
Jul 11, 2023
| cursor: pointer; | ||
| background: none; | ||
| text-align: start; | ||
| text-decoration: ${ as === 'a' ? 'none' : undefined }; |
There was a problem hiding this comment.
Add a style that cancels the default underline only for the anchor element.
|
Flaky tests detected in 1194265. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5518976926
|
mirka
reviewed
Jul 12, 2023
Comment on lines
-30
to
-37
| &:focus { | ||
| box-shadow: none; | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
It looks like we still need to keep this reset, because the :focus state gets triggered (without being overridden by a :focus-visible state) when you click on the link:
There was a problem hiding this comment.
This focus style seems to be derived from WP-Admin's common.css.
I think it would be better to resolve this at the component level, so I will move this focus style to the component.
| outline: 2px solid transparent; | ||
| } | ||
| `; | ||
| export const unstyledButton = ( as: 'a' | 'button' ) => { |
There was a problem hiding this comment.
Should we also add an explicit font-size to normalize the sizes between button and anchor? (Like this)
In Storybook we can see the font sizes will be different due to the user agent styles.
There was a problem hiding this comment.
Please see: #52495 (comment)
t-hamano
force-pushed
the
item/fix-focus-state-as-a
branch
from
b19fbfa
to
6f94d80
Compare
t-hamano
force-pushed
the
item/fix-focus-state-as-a
branch
from
6f94d80
to
48b6050
Compare
t-hamano
commented
Jul 12, 2023
Comment on lines
+13
to
+14
| font-size: ${ font( 'default.fontSize' ) }; | ||
| font-family: inherit; |
There was a problem hiding this comment.
Should we also add an explicit font-size to normalize the sizes between button and anchor? (Like this)
I noticed that in addition to font size, the browser default font family was applied to the button element, so I added an inherited style.
t-hamano
changed the title
Item component rendered as anchor element
t-hamano
commented
Jul 12, 2023
| ); | ||
| // Windows high contrast mode. | ||
| outline: 2px solid transparent; | ||
| outline-offset: 0; |
There was a problem hiding this comment.
The outline-offset was added in this PR and is needed to make the outline size consistent in Windows high contrast mode.
|
Thanks for the review, @mirka 🙇 I have updated the styles to work correctly in both the Site Editor and Stylebook. From what I have tested, it appears to be working correctly. site-editor.mp4storybook.mp4 |
t-hamano
commented
Jul 12, 2023
Comment on lines
+56
to
+60
| { | ||
| children: 'Anchor item', | ||
| as: 'a', | ||
| href: 'https://wordpress.org', | ||
| }, |
There was a problem hiding this comment.
I added an anchor element to the Storybook, is that ok?
|
Thanks for the review, @mirka! The E2E test is failing, but this PR should not be the cause, since the same error occurs in trunk. I would like to merge this issue as soon as it is resolved. |
t-hamano
commented
Jul 13, 2023
| "keywords": [ "reusable" ], | ||
| "keywords": [ "reusable" ], |
There was a problem hiding this comment.
When I merged the trunk, it included an unintended modification, but the indentation should be correct with this change 🙈
mirka
reviewed
Jul 13, 2023
packages/components/CHANGELOG.md
Outdated
| @@ -10,6 +10,7 @@ | |||
| ### Bug Fix | |||
|
|
|||
| - `Popover`: Pin `react-dropdown-menu` version to avoid breaking changes in dependency updates. ([52356](https://github.com/WordPress/gutenberg/pull/52356)). | |||
| - `Item`: Unify focus style and add default font styles. ([52495](https://github.com/WordPress/gutenberg/pull/52495)). | |||
There was a problem hiding this comment.
Not very important because it's just an optional check, but FYI the CI check for the changelog is failing because this is missing a number symbol:
Suggested change
| - `Item`: Unify focus style and add default font styles. ([52495](https://github.com/WordPress/gutenberg/pull/52495)). | |
| - `Item`: Unify focus style and add default font styles. ([#52495](https://github.com/WordPress/gutenberg/pull/52495)). |
westonruter
added a commit
that referenced
this pull request
Jul 13, 2023
…dd/defer-script-loading-strategy * 'trunk' of https://github.com/WordPress/gutenberg: (24 commits) Add filter to turn off Interactivity API for a block (#52579) Search: Remove unnecessary useEffect (#52604) Navigation: Simplify the useSelect for useNavigationMenus (#51977) Item: Unify focus style and add default font styles (#52495) Update Changelog for 16.2.1 Bump plugin version to 16.2.1 Avoid passing undefined `selectedBlockClientId` in `BlockActionsMenu` (#52595) Cover Block: Fix block deprecation when fixed background is enabled (#51612) Nav block: link text color inheritance fixes and tests (#51710) Stabilize defaultBlock, directInsert API's and getDirectInsertBlock selector (#52083) Fix console warning by improving error handling in Nav block classic menu conversion (#52591) Fix: Remove link action of Link UI for draft pages created from Nav block does not correctly remove link. (#52415) Lodash: Remove remaining `_.get()` from block editor and deprecate (#52561) Fix importing classic menus (#52573) ResizableFrame: Make keyboard accessible (#52443) Site Editor: Fix navigation menu sidebar actions order and label (#52592) correct a typo: sapce -> space (#52578) Avoid errors in Dimension visualizers when switching between iframed and non-iframed editors (#52588) Patterns: Add client side pagination to patterns list (#52538) Site Editor: Make sidebar back button go *back* instead of *up* if possible (#52456) ...
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.
Related: PRs:
:focus-visible#51787Discovered this while checking the accessibility of the Site Editor.
What?
This PR fixes a problem where the focus state is not applied when
as="a"prop is specified for theItemcomponent and it is rendered as an anchor element. In the latest trunk, the onlyItemcomponent with this prop defined is the "Manage all of my patterns" link in the Site Editor.Why?
In #51576, the focus style has been added when the
Itemcomponent is rendered as abutton. Later, in #51576, it was changed fromfocustofocus-visible. This focus style also applies a transparent border for viewing outlines in Windows high contrast mode. Therefore, when rendered as an anchor element, the focus is not visible in Windows.c53233a55f1eb435f10e5cc36c77909c.mp4
Strictly speaking, however, this is due to the focus style of the anchor element being reset in the Site Editor. If you remove the override in the Site Editor, the outline should be visible, but the outline thickness will be different.
76a508233c6f629fa822bf6fefa96b70.mp4
I think it makes sense to eliminate extra overrides in the ite editor and unify the focus styles of the hoge and a elements at the component level.
I think It would make sense to unify the focus style of the button and anchor element at the component level.
How?
I have applied unstyledButton if it is anchor element. Since the only
Itemcomponent withas="a"is currently the "Manage all of my patterns" button, there should be no impact elsewhere.Testing Instructions
Testing Instructions for Keyboard
Itemcomponents.Screenshots or screencast
Default Mode
100d5b2baf5f57574a8e07f54401ef64.mp4
Windows High Contrast Mode
d08c49f99716e5853db80c5962237f48.mp4