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

JumpTo Main does not work on Home Page #135

Closed
mcking65 opened this issue May 19, 2022 · 5 comments · Fixed by #131
Closed

JumpTo Main does not work on Home Page #135

mcking65 opened this issue May 19, 2022 · 5 comments · Fixed by #131
Assignees
Labels
bug Something isn't working Infrastructure

Comments

@mcking65
Copy link
Contributor

In the preview of PR 99 where the APG JumpTo menu is implemented, jump to main content function does not work on the APG Home page in the /WAI/ARIA/APG site.

Jump to main landmark works on all other pages in the site. Jump to other landmarks and headings works on the home page. It is only jumping to the main content landmark on the home page that does not work.

@mcking65 mcking65 added bug Something isn't working Infrastructure labels May 19, 2022
@jongund
Copy link
Contributor

jongund commented May 19, 2022

@mcking65
It looks like there is a hidden H1 on the home page.
JumpTo looks for the first child heading in main to set focus, JumpTo is assuming that the first heading in a main element is visible.
What is the purpose of the hidden H1?

Jon

@mcking65
Copy link
Contributor Author

I'll let @alflennik or @richnoah explain the hidden h1; I don't know.

However, I'm thinking about the robustness of the jump to main algorithm. It doesn't seem very robust to assume that main starts with a heading. Sometimes there is an H1 outside of main and a fair bit of content that precedes the first heading in main. While not ideal design, that does happen on some web sites. Would jump To main not work on those sites? Would it jump to a heading that is far from the beginning of main?

I like the experience when main starts with a heading an jumpTo focuses that heading; it works very nicely. I am concerned about situations where that may not be the best behavior. In some situations, it could be better to focus a div containing text, a paragraph, or some other element. I wonder if it would be more reliable if focus were set on the main element itself in cases that are ambiguous.

At a minimum, the algorithm should probably check to ensure the element it will focus is visible.

@mcking65
Copy link
Contributor Author

@jongund

Are you able to look into this? What are your thoughts on my previous comment?

@jongund
Copy link
Contributor

jongund commented Jun 15, 2022

Here is a an update to the SkipTo.js script that should fix the problem of trying to move focus to a hidden header.

https://github.com/paypal/skipto/tree/dev/v4.2/downloads/js

As per the conversation today, the APG will just switch from using "JumpTo" to using "SkipTo" to eliminate the need to maintain two separate code repositories and make it easier for other people to add the "SkipTo" functionality to their website.

@mcking65 mcking65 transferred this issue from w3c/aria-practices Jun 19, 2022
@mcking65
Copy link
Contributor Author

This is fixed by #125 by having the hidden H1 removed. However, #131 also ensures that the skipto function only focuses visible elements.

@mcking65 mcking65 linked a pull request Jun 20, 2022 that will close this issue
mcking65 added a commit that referenced this issue Jun 23, 2022
* Correct copyright and license to accurately attribute authorship and source
* Fix #133: skip to menu label incorrect on Mac
* Fix #134: can't scroll to bottom of skip to menu if not fully visible in viewport
* Fix #135: skip to main does not work on Home Page by ensuring target of skip is visible.

Co-authored-by: Matt King <a11yThinker@gmail.com>
Co-authored-by: Howard Edwards <howarde.edwards@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants