Conversation
ciampo
added
[Type] Bug
ciampo
commented
Jul 4, 2022
Contributor
Author
There was a problem hiding this comment.
For test purposes only. I will remove this storybook example before merging
|
Size Change: +3 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
alexstine
approved these changes
Jul 4, 2022
Contributor
alexstine
left a comment
alexstine
left a comment
There was a problem hiding this comment.
@ciampo LGTM.
The reason why I did not do this myself is I did not know if this would be against style guide naming convention to have a double component prefix before the instance ID. If you think it is fine, that is a simple enough fix for me.
Contributor
Author
|
Thank you for the quick review!
There isn't a convention (at least that I'm aware of), especially since the Cc'ing @sarayourfriend (who may have some additional context on this function) and @mirka in case I'm missing anything here, before merging |
alexstine
added
the
[Focus] Accessibility (a11y)
This reverts commit e832770c5db0cb4393d7169cc4141ac2553cb4d4.
ciampo
force-pushed
the
fix/combobox-control-form-token-field-duplicated-token-input-id
branch
from
d6f89ef to
7a21bfa
Compare
ciampo
deleted the
fix/combobox-control-form-token-field-duplicated-token-input-id
branch
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 #42112
Fixes a bug which caused different
inputelements to be assigned the sameidin case aComboboxControland aFormTokenFieldcomponents are rendered in the same page and are assigned the sameinstanceId.I initially wanted to add a unit test for this, but:
ComboboxControlhas no unit tests (😱)FormTokenField's tests are written withReact.TestUtils(which I never used before)...and so, for the sake of landing this fix quickly, I decided to skip unit tests.
Why?
Every ID assigned to an element in a page should be unique — non-unique IDs technically make the page's markup non-valid and can cause severe difficulties to users relying on assistive technology.
See #42112 (comment) for a more detailed explanation.
How?
In order to avoid the
TokenInputcomponent to receive the sameinstanceIdfrom bothComboboxControlandFormTokenField, theComboboxControlcomponent now passes a customprefixwhen callinguseInstanceId. This means that, while theids received byFormTokenFieldare simple numbers (1, 2, 3..), theids generated forComboboxControlwill all be prefixed (i.e.combobox-control-1,combobox-control-2,combobox-control-3...)Testing Instructions
Follow the instructions in #42112 's description, the bug should be not reproducible anymore.
Alternatively, I've added a temporary Storybook example:
npm run storybook:devand visit http://localhost:50240/?path=/story/components-comboboxcontrol--test-duplicate-instance-idsComboboxControlandFormTokenFieldreceive the sameinstanceIdinternally)inputelements rendered in the page, assess that they have differentidsFor completeness, undo the changes to
packages/components/src/combobox-control/index.jsintroduced from this PR, repeat the points above, and verify that the bug can be reproduced.Screenshots or screencast