-
Notifications
You must be signed in to change notification settings - Fork 345
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
Listbox Examples: Update scrolling of listbox item with focus into view when page is magnified #2622
Conversation
…ing practices and documentation
The ARIA Authoring Practices (APG) Task Force just discussed 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 |
For visual testing: |
The ARIA Authoring Practices (APG) Task Force just discussed 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 |
@mcking65 |
Mike Pennisi will look into the issue. |
The ARIA Authoring Practices (APG) Task Force just discussed 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 |
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: |
The ARIA Authoring Practices (APG) Task Force just discussed 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 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
This isn't related to what #2749 was solving, otherwise all the regression tests would time out instead. The error does reference I can take a deeper look tomorrow to help track down the actual cause of the failed test. cc @jongund |
I had re-ran 1 & 4 during the meeting too and they passed before the merge. Seems those were flaky.
This must have been the case, it's good now! |
There was a problem hiding this comment.
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.
Requested changes are addressed and reviewed by others
@andreancardona gentle reminder for review of latest code |
There was a problem hiding this 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
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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> |
There was a problem hiding this comment.
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.
<td><code>ul</code></td> | ||
<td> | ||
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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> |
There was a problem hiding this comment.
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.
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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> |
There was a problem hiding this comment.
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.
I'm so sorry for the delay on this @mcking65 I've just submitted my review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small changes
The ARIA Authoring Practices (APG) Task Force just discussed 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 |
Opened #2802 to track resolution of the gap in regression testing identified by @jugglinmike. All feedback has been addressed now. |
In the listbox examples, fixes #2545 with the following changes:
While fixing this bug, the following additional changes were also made to improve code quality and implement the latest APG code guide practices:
event.key
instead ofevent.keyCode
for identifying keyboard commands.class
constructors instead ofprototype
.mousedown
event topointerdown
event to support mobile devices.aria-activedescendant
.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).