Focus first focusable on Link UI #51105
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
jeryj
added
[a11y] Keyboard & Focus
[Feature] Link Editing
|
Size Change: -25 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
This looks good to me, but now we probably need to undo some of the changes to the e2e tests from this PR, since it was assuming the focus on the text input |
getdave
reviewed
Jun 6, 2023
There was a problem hiding this comment.
@jeryj thank you for the PR.
Just noting that this was specifically introduced to ensure the URL field was always focused.
Can we check that with this change, the URL field is always the first focuseable, even if the Text field is displayed?
Once we've confirmed we can update tests accordingly to explicitly assert that the URL field is focused and reference this PR in the code.
|
I'm not sure of all the contexts, but it appears first in the render output, so I think it would have to be the first field. |
jeryj
force-pushed
the
fix/focus-first-focusable-link-ui
branch
from
82a3862 to
3712fc5
Compare
|
Test breakages look related to tab order which has likely changes now you changed the focus rules. This is why I've advised Maggie to avoid relying on tab ordering in the tests we are porting to Playwright and instead have a single test which asserts on keyboard navigation. |
getdave
force-pushed
the
fix/focus-first-focusable-link-ui
branch
from
25b019b to
1b446ef
Compare
|
@jeryj I hope I've fixed the tests here. Ideally we should be able to merge this one. |
getdave
approved these changes
Jun 9, 2023
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Focus first focusable on Link UI * Fix “should preserve trailing/leading whitespace from linked text in text input” test * Fix “should allow for modification of link text via Link UI” test --------- Co-authored-by: Dave Smith <getdavemail@gmail.com>
priethor
added
[Focus] Accessibility (a11y)
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?
Always focus the first focusable element when the Link UI opens
Why?
When something takes focus (like a popover or modal), it should also place focus on the first focusable element. It's an unexpected pattern for it to be placed on the second input.
How?
Instead of guessing the placement of the URL input, always focus the first one (which does happen to be the URL input as of #50957).
Testing Instructions
Screenshots or screencast