downshift-js / downshift

🏎 A set of primitives to build simple, flexible, WAI-ARIA compliant React autocomplete, combobox or select dropdown components.
http://downshift-js.com/
MIT License
12.06k stars 929 forks source link

Incorrect use of <label for=FORM_ELEMENT> #1608

Closed endaquigley closed 2 months ago

endaquigley commented 4 months ago

Relevant code or config:

Issue exists in the Downshift docs.

What you did:

What happened:

Screenshot 2024-06-05 at 14 13 16

Problem description:

Incorrect use of \

The label's for attribute doesn't match any element id. This might prevent the browser from correctly autofilling the form and accessibility tools from working correctly.

To fix this issue, make sure the label's for attribute references the correct id of a form field.

Suggested solution:

Not sure if this is a false positive as the component still seems to work as intended.

silviuaavram commented 4 months ago

Hi. Which examples, exactly? The ones I checked work.

Screenshot 2024-06-05 at 16 56 31
endaquigley commented 4 months ago

Hi @silviuaavram,

Yes, the components work as expected but we still get the following error in the DevTools issues panel.

7 resources affected, so I'm guessing it affects all the examples on the page:

Screenshot 2024-06-05 at 15 05 54
silviuaavram commented 4 months ago

Well, devTools may be wrong. Specifically here, there are no issues, the ids match, the click and focus technique works, not sure what we can do about it.

silviuaavram commented 4 months ago

Maybe, just maybe, it complains that the target is a div, not a form element.

Now, to be honest, I don't think we need the for here in the label markup. If I remember we manually focus the div on label click, and we also use aria labelledby on the div in order to get the markup from the label.

If I'm correct I propose to fix it by removing the for attribute, update the unit tests, and check that the click/focus and label works as before. Let me know if you want to create a PR. Thank you!

endaquigley commented 4 months ago

Switching to a button element removes the DevTools error but it also changes the behaviour of the component (clicking on the label will now open the menu, instead of just focusing on the element like before).

According to the Wayback Machine it looks like Downshift used to recommend using a button element but switched to a div element a few years ago to be WAI-ARIA 1.2 compliant https://github.com/downshift-js/downshift/issues/1335


Screenshot 2024-06-06 at 02 43 22

Removing the htmlFor attribute from the label element causes another issue:

A <label> isn't associated with a form field.

Hard to know what to do, we’re probably going to stick with the div element and treat it as a false positive.

silviuaavram commented 4 months ago

That’s true, we performed this change a few versions ago. Consequently I would try the implementation without the for attribute and double check if it’s working properly from the a11y point of view.

On Thu, 6 Jun 2024 at 11:21, Enda Quigley @.***> wrote:

Switching to a button element removes the DevTools error but it also changes the behaviour of the component (clicking on the label will now open the menu, instead of just focusing on the element like before).

According to the Wayback Machine it looks like Downshift used recommend using a button element but switched to a div element a few years ago to be WAI-ARIA 1.2 compliant #1335 https://github.com/downshift-js/downshift/issues/1335

Screenshot.2024-06-06.at.02.43.22.png (view on web) https://github.com/downshift-js/downshift/assets/753515/31c9e3ff-bf5f-467f-bf5c-710922cbb343

Hard to know what to do, we’re probably going to stick with the div element and treat it as a false positive.

— Reply to this email directly, view it on GitHub https://github.com/downshift-js/downshift/issues/1608#issuecomment-2151691944, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACWAZAGOO6JAQBPVEETUUWLZGALZ3AVCNFSM6AAAAABI2XHMWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJRGY4TCOJUGQ . You are receiving this because you were mentioned.Message ID: @.***>

endaquigley commented 4 months ago

I updated my previous comment shortly after posing it as I forgot to mention what happens when you remove the htmlFor attribute from the label element.


Removing the htmlFor attribute from the label element causes another issue:

A <label> isn't associated with a form field.
silviuaavram commented 2 months ago

Closing since we can't do any relevant fix here, and the end result is actually what we want.