Prevent navigation on url input suggestion selection via enter key #40906
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
draganescu
requested review from
Mamaduka and
youknowriad
and removed request for
ellatrix
draganescu
added
[Type] Bug
|
Size Change: +6 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
alexstine
added
the
[Focus] Accessibility (a11y)
|
@draganescu I'll check this out later today and make sure it still meets A11Y requirements. Just a side note, I added "Accessibility" label so that way it could be more easily found. There are so many sub accessibility labels it would take me ages to look through them all. Generally, I always tag with "Accessibility" and whatever necessary sub-label. Thanks. |
Mamaduka
approved these changes
May 9, 2022
There was a problem hiding this comment.
The fix works as expected ✅ Tested in Chrome and Firefox.
I'm approving the PR, but let's wait for @alexstine's a11y review.
Which menu option is that?
Thanks. |
|
That would be the "Edit" button in the "Replace" dropdown. |
|
@Mamaduka I do not see an Edit option in the Replace dropdown. Just the options I posted in my last comment. 😕 |
|
@alexstine, It's usually the last option in that dropdown — part of the "Current media URL:" form. P.S. We probably should make that text a proper label. |
|
@alexstine to get to the pencil icon with the keyboard once you are in the replace drop down menu you use arrow keys to navigate the 1st two options but you need to press tab to get to the next section of the drop down menu where the edit button is. Pressing tab will focus the edit button. |
|
@draganescu Could you please create a follow-up issue to change how this works? Pressing Tab to get to the Edit button is horribly confusing and there is no way to navigate back. This block needs help. This is an example of how you can create something so inaccessible that I didn't even know about it unless told. It presents a real problem as I would have never found this during testing. 😞 Thanks. |
|
This looks to be a breaking change. If you were using a I'm not sure if there is a good way to fix the BC issue without also reverting the fix. But I wanted to flag it as it broke one of our blocks when updating to 6.1. Example code: <form
onSubmit={ ( e ) => {
e.preventDefault();
setAttributes( {
productHref: editingLink,
} );
onClose();
} }
>
<URLInput
disableSuggestions
value={ editingLink }
onChange={ setEditingLink }
/>
</form> |
|
@TimothyBJacobs oups! How should we document it best for those affected? |
|
I think a note in the changelog that points to |
|
After #50158 is merged, this should be less of a breaking change. |
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 #38950
Fixes #40449
Why?
We don't have to navigate to selected urls.
How?
For the enter key on the suggestion list for url input there is a select behavior which did not have preventDefault called on the key event.
Testing Instructions
https://live.staticflickr.com/3784/12091853594_b6f2668721_b.jpgOther
The changes seem to come from #32552 but not 100% sure and also there is a comment that says
... but I don't think this is about the ENTER key. In the past (looking through commits the enter key was handled directly in the button's onClick prop. Later it had a stopPropagation, not it has no handling, hence the error.
cc @youknowriad for a trip down memory lane if there is anything from the past refactoring that required the ENTER key to not be handled.