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

Select-Only Combobox Example: Add click handler to focus combobox when label is clicked #2889

Merged
merged 2 commits into from
Apr 7, 2024

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Dec 19, 2023

Fix issue #2859 by focusing the combobox input when the label is clicked.

Preview Updated Select-only Combobox example

Review checklist

Reviewers: To learn what needs to be covered by each review, Follow the link for the type of review to which you are assigned.



WAI Preview Link (Last built on Sun, 07 Apr 2024 14:52:07 GMT).

<div class="combo js-select">
<div id="combo1-label" class="combo-label">Favorite Fruit</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for changing from label element to div element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label element doesn't really provide any behavior and it may be confusing to add event handlers to the element.

@mcking65 mcking65 changed the title added click event to combobox label to move focus Select-Only Combobox Example: Add click handler to focus combobox when label is clicked Feb 20, 2024
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Fix to make combobox labels clickable.

The full IRC log of that discussion <jugglinmike> Topic: Fix to make combobox labels clickable
<jugglinmike> github: https://github.com//pull/2889
<jugglinmike> Jem: We need a code reviewer and a functional reviewer
<jugglinmike> Matt_King: It could be the same person
<jugglinmike> Matt_King: This is a very small pull request!
<jugglinmike> Matt_King: I posted a question to the patch
<jugglinmike> Matt_King: I don't know why it changes the "label" element to a "div" element. I don't think it has any practical effect either way, but the "label" element seems more semantically accurate
<jugglinmike> Jem: Hearing no volunteers for review, we'll leave this on the agenda for next week

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Fix to make combobox labels clickable.

The full IRC log of that discussion <jugglinmike> Topic: Fix to make combobox labels clickable
<jugglinmike> github: https://github.com//pull/2889
<jugglinmike> Matt_King: I put this back on the agenda because we were trying to get people to review it last week
<jugglinmike> Matt_King: I also had a question for jongund
<jugglinmike> Matt_King: Previously, the label was in a "<label>" element, and now, the label is in a "<div>" element
<jugglinmike> Matt_King: My question for jongund is: was that change intentional?
<jugglinmike> jongund: It was. To me, the "<label>" element wasn't doing anything. I thought a "<label>" element with an associated event handler might be misleading for folks reading to code
<jugglinmike> Matt_King: In the past, we've advocated for using semantic HTML even in the presense of ARIA because the semantic HTML could act as a sort of fallback for browsers/ATs that don't recognize the ARIA
<jugglinmike> Matt_King: But your thinking is kind of the opposite of that
<jugglinmike> Matt_King: It's a more pedagogical case
<jugglinmike> Matt_King: I could see it either way
<jugglinmike> Jem: As a developer, I'm accustomed to using the semantic elements whenever possible
<jugglinmike> Matt_King: Right, but jongund's point is that the "<label>" doesn't do anything in this case. The "click" handler could be confusing to readers. This is example code that we're considering, after all
<jugglinmike> howard-e: From what I've seen, there are more instances of "<div>" elements being modified to support "click" handlers
<jugglinmike> Jem: I typically write code from a "semantics-first" mindset
<jugglinmike> jugglinmike: I'm thinking about graceful degradation. Are there any implications to this distinction in cases where there is no event handler (because the JavaScript has not executed, for whatever reason)
<jugglinmike> dmontalvo: I'm not convinced that this should be changed from "<label>" to "<div>"
<jugglinmike> jongund: "<label>" is a special thing--it does something when used correctly
<jugglinmike> jongund: In the ARIA spec, the group decided not to have a role named "label" because it wouldn't do anything.
<jugglinmike> jongund: If we say that you should use the "<label>" element here, then all our examples that used "aria-labeledby" had better point to an HTML "<label>" element
<dmontalvo> q+
<jugglinmike> Matt_King: I'll have to do some analysis, but I think jongund's making a pretty solid argument (because I don't think a change like the one he's just described would be an improvement)
<jugglinmike> dmontalvo: Thanks jongund, I wasn't seeing the "aria-labeledby" attribute on the "<div>" element. That's an important aspect!
<jugglinmike> dmontalvo: I withdraw my earlier comment
<jugglinmike> s/labeledby/labelledby/g
<jugglinmike> Jem: Since we're using "aria-labelledby", using a "<label>" element doesn't add any value
<jugglinmike> Jem: Could we maybe add a note as a comment?
<jugglinmike> Matt_King: This is covered by the ARIA documentation
<jugglinmike> Matt_King: So I think jongund is helping us to be more consistent with what's already been done in other examples
<jugglinmike> Matt_King: It sounds like we have consensus
<jugglinmike> Matt_King: We need reviewers on this patch
<jugglinmike> Matt_King: We want a code reviewer and a functional review, and that's it
<jugglinmike> Matt_King: We don't need a test reviewer because this doesn't impact the tests
<jugglinmike> siri: I can perform a functional review with Android
<jugglinmike> Matt_King: Great!
<jugglinmike> Matt_King: And I will re-review based on this conversation today
<jugglinmike> Matt_King: I guess that's enough

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Fix to make combobox labels clickable.

The full IRC log of that discussion <jugglinmike> Topic: Fix to make combobox labels clickable
<jugglinmike> github: https://github.com//pull/2889
<jugglinmike> Matt_King: This is basically ready to ship
<jugglinmike> Matt_King: We're waiting on one more review--Siri was going to test it on mobile
<jugglinmike> Matt_King: Since Siri isn't here, I may send her an e-mail about this

Copy link
Contributor

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I am able to see the focus of the combobox when clicking on the label

@shirsha
Copy link

shirsha commented Apr 4, 2024

This is working as expected.

@shirsha shirsha closed this Apr 4, 2024
@shirsha shirsha reopened this Apr 4, 2024
@mcking65 mcking65 merged commit beef80e into main Apr 7, 2024
24 of 25 checks passed
@mcking65 mcking65 deleted the fix/issue-2859-select-only branch April 7, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select-Only Combobox: Clicking the label does not focus the combobox
6 participants