Components: Use aria-pressed attribute instead of the hard-coded is-pressed class
#55004
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
added
[Type] Bug
|
Size Change: -5 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
t-hamano
requested review from
ellatrix,
fluiddot,
dcalhoun and
ajitbohra
as code owners
|
Could we use |
|
Flaky tests detected in 9719b78. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6392470457
|
t-hamano
changed the title
isPressed prop instead of the hard-coded is-pressed classaria-pressed attribute instead of the hard-coded is-pressed class
ciampo
added
Backport to Gutenberg RC
|
In terms of semantics, A few options come to mind:
I'd probably be up for applying both fixes, since one doesn't exclude the other. Finally, one last consideration: this is a typical example of why we discourage consumers of |
|
Thank you for your detailed opinion!
Regarding this PR, do you mean that In that case, when updating the |
In this case,
This one's a bit harder. There aren't any clear additional semantics at play, because it's mostly a visual indication that the underlying menu has something selected. But again,
For what it's worth, this was the case, but it broke other things, so it had to be reverted. If we go ahead with this, we'll need to proceed with caution!
Would be relatively straightforward to add While we're talking semantics, the dropdown menu needs a bit of work. The toggle button should link to the menu with an
|
Yes, we should be using the correct attribute for the context. That might also be |
|
Thank you @andrewhayward for the feedback! So I guess the immediate next step is to put back the As follow-ups, we could:
What do y'all think?
That's good feedback for my work on the next version of |
Yes, I also agree with this approach 👍 |
|
Cool! I guess we can close this PR and open a new one that adds the |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
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.
Fixes: #55002
Related to #54740
What?
This PR uses the
isPressedproparia-pressedattribute instead in components that have manually assigned theis-pressedclass.This PR fixes two unintended pressed styles:
Why?
In #54740,
isPressedprop is now internally converted to thearia-pressedattribute. However, as fixed in this PR,isPressedprop oraria-pressedattribute may not be used and theis-pressedclass is given directly.In this case, the component will not be given the
aria-pressedattribute; the selector for the.is-pressedclass has been removed here, and the intended style will not be applied.How?
Simply replaced with
isPressedproparia-pressedattribute.Testing Instructions
Other concern
As in this case, there may be many consumers who directly grant the
is-pressedclass just to change its appearance. Reinstating the selector that was removed here would solve that, but would instead break the backward compatibility mentioned in #54740.I think we need to explore approaches to resolve these.