Ensure a span is used for non-labelable elements when using <Label />#3831
Open
RobinMalfait wants to merge 6 commits intomainfrom
Open
Ensure a span is used for non-labelable elements when using <Label />#3831RobinMalfait wants to merge 6 commits intomainfrom
span is used for non-labelable elements when using <Label />#3831RobinMalfait wants to merge 6 commits intomainfrom
Conversation
This ensures that the DOM is valid, otherwise using a native `<label for="…">` with an ID that points to a non-labelable element could result in an issue. A labelable element is defined here: > button, input (if the type attribute is not in the Hidden state) meter, output, progress, select, textarea, form-associated custom elements > > — https://html.spec.whatwg.org/multipage/forms.html#category-label But it doesn't say anything about custom implementations where you implement the behavior yourself. E.g.: components with `role="checkbox"` With this implementation, we fallback to a simple inline `span` which is not focusable, but is clickable, mimicking the native `<label>`. We still use the `aria-labelledby` attribute so tools like VoiceOver work as expected. Last but not least, the `for` (`htmlFor`) attribute is removed when dealing with a `span` instead of a native `label`.
We already figured out that we only ever use a real `<label>` when we target a real labelable element. This also means that we can entirely rely on the native browser behavior for labels. However, when we are _not_ dealing with a labelable element, we use a span and therefore we have to perform that `click` logic ourselves (which we conveniently already had).
`el.role` is undefined in jsdom, but `el.getAttribute('role')` is not.
So we can fallback to that, but short-circuit if `el.target` is
available.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
tests need to be tweaked now that a |
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
2 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.
The
<label for="…">requires that the target element is labelable. The spec defines labelable elements as:It doesn't say anything about custom implementations where a
role="…"is being used (even though it works as expected).This PR fixes the issue shown in devtools, by using a
spanwithout theforattribute in case we are pointing to a non-labelable element.Unfortunately, we only know what we are pointing to at runtime, so during SSR we will use a
spanas well, but once everything is hydrated, a properlabelwill be used if possible.In a perfect world, we do use the native
<label>and switch tospanif we know it's invalid, but then the devtools warning still shows since I believe it checks the HTML coming from the server.Fixes: tailwindlabs/tailwind-plus-issues#1792
Test plan
spanworksBefore:
After:
