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

Scrollable Listbox Example: Ad default value 'None' so first item can be focusable on load #3139

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

wagnermaciel
Copy link
Contributor

@wagnermaciel wagnermaciel commented Oct 3, 2024

Fixes #3138

Preview

Preview changes to Scrollable Listbox Example in compare branch

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.

Not applicable:


WAI Preview Link (Last built on Wed, 06 Nov 2024 01:45:17 GMT).

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed PR 3139 - Scrollable listbox fix.

The full IRC log of that discussion <Jem> Topic: PR 3139 - Scrollable listbox fix
<Jem> github:https://github.com//pull/3139
<Jem> mck: This is ready to review and added the checklist
<Jem> ..it touches only two files, html and js
<Jem> arie: one testing failed, snapshot issue.
<Jem> jem: assertion error for aria-selected.
<Jem> jon will do code and testing review

Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

Thank you for this update to scrollable listbox.
You need to also update the test case for the additional option:
https://github.com/wagnermaciel/aria-practices/blob/listbox/test/tests/listbox_scrollable.js

With the following change to the const ex:

const ex = {
  ...
  numOptions: 27,  
  firstOptionSelector: '#ex #ss_elem_None',  
};

Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

As long as the recession tests work, I approve the change.
Thank you for the update!

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed PR 3139 - Scrollable listbox fix.

The full IRC log of that discussion <jugglinmike> Topic: PR 3139 - Scrollable listbox fix
<jugglinmike> github: https://github.com//pull/3139
<jugglinmike> Matt_King: jongund requested a change from the author, and it looks like they made that change
<jugglinmike> Matt_King: And all the tests pass
<jugglinmike> jongund: I have reviewed their work since they made the changes, so it looks good to me, now
<jugglinmike> jongund: I was doing code review and test review
<jugglinmike> Matt_King: I think this is now just waiting on me. I'll review it, and if it's looking good, then it'll get merged!

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

@wagnermaciel

Thank you for the fix! We'll get it into the next publication on December 12.

@mcking65 mcking65 changed the title Fix scrollable listbox not focusing first item Scrollable Listbox Example: Ad default value 'None' so first item can be focusable on load Nov 19, 2024
@mcking65 mcking65 merged commit b0c04c4 into w3c:main Nov 19, 2024
12 checks passed
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.

Bug: First element in listbox should receive focus
4 participants