Skip to content

Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified #2622

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.

Sign up for GitHub

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

Jump to bottom
Merged
mcking65 merged 62 commits into from
Sep 19, 2023
Merged

Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified #2622

mcking65 merged 62 commits into from
Sep 19, 2023

Conversation

@jongund
Copy link
Contributor

@jongund jongund commented Feb 17, 2023 •
edited by michael-n-cooper-bot
Loading

In the listbox examples, fixes #2545 with the following changes:

  • Updated code to scroll the element referenced by @aria-activedescendant@ into view when view is magnified
  • Updated accessibility features documentation in support of this change.

While fixing this bug, the following additional changes were also made to improve code quality and implement the latest APG code guide practices:

  • Use event.key instead of event.keyCode for identifying keyboard commands.
  • Use class constructors instead of prototype.
  • Changed mousedown event to pointerdown event to support mobile devices.
  • Provide high contrast focus ring around option referenced by aria-activedescendant.
  • Fix bugs in keyboard support for the toolbar
  • Update documentation to improve consistency with current editorial practices

Preview Updated Listbox Examples

Review checklist

WAI Preview Link failed to build on 'Update site files' step. (Last tried on Mon, 18 Sep 2023 23:53:05 GMT).

@jongund jongund requested review from jnurthen and mcking65 February 17, 2023 03:29
@michael-n-cooper-bot michael-n-cooper-bot mentioned this pull request Feb 17, 2023
Closed
@jongund jongund changed the title Bugfix/issue 2545 Update scrolling of listbox item with focus into view when page is magnified Feb 17, 2023
@jongund jongund changed the title Update scrolling of listbox item with focus into view when page is magnified Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified Feb 17, 2023
@css-meeting-bot
Copy link
Member

css-meeting-bot commented Feb 21, 2023

The ARIA Authoring Practices (APG) Task Force just discussed Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified by jongund · Pull Request #2622 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> subtopic: Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified by jongund · Pull Request #2622 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2622
<jugglinmike> Matt_King: The fact that we have multiple list boxes on one page is a problem, but we don't have to address that here
<jugglinmike> Jem: I'm adding a reviewer list to the pull request description, now
<jugglinmike> Matt_King: these examples were never reviewed on a mobile device as far as I know
<jugglinmike> Matt_King: I don't want to expand the scope of this pull request to include mobile bugs

@a11ydoer a11ydoer requested review from alflennik and ariellalgilmore and removed request for jnurthen February 21, 2023 20:02
@ariellalgilmore
Copy link
Contributor

ariellalgilmore commented Feb 26, 2023 •
edited
Loading

For visual testing:
I am not able to rearrange any of the items in the listbox rearrangeable deploy preview:

@css-meeting-bot
Copy link
Member

css-meeting-bot commented Feb 28, 2023

The ARIA Authoring Practices (APG) Task Force just discussed Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified by jongund · Pull Request #2622 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> Subtopic: Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified by jongund · Pull Request #2622 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2622
<jugglinmike> arigilmore: I've commented with notes on my initial review
<jugglinmike> Matt_King: the test failures are blocking the review failures
<jugglinmike> jongund: I haven't had an opportunity to look into those test failures, yet

@jongund
Copy link
Contributor Author

jongund commented Mar 2, 2023

@mcking65
Not sure how to fix the eslinting issues of "Listbox" not defined and "Listbox" not defined.
I guess the previous code using the "aria" object to define Listbox and Toolbar widgets, but it seems that eslint wants the classes all defined in the same file, not in a shared separate "listbox.js" or "toolbar.js" file.
Is there something in the eslint configuration to ignore these issues or is their a coding pattern that would fix this issue?

@a11ydoer
Copy link
Contributor

a11ydoer commented Mar 7, 2023

Mike Pennisi will look into the issue.

@css-meeting-bot
Copy link
Member

css-meeting-bot commented Mar 7, 2023

The ARIA Authoring Practices (APG) Task Force just discussed Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified by jongund · Pull Request #2622 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> Subtopic: Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified by jongund · Pull Request #2622 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2622
<jugglinmike> Matt_King: This had a bunch of changes and it needed a bunch of review
<jugglinmike> jongund: the ESLint linter is complaining about a coding pattern we're using in listbox.js
<jugglinmike> jongund: I tried using modules, but the linter doesn't like that, either
<jugglinmike> howard-e: I have no suggestion right now, but I can take a look
<jugglinmike> Matt_King: is the linter forcing a good practice? That's something I can't answer--it seems like a code style and engineering thing.
<jugglinmike> jongund: A simple fix would be to define the listbox class in each of the four examples that use it, but then you'd have code duplicated.
<jugglinmike> I'll take a look and give jongund some advice

@jugglinmike jugglinmike mentioned this pull request Mar 7, 2023
Merged
@jugglinmike
Copy link
Contributor

jugglinmike commented Mar 7, 2023

Here's an idea for how to fix those linting errors.

@jongund jongund requested a review from jugglinmike July 11, 2023 18:44
@mcking65
Copy link
Contributor

mcking65 commented Jul 24, 2023

I don't understand why 3 of the regression tests are failing. It is also odd that a link that was testing fine earlier today is now failing:

https://accessibility-bookmarklets.org/

@css-meeting-bot
Copy link
Member

css-meeting-bot commented Jul 25, 2023

The ARIA Authoring Practices (APG) Task Force just discussed Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified.

The full IRC log of that discussion <jugglinmike> Subtopic: Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified
<jugglinmike> github: https://github.com//pull/2622
<jugglinmike> s/John/Jon/g
<jugglinmike> Matt_King: This was authored by Jon. He said he's running late and may not make it to this meeting at all.
<jugglinmike> Matt_King: The regression workflows are failing. I've tried to re-run them, but they continue to fail.
<jugglinmike> Matt_King: And they're failing in places that are not related to this pull request.
<jugglinmike> howard-e: I can take a look at what's going wrong. It looks like numbers 1, 2, and 4 are giving us trouble
<Jem> https://accessibility-bookmarklets.org/
<jugglinmike> Matt_King: It's not a truly necessary link, and we can probably remove it. We'll definitely have to remove it if the site doesn't exist anymore!
<jugglinmike> Jem: The site is responding for me right now
<jugglinmike> Matt_King: Okay, so the link checker should probably pass, now. Maybe that was just a temporary anomaly with that website.

@mcking65
Copy link
Contributor

mcking65 commented Jul 25, 2023

@howard-e

regression 2 is still failing after merging in #2749

@howard-e
Copy link
Contributor

howard-e commented Jul 25, 2023

@mcking65 the single error that's reported here is:

not ok - listbox_rearrangeable › content/patterns/listbox/examples/listbox-rearrangeable.html [data-test-id="span-aria-hidden"]: aria-hidden="true" on span[class=icon] elements; 'Element query returned no results: #ex1 span.icon'; more in details 
not ok 65 - listbox_rearrangeable › content/patterns/listbox/examples/listbox-rearrangeable.html [data-test-id="span-aria-hidden"]: aria-hidden="true" on span[class=icon] elements
  ---
    name: AssertionError
    message: 'Element query returned no results: #ex1 span.icon'
    assertion: fail
    at: |-
      Object.queryElements (test/util/queryElements.js:16:7)
      async assertAttributeValues (test/util/assertAttributeValues.js:17:18)
      async test/tests/listbox_rearrangeable.js:132:5
  ...

This isn't related to what #2749 was solving, otherwise all the regression tests would time out instead. The error does reference content/patterns/listbox/examples/listbox-rearrangeable.html however, which this PR modifies so my initial guess is that something has changed to affect the test.

I can take a deeper look tomorrow to help track down the actual cause of the failed test.

cc @jongund

@howard-e
Copy link
Contributor

howard-e commented Jul 25, 2023

Matt_King: The regression workflows are failing. I've tried to re-run them, but they continue to fail.
Matt_King: And they're failing in places that are not related to this pull request.
howard-e: I can take a look at what's going wrong. It looks like numbers 1, 2, and 4 are giving us trouble

I had re-ran 1 & 4 during the meeting too and they passed before the merge. Seems those were flaky.

https://accessibility-bookmarklets.org/
Matt_King: It's not a truly necessary link, and we can probably remove it. We'll definitely have to remove it if the site doesn't exist anymore!
Jem: The site is responding for me right now
Matt_King: Okay, so the link checker should probably pass, now. Maybe that was just a temporary anomaly with that website.

This must have been the case, it's good now!

howard-e
howard-e requested changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@howard-e howard-e left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Following the changes in 38323e3, span.icon no longer exists which is why an error was flagged. The references in test/tests/listbox_rearrangeable should be updated to span.checkmark instead.

@jongund jongund requested a review from howard-e July 26, 2023 22:58
@jongund jongund requested a review from andreancardona August 1, 2023 18:14
@mcking65 mcking65 dismissed jugglinmike’s stale review August 7, 2023 23:01

Requested changes are addressed and reviewed by others

@mcking65 mcking65 removed the request for review from jugglinmike August 7, 2023 23:03
@mcking65
Copy link
Contributor

mcking65 commented Aug 7, 2023

@andreancardona gentle reminder for review of latest code

andreancardona
andreancardona requested changes Aug 15, 2023
View reviewed changes
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

a few minor minor grammatical fixes but all code looks good

content/patterns/listbox/examples/listbox-collapsible.html
<td>
<ul>
<li>Moves focus into and out of the listbox.</li>
<li>If the listbox is expanded, selects the focused option, collapses the listbox, and moves focus out of the listbox.</li>
Copy link
Contributor

@andreancardona andreancardona Aug 15, 2023

Choose a reason for hiding this comment

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

Suggested change
<li>If the listbox is expanded, selects the focused option, collapses the listbox, and moves focus out of the listbox.</li>
<li>If the listbox is expanded, it selects the focused option, collapses the listbox, and moves focus out of the listbox.</li>

Copy link
Contributor

@mcking65 mcking65 Sep 18, 2023

Choose a reason for hiding this comment

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

For brevity, we don't write complete sentences in the description column of the keyboard tables unless a supplementary note is required, which is very rare. The description is a phrase that assumes an implicit subject of the key command.

content/patterns/listbox/examples/listbox-collapsible.html
<ul>
<li>Set by the JavaScript when it displays and sets focus on the listbox; otherwise is not present.</li>
<li>Refers to the option in the listbox that is visually indicated as having keyboard focus.</li>
<li>When an option in the listbox is visually indicated as having keyboard focus, refers to that option.</li>
Copy link
Contributor

@andreancardona andreancardona Aug 15, 2023

Choose a reason for hiding this comment

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

Suggested change
<li>When an option in the listbox is visually indicated as having keyboard focus, refers to that option.</li>
<li>When an option in the listbox is visually indicated as having keyboard focus, it refers to that option.</li>

Copy link
Contributor

@mcking65 mcking65 Sep 18, 2023

Choose a reason for hiding this comment

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

Same as with keyboard commands, for brevity, we avoid repetitiousness by leaving the subject out of the description.

content/patterns/listbox/examples/listbox-collapsible.html
<li>Set by the JavaScript when it displays and sets focus on the listbox; otherwise is not present.</li>
<li>Refers to the option in the listbox that is visually indicated as having keyboard focus.</li>
<li>When an option in the listbox is visually indicated as having keyboard focus, refers to that option.</li>
<li>Enables assistive technologies to know which element the application regards as focused while DOM focus remains on the listbox element.</li>
Copy link
Contributor

@andreancardona andreancardona Aug 15, 2023

Choose a reason for hiding this comment

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

Suggested change
<li>Enables assistive technologies to know which element the application regards as focused while DOM focus remains on the listbox element.</li>
<li>Enables assistive technologies to know which element the application regards to as focused while the DOM focus remains on the listbox element.</li>

Copy link
Contributor

@mcking65 mcking65 Sep 19, 2023

Choose a reason for hiding this comment

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

Adding 'to' would change the meaning. Perhaps we could have used "is identifying" or "is indicating" instead of "regards". As I rethink the wording, it seems that use of "regards" is somewhat anthropomorphizing the web page by implying that the web page can hold something in regard. I don't want to change the wording in this one case, however, because we use identical wording on several example pages. We could consider a PR that revises all instances of this phrase if this is a problematic wording. I'm not convinced that it is.

@andreancardona
Copy link
Contributor

andreancardona commented Aug 15, 2023

@andreancardona gentle reminder for review of latest code

I'm so sorry for the delay on this @mcking65 I've just submitted my review

andreancardona
andreancardona reviewed Aug 22, 2023
View reviewed changes
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

Very small changes

@css-meeting-bot
Copy link
Member

css-meeting-bot commented Aug 22, 2023

The ARIA Authoring Practices (APG) Task Force just discussed Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified.

The full IRC log of that discussion <jugglinmike> Subtopic: Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified
<jugglinmike> github: https://github.com//pull/2622
<jugglinmike> Matt_King: I believe we were waiting on one last code review from andrea
<jugglinmike> andrea: I made some very small requests
<jugglinmike> Matt_King: jongund is out right now. Since andrea wrote these using GitHub's "suggestion" feature, I can apply the changes on jongund's behalf. I'll follow up with that after this meeting
<Jem> https://github.com//pull/2775

@mcking65 mcking65 mentioned this pull request Sep 19, 2023
Open
@mcking65
Copy link
Contributor

mcking65 commented Sep 19, 2023

Opened @jugglinmike.

All feedback has been addressed now.

@mcking65 mcking65 merged commit eed124a into main Sep 19, 2023
@mcking65 mcking65 deleted the bugfix/issue-2545 branch September 19, 2023 00:43
@howard-e howard-e mentioned this pull request Oct 2, 2023
Merged
@mcking65 mcking65 added the enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy label Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcking65 mcking65 mcking65 left review comments

@andreancardona andreancardona andreancardona requested changes

@alflennik alflennik alflennik approved these changes

@howard-e howard-e howard-e approved these changes

@ariellalgilmore ariellalgilmore ariellalgilmore approved these changes

@charmarkk charmarkk charmarkk approved these changes

@jugglinmike jugglinmike jugglinmike left review comments

Assignees

@ccanash ccanash

Labels

enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy

Projects

None yet

Milestone

2023-10-02 Publication Pull Requests

Development

Successfully merging this pull request may close these issues.

Scrollable Listbox Example: List entries are not displayed in the visible area

Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant