form-token-field/TokenInput: Fix automatic focus on first render due to updating selected value from async stores #44347
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
alexstine
added
[Type] Bug
alexstine
requested review from
talldan,
ciampo,
afercia,
joedolson and
tellthemachines
|
Size Change: +26 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
alexstine
commented
Sep 22, 2022
ciampo
requested changes
Sep 23, 2022
There was a problem hiding this comment.
Thank you for working on a potential fix, @alexstine .
Unfortunately I don't think that the current approach is correct, as explained in the inline comments below.
Reading your comments in this PR and in #42285, I'm not sure that we have an exact understanding of what the correct fix should be to solve the regression.
It also looks like the issue may be more in the way certain combinations of screen readers announce updates, rather than an issue in the code here?
Comment on lines
-250
to
-253
| aria-label={ | ||
| currentLabel | ||
| ? `${ currentLabel }, ${ label }` | ||
| : null |
There was a problem hiding this comment.
Is it ok to remove this aria-label, given that we're already using BaseControl ?
CC'ing @tellthemachines since it looks like she introduced this change in #27431
There was a problem hiding this comment.
@ciampo If done right, there is no reason native HTML should not work for this control as suggested by @joedolson in the related issue. Sometimes ARIA complicates things and sometimes it has a really good use. I am not sure how this works for Mac yet as I have not tested it.
| @@ -81,6 +81,7 @@ function ComboboxControl( { | |||
| const [ inputHasFocus, setInputHasFocus ] = useState( false ); | |||
| const [ inputValue, setInputValue ] = useState( '' ); | |||
| const inputContainer = useRef(); | |||
| const changingLabel = selectedSuggestion?.label || inputValue; | |||
There was a problem hiding this comment.
Apart from that, the introduction of the changingLabel variable completely breaks the typing interaction in ComboboxControl, since the input won't show what's being typed by the user when in its expanded state and a value has already been selected
(the video below shows what just described)
combobox-input-broken.mp4
There was a problem hiding this comment.
I wrote this in such a way that if suggestions do not have a match, then show what is typed but I agree this needs refinement.
|
@ciampo I understand the problem very clearly. In Firefox with NVDA on Windows, the This is certainly a code problem because
If you have other ideas on how to fix, open to discussion. Otherwise, I'd like to find a way to make this work as placing focus on a combo box hurts my workflow as an Accessibility Team rep because that is where focus goes every time I add/edit a post on the Make WordPress Accessible site. Due to the fact there are more than 25 users to select from. |
|
@ciampo Out of interest, what did you think of this approach? This, in my opinion, is much more of a mess because we are guessing developers intent. Essentially, I am trying to guess when the initial render is done and when the async updating finishes. Based on this, that is when I dynamically apply the Thoughts? |
…form-token-field-a11y
…us alone via the special prop.
alexstine
force-pushed
the
refactor/form-token-field-a11y
branch
from
62b1e7e to
98e96e7
Compare
alexstine
commented
Sep 28, 2022
There was a problem hiding this comment.
@ciampo Okay, sorry about this. I completely misunderstood @joedolson comment. I thought the goal was to make value handle everything, but the goal was only allowing value to handle the initial selection. I have refactored the PR to work much better. Please let me know your thoughts about implementation in this version.
| @@ -247,11 +247,7 @@ function ComboboxControl( { | |||
| instanceId={ instanceId } | |||
| ref={ inputContainer } | |||
| value={ isExpanded ? inputValue : currentLabel } | |||
| aria-label={ | |||
There was a problem hiding this comment.
Should not need this any longer since value displays the currently selected option.
| ? `${ currentLabel }, ${ label }` | ||
| : null | ||
| } | ||
| inputHasFocus={ inputHasFocus } |
There was a problem hiding this comment.
Need to pass this so I know if input has focus or not.
| @@ -22,6 +22,7 @@ export function UnForwardedTokenInput( | |||
| const { | |||
| value, | |||
| isExpanded, | |||
| inputHasFocus, | |||
There was a problem hiding this comment.
Gives me the focus of the input field.
| @@ -62,7 +63,7 @@ export function UnForwardedTokenInput( | |||
| : undefined | |||
| } | |||
| aria-activedescendant={ | |||
| selectedSuggestionIndex !== -1 | |||
| inputHasFocus && selectedSuggestionIndex !== -1 | |||
There was a problem hiding this comment.
Only add this attribute if input has focus. Otherwise, value takes care of displaying the currently selected option.
| @@ -199,6 +199,7 @@ export interface TokenProps extends TokenItem { | |||
|
|
|||
| export interface TokenInputProps { | |||
| isExpanded: boolean; | |||
| inputHasFocus?: boolean; | |||
There was a problem hiding this comment.
I think it is okay to keep this as optional, I just need to add a fallback to handle this if the var does not exist.
| @@ -30,6 +31,9 @@ export function UnForwardedTokenInput( | |||
| } = props; | |||
|
|
|||
| const size = value ? value.length + 1 : 0; | |||
| // Ensure this is set to true if inputHasFocus is not defined. | |||
| const checkInputHasFocus = | |||
There was a problem hiding this comment.
It is not a given that this var will be used for this component. If it is not defined, still allow us to keep same functionality.
|
Thank you @alexstine for your determination in working on an alternative approach! Apologies if I'm not being very responsive, but I've been having a busy week (and I won't be around much next week too). If I understand this new approach correctly, the idea is to add the If that's the case, I suggest a few improvements:
I applied these ideas to a separate PR: #44526. It would be great if you could take a look and let me know if the changes proposed in that PR make sense to you and test well |
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 #42285
Adds code to
TokenInputto detect two things.valuevariable. If one is passed in, this means there will be a dynamic selection index update which will cause focus.TokenInputgains focus by a user, first render goes to false.Why?
This is a nasty A11Y bug that needs to go away.
How?
I think, magic.
Testing Instructions
Screenshots or screencast