Fix image link preset suggestions arrow key navigation #58615
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
ntsekouras
added
[Focus] Accessibility (a11y)
ntsekouras
requested review from
andrewhayward,
mirka,
ciampo,
afercia and
artemiomorales
|
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 Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf 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. |
|
Size Change: 0 B Total Size: 1.7 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for looking into this. This PR appears to fix the issue 👍
As for why the NavigableMenu component was replaced with a MenuGroup, that was actually done in this commit by @richtabor, who may be able to offer additional insight.
Idea was to closer align with LinkControl, for the future migration to using LinkControl exclusively here. |
mirka
approved these changes
Feb 5, 2024
There was a problem hiding this comment.
Looks good.
Aside from the keyboard navigation aspect, MenuGroup (role="group") is also not a sufficient wrapper for MenuItem (role="menuitem") in this context, because it still needs an enclosing menu. (NavigableMenu is role="menu".) Maybe that could've been more clear in the docs.
|
Thanks all for fixing this.
Yes, that would be appreciated but I'm not sure it would solve the underlying problem. I'd greatly appreciate a discussion on how to prevent such breakages. To me, it looks like:
It's not my intent to blame anyone. We all make mistakes, I make many. However, I see a recurring trend in many pull requests where there's a total lack of keyboard testing and that's less than ideal. I will create a GitHub discussion, any constructive feedback is very welcome. |
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?
Fixes: #58495
When adding a link to an image, the initial view of the UI shows some preset suggestions:
In this PR the
NavigableMenucomponent was replaced with aMenuGroup, which broke the arrow key navigation. I don't see any reason for that change, but @artemiomorales would be good to confirm or explain why is needed and take action accordingly.Step-by-step reproduction instructions