-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Checks added for attribute "for" in label and for attribute "name" in input element #43
Checks added for attribute "for" in label and for attribute "name" in input element #43
Conversation
Thanks for looking at this! It looks really good, however my concern with this approach is that it will show a false negative. I.e The below is valid, but looks like it will be shown as an error: <label>
First name
<input type="text" />
</label> Similar with the <label for="firstname">First name</label>
<input id="firstname" /> I'd like to hear your thoughts 😊 |
I got your point. I think it requires a lot more thought. The current solution will show errors for individual
Using CSS we can determine whether this relation exists, like this:
But, then the CSS will be applied to the Also, the case you mentioned is valid, however, enforcing these rules would guarantee that screenreaders work as desired. Using Please share your thoughts about it. |
@danspratling would be interesting to hear your thoughts on this 🙂 I'm happy to have half a feature, but I'd like to avoid false errors if we can. I think @Being-Maverick is going somewhere good with their previous comment and are on the right lines. |
First, just want to clear up that only Anyway, with that out of the way... it seems like what I said before was wrong (sorry about that!).
You can see that in action here if you click the text, rather than the checkboxes. I also checked this with mac voiceover and neither input1 or input3 are read out. Only input2 (using id) is read out correctly. https://codepen.io/dan_spratling/pen/MWyNvOB Input4 however, is read out correctly and we definitely don't want to be creating false errors. However I'm not sure the best way to approach this in css either. By it's very nature, css is unable to navigate backwards up the dom tree, so once you're inside an element you cannot get back out. I'm not sure the best solution here, as this means we can never check if an input exists inside a label while removing the error from the parent. I feel like this is a problem only javascript would be able to fix. Which leaves us with 2 options.
I would strongly recommend that we go for option 2 here, even if it throws up false errors. Maybe we could add a note to the readme and a very clear explanation in the error description and allow users to turn it off somehow if they wish. @jackdomleo7 what are your thoughts? |
@danspratling CSS has a pseudo-class selector that would allow doing this: label:not(:has(input)) {
/* selects a label that doesn't have an input inside */
}
section:not(:has(h1,h2,h3,h4,h5,h6)) {
/* selects the sections that don't have a heading */
}
... That being ideal, but not a realistic option at the moment. A different approach that could be attempted: /* Make all the input fail if they don't have aria-label, aria-labelledby, or id. */
input:not([aria-label]):not([aria-labelledby]):not([id]) {
/* error */
}
/* If those inputs are inside of a label, remove the error */
label input:not([aria-label]):not([aria-labelledby]):not([id]) {
/* remove error */
} This misses some cases (e.g., inputs with an |
@danspratling I'm not sure how I feel about sometimes throwing false errors (I'm not sure if it will be more helpful or less helpful to the user depending on the feature). But we can certainly try it! I really do like @alvaromontoro's approach too! |
I agree with the approach suggested by @alvaromontoro. But then as I mentioned before, we can't set content property on the input tag. Here is an example: https://codepen.io/charchitkapoor/pen/BazBBBm Therefore, we can't display an error if the input is missing one of the attributes. I would suggest rather than displaying the current issue as an error, we should display it as a warning. We should check for What are your thoughts? |
You could combine this approach with what @cat-street suggests on this other issue, and style the input that has an error. Here is a demo: https://codepen.io/alvaromontoro/pen/abNegmy One problem with this approach is that if the input was styled, it loses some of the styles if it's correct, as you can see on the demo: |
Yeah, this seems good. Let's hear what @jackdomleo7 has to say. |
This is an interesting one, so all I'm going to ask is, will it show any errors for anything that isn't an error? 🙂 I'd rather have a tool that doesn't include a feature due to limitations, than one that shows errors for a few scenarios and falsely shows them for other scenarios. It's perfectly ok for this tool to not pick up everything because there are other backup tools for that such as editor plugins and tools like Chrome's Lighthouse. 🙂 |
I don't think the selectors above would mark false positives, but I could be wrong. If an |
Let me doing some more manual testing when I get chance. 😊 |
Types of changes
Description
<label>
.<input>
.Resolves: #7
Screenshot(s)
Checklist:
x
if you have considered this but thought there was nothing to add or modify).contributors
section inpackage.json
(still put anx
if you have considered this but decided not to add yourself).test/index.html
to thetest/index.html
in themaster
branch).Help
I could not test in Safari because I do not have access to an Apple device.