-
Notifications
You must be signed in to change notification settings - Fork 425
All Pages: Update SkipTo feature to version 5.9 #3213
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm finding the extra information in the group labels very distracting. It is making the menu content harder to understand. Why is it including only headings in main? Seems like we should not be limiting the list of headings to main content. I don't recall when that change was made, but I'm not sure it's the most desirable approach. The more information button in the about dialog gives a 404. |
|
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: Pull request 3213 - Update skipTo<jugglinmike> github: https://github.com//pull/3213 <jugglinmike> Matt_King: I added some comments to the pull request <jugglinmike> Matt_King: Before we dive in: in general, jongund made changes to the menu that is included at the top of each page <jugglinmike> Matt_King: This skipTo includes both landmark regions and headings, but only headings in the main content. I didn't realize that until yesterday. I'm wondering where that decision came from--that we would exclude headings from any other part of the page <jugglinmike> jongund: That is configurable in skipTo. We can reconfigure it to show all headings <jugglinmike> jongund: There's now browser extension versions of skipTo where users have access to a lot of configuration <jugglinmike> Matt_King: That configuration appears on every single page--we don't have it centrally. <jugglinmike> jongund: I could change the default to be all headings <jugglinmike> Zakim, end the meeting |
|
@mcking65 |
|
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 3213: Update SkipTo feature to version 5.8<jugglinmike> github: https://github.com//pull/3213 <jugglinmike> Matt_King: A few weeks ago, I gave jongund some feedback (especially on group labeling) <jugglinmike> Matt_King: The skipTo feature that is at the top of every page comes from a shared script inside of content/shared/js <jugglinmike> Matt_King: jongund copies that in directly from what he maintains in his skipTo repository <jugglinmike> Matt_King: This is version 5.8 of the script <jugglinmike> Matt_King: The pull request has been updated,but I can't tell what exactly has changed <jugglinmike> Matt_King: As a screen reader user, I have found the extra information in the group label to be not helpful <jugglinmike> jongund: There is in the actual group label, information about the number of headings and labels <jugglinmike> Matt_King: I'd like it removed from the screen reader label of the group <jugglinmike> Matt_King: When I go from one group to another group, JAWS reads the group label before it reads the menu item. So I'm on the APG homepage, and I open the skipTo. When it first opens, it's not particularly distracting. But when I'm navigating the menu (I get to "Complimentary" which is the third landmark <jugglinmike> Matt_King: This might be a general problem with putting groups in menus, but it just makes it harder to understand the menu <jugglinmike> jongund: Should we give the group an empty heading? <jugglinmike> Matt_King: I think, since there is a group of landmark regions and a group of headings... <jugglinmike> Matt_King: I think if it just said "landmark regions" instead of the number of landmark regions. And "headings". <jugglinmike> Matt_King: Maybe the thing that makes it hard to understand (at least to me) is adding the number. Because there are are a lot of numbers elsewhere in this text <jugglinmike> jongund: Okay, I can remove the number <jugglinmike> Matt_King: I don't care if you leave the number in visually; it's just redundant information for a screen reader user <jugglinmike> jongund: The reason for the number is because there are so many items that they can visually fill up the screen <jugglinmike> jongund: Regarding the scrolling behavior; people can try it out and share some feedback <jugglinmike> Matt_King: What does this mean, "labels are always visible"? <jugglinmike> jongund: If there are a lot of landmarks on the page, you wouldn't even see the headings group <jugglinmike> jongund: I clip them so that if there's more, there will be a scrollbar <jugglinmike> Matt_King: So it's separately visually scrollable with the mouse or keyboard <jugglinmike> jongund: That's right <jugglinmike> jongund: The menu is visually smaller, and there's an "about skipTo" message at the bottom <jugglinmike> Matt_King: Oh, right. That should say something about it being an external link. Right now, it just says "about skipTo.js". It doesn't say that it's going to another website <jugglinmike> jongund: It only opens a dialog <jugglinmike> Matt_King: Ah, yes. And from there, it gives you an option to go to another site <jugglinmike> Matt_King: Okay, so no one is going to leave APG without realizing that they are leaving APG <jugglinmike> Matt_King: Okay, we need some reviewers <jugglinmike> Matt_King: We don't need a code review (this code isn't meant to be pedagogical code for others; it's only infrastructure) <jugglinmike> Matt_King: Though if anyone wants to review the code, that's great! <jugglinmike> Matt_King: I'm mostly interested in the functionality, both with and without a screen reader <jugglinmike> arigilmore: I can try <jugglinmike> Matt_King: Thank you, arigilmore! I will assign you |
|
@howard-e |
|
I have updated to version 5.8.2 to fix some other issues, would be good to review once the preview gets re-built. |
|
This looks good to me! The only thing is not sure if i'm correctly trying on mobile. It reads the skip-to button, but I'm not able to initiate the skip-to on mobile using an Iphone with VO. |
|
Hi @jongund, I cleared my iOS Safari cache and I’m afraid I still get the same results. I asked two of my colleagues to try the preview link on their iPhones, and I have good news and bad news. 😅 It worked correctly for one of them, but the other got the same result that I do. 🤦🏻 All three of us are running iOS 26 and have default VoiceOver settings. In my case, this aspect is suspicious: upon focus, the button‘s |
|
@adampage Accessibility > Keyboard & Typing > Full Keyboard Access > On When I turn this feature off I replicate the behavior on my iPhone you and other people in the task force are reporting. Thank you to you and all the other task force members who have helped in identifying this issue. |
|
Heya @jongund, I made a quick Codepen to satisfy my own curiosity about |
I tried translate and it did not seem to make a difference, but functionally works the same. |
|
Heya @jongund, I just checked out the latest deployment in iOS with both Chrome and Safari with VoiceOver. Fixed! 💪🏻 |
|
I tested with Android and Talk Back and it was working |
adampage
left a comment
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.
This is terrific, @jongund. Approval for me 🚀 , but I did leave a few questions/suggestions for your consideration.
I also checked it out in Android, where it seems to be working great. 👍🏻
|
|
||
| msgNextRegion: 'Next region', | ||
| msgPreviousRegion: 'Previous region', | ||
| msgRegionIsHidden: 'Region is hidden', |
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.
I encountered this “Region is hidden” message but didn’t quite understand the intention? The message also seems like it may be difficult if not impossible for a screen reader user to discover, since it doesn’t trigger any announcement when it appears, and then it’s quick to vanish when I navigate elsewhere. Is that by design?
Here’s how I’m able to reliably get the message to appear from a fresh load of the preview APG home page in macOS Chrome:
- Tab once to reveal “Skip to Content” button.
- Press
Enterto open. - Press
Gto scroll and move focus to the “Get Involved” heading. - Press
Nto scroll to the main navigation region. - (“Region is hidden” message appears).
- Interestingly, a second press of the
Nkey here now assigns focus to the navigation region.
| shortcutRegionComplementary: 'c', | ||
|
|
||
| shortcutsGroupEnabledLabel: 'Shortcuts: Enabled', | ||
| shortcutsGroupDisabledLabel: 'Shortcuts: Disabled', |
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.
I was a little disoriented by the invisible <div role="separator">Shortcuts: Disabled</div> that’s exposed in the a11y tree between “Help improve this page” and “About SkipTo.js”? Is this meant to be discoverable to assistive tech users?
| } | ||
| else { | ||
| if (menuButtonNode.classList.contains('popup')) { | ||
| const popupOffset = -1 * rect.height + 'px'; |
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.
| const popupOffset = -1 * rect.height + 'px'; | |
| const popupOffset = -1 * Math.ceil(rect.height) - 1 + 'px'; |
This is a super cosmetic nitpick, but I think because I’m reviewing on a high pixel density display, I can see a tiny sliver of the concealed skip button at the top edge of my viewport. How about rounding that rect.height up to the nearest whole pixel and then adding a small buffer to ensure it falls off screen?
content/shared/js/skipto.js
Outdated
| templateInfoDialog.innerHTML = ` | ||
| <dialog id="${DIALOG_ID}"> | ||
| <div class="header"> | ||
| <h2>Keyboard Shortcuts</h2> |
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.
| <h2>Keyboard Shortcuts</h2> | |
| <h2>About SkipTo.js</h2> |
It seems keyboard shortcuts have been disabled in this configuration, so the hardcoded “Keyboard Shortcuts” heading here doesn’t seem to apply? How about a more general heading matching the label of the trigger button?
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.
I will get that fixed, thanks for finding it!
shirsha
left a comment
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.
reviewed
|
@howard-e |

Updates include:
WAI Preview Link (Last built on Tue, 28 Oct 2025 12:08:25 GMT).