Skip to content
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

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

Being-Maverick
Copy link
Contributor

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

  1. Added check whether the "for" attribute is provided a non-empty value in a <label>.
  2. Added check whether the "for" attribute is provided a non-empty value in a <input>.

Resolves: #7

Screenshot(s)

Screenshot (59)

Checklist:

  • I have thoroughly read the CONTRIBUTING guidelines.
  • I understand my pull request will be thoroughly reviewed at high detail.
  • I understand the work in my pull request will only be available in the next version release of Checka11y.css and not in the current version release.
  • I confirm the work in this pull request is valid according to my findings and is not something for anything personal.
  • I have updated the README where and if applicable (still put an x if you have considered this but thought there was nothing to add or modify).
  • I have added myself to the contributors section in package.json (still put an x if you have considered this but decided not to add yourself).
  • I have checked I have not committed any accidental files.
  • I have tested all the main modern browsers (I.e. Chrome, Firefox, Edge, Safari - please leave this unchecked if there were any browsers listed you could not test and list them in the help section with details why you couldn't test that browser)
  • All new and existing a11y checks still work correctly (compare your local test/index.html to the test/index.html in the master branch).

Help

I could not test in Safari because I do not have access to an Apple device.

@jackdomleo7
Copy link
Owner

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 name attribute, I'm sure you can use id instead of name (but maybe my research is skewed)? So, the following should be also valid:

<label for="firstname">First name</label>
<input id="firstname" />

I'd like to hear your thoughts 😊

@Being-Maverick
Copy link
Contributor Author

I got your point. I think it requires a lot more thought. The current solution will show errors for individual <label> and <input>, which is not what we would like to happen. What we really want is related <label> and <input> to have the mentioned attributes.
Now, these can be related in two ways:

  1.   <label>
         <input/>
      </label>
    
  2.   <label></label>
      <input/>
    

Using CSS we can determine whether this relation exists, like this:

  1.     label > input {
        }
    
  2.     label + input {
        }
    

But, then the CSS will be applied to the <input> element. And since <input> doesn't support content property, the error will not be displayed properly.
We need to somehow apply the CSS to the required label after determining the relation. I researched it, but couldn't any feasible solution. I would love to hear if there is any solution.

Also, the case you mentioned is valid, however, enforcing these rules would guarantee that screenreaders work as desired. Using id won't work however, for and name, work in conjunction and is also the recommended way.

Please share your thoughts about it.

@jackdomleo7
Copy link
Owner

@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.

@jackdomleo7 jackdomleo7 added a11y feature New feature or request for an a11y check needs more info Further information from author is requested labels Oct 2, 2020
@danspratling
Copy link
Contributor

danspratling commented Oct 3, 2020

Now, these can be related in two ways:
1.

  <label>
     <input/>
  </label>
  <label></label>
  <input/>

First, just want to clear up that only 1. is valid @Being-Maverick. 2. is invalid markup as the for attribute is missing on the label. An input directly following a label is never associated with that label.

Anyway, with that out of the way... it seems like what I said before was wrong (sorry about that!).

for attributes should be associated with id not name. Name is only used for form submissions

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.

  1. We put this on the backburner and maybe consider it if we ever device to use javascript in the project leaving a glaring hole in the project which we know is one of the most common accessibility issues
  2. We opinionate the library, and error when labels are not marked up with a for attribute, regardless if an alternate method exists (by nesting).

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?

@alvaromontoro
Copy link
Contributor

alvaromontoro commented Oct 3, 2020

@danspratling CSS has a pseudo-class selector that would allow doing this: :has()... Unfortunately, it is not supported by any major browser yet, but if/when it does, it would be helpful for detecting many issues:

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 id, but no label pointing to that id), but it will highlight some wrong ones.

@jackdomleo7
Copy link
Owner

@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!

@Being-Maverick
Copy link
Contributor Author

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 for attribute at the label and then display a suitable warning. In the future, where :has() selector is supported then we can upgrade
it to check for input as well.

What are your thoughts?

@alvaromontoro
Copy link
Contributor

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:

screenshot from demo showing 3 fields

@Being-Maverick
Copy link
Contributor Author

Yeah, this seems good. Let's hear what @jackdomleo7 has to say.

@jackdomleo7
Copy link
Owner

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. 🙂

@alvaromontoro
Copy link
Contributor

I don't think the selectors above would mark false positives, but I could be wrong.

If an <input> does not have an id, it cannot be target of a for in a <label>; which would be Ok if it the <input> had the attributes aria-label and/or aria-labelledby. That's why the selector is for <input>s that don't have id, aria-label, and aria-labelledby (all three missing). Then, for the <input>s that fall in that category, we remove the error if they are inside a <label> (as in that case it would be Ok too).

@jackdomleo7
Copy link
Owner

Let me doing some more manual testing when I get chance. 😊

@jackdomleo7 jackdomleo7 changed the base branch from master to valid_input_and_label November 12, 2020 22:44
@jackdomleo7 jackdomleo7 self-requested a review November 12, 2020 22:44
@jackdomleo7 jackdomleo7 merged commit 693dbf3 into jackdomleo7:valid_input_and_label Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y feature New feature or request for an a11y check needs more info Further information from author is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inputs missing labels & Labels missing inputs
4 participants