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

Skipto menu on all pages: Update skipto.js script to version 5.1.6 #2680

Merged
merged 6 commits into from
Jun 26, 2023

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Apr 25, 2023

This update fixes the following issues:

  1. Accessible name computation takes into account Firefox when there are img elements in a heading (H1-H6).
  2. skipto.js will only load once, if the skipto.js is loaded more than once the successive loadings will be ignored and send a warning message to the console.

WAI Preview Link (Last built on Thu, 11 May 2023 14:18:55 GMT).

@alflennik
Copy link
Contributor

Thank you for opening this PR with the correct version of the library - since we decided to move this library to the wai-aria-practices repo I copied the new version over there and the PR is w3c/wai-aria-practices#217. If that makes sense could you close this PR?

@alflennik
Copy link
Contributor

Here's an update: I was talking to @mcking65 over in #2682, and he said that we shouldn't remove skipto from the aria-practices library. I updated my open PR and it includes the updated 5.1.5 version of skipto.

@jongund
Copy link
Contributor Author

jongund commented May 5, 2023

What ever works for the templates, but please use version 5.1.6 for all templates, since it will correctly identify if skipto.js is being loaded more than once.

@jongund jongund changed the title updated skipto.js script to 5.1.5 updated skipto.js script to 5.1.6 May 9, 2023
@jongund
Copy link
Contributor Author

jongund commented May 9, 2023

@alflennik
I updated SkipTo to version 5.1.6.
5.1.6 fixes a rare bug in computing landmarks when there are no headings in the main content.

jongund added 2 commits May 10, 2023 08:26
Updated default configuration for APG be changing `attachElement` to `div` and default `colorTheme` to `aria`.
@jongund
Copy link
Contributor Author

jongund commented May 11, 2023

@alflennik
Could we add the following configuration script to the template?
The version I am butting in the pull request changes the default values of the following three options based on the preferences for the WAI/APG website.
If we can add this configuration script to the template any updates to SkipTo can be more easily applied to the website, by a simple update of the unmodified script.

<script>
var SkipToConfig =  {
  displayOption: 'popup',
  containerElement: 'div',
  colorTheme: 'aria'
};
</script>

More information on configuration options:
https://skipto-landmarks-headings.github.io/page-script-5/

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed updated skipto.js script to 5.1.6.

The full IRC log of that discussion <jugglinmike> subtopic: updated skipto.js script to 5.1.6
<jugglinmike> github: https://github.com//pull/2680
<jugglinmike> Matt_King: Is there another way to do this without requiring all the HTML source to be edited? Specifically, a way that would be straightforward to anyone using skipTo
<jugglinmike> jugglinmike: the RequireJS library was configurable via the HTML script tag which included it. We could configure skipTo with a "data-" attribute, if skipTo were updated to recognize such an attribute
<jugglinmike> Matt_King: I think we want to merge gh-2680 and then get alignment on whether or not the HTML files in ARIA-Practices with have a script tag for skipto.js or not (and if not, where the injection will be done). From there, we can simplifying this aspect of configuration

@jongund
Copy link
Contributor Author

jongund commented May 31, 2023

@alflennik @mcking65
I have added a feature to skipto.js to configue it using the data-skipto attribute on the script tag.
So if you are interested you could download version 5.2 and use the following code to configure skipto for the APG website:

<script 
  data-skipto="displayOption: popup; containerElement: div; colorTheme: aria"
  src="WAI/content-assets/wai-aria-practices/shared/js/skipto.js">
</script>

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed updated skipto.js script to 5.1.6.

The full IRC log of that discussion <jugglinmike> Subtopic: updated skipto.js script to 5.1.6
<jugglinmike> github: https://github.com//pull/2680
<jugglinmike> Matt_King: I know there's a version 5.2, so I'm wondering if we should merge this as-is. Maybe at a later time, we could upgrade again
<jugglinmike> jongund: This pull request should work according to the needs of the WAI site
<jugglinmike> Matt_King: Okay, I'm going to go forward with that, and we can add this to the list of changes for publication
<jugglinmike> Matt_King: When you're ready jongund, we can upgrade to the parameterized version of skipTo.js. That will simplify maintenance going forward

@mcking65
Copy link
Contributor

mcking65 commented Jun 10, 2023

@charmarkk @ariellalgilmore @andreancardona @curtbellew @shirsha

Are any of you available to do a quick review/test to ensure there are no regressions or new issues in this version of the skipto menu?

This is the preview link to test.

I am specifically looking for test of visual appearance, keyboard, and mouse behavior in:

  • Chrome, Edge, and Firefox on Windows
  • Chrome and Safari on macOS

I have tested with screen readers.

@mcking65 mcking65 changed the title updated skipto.js script to 5.1.6 Skipto menu on all pages: Update skipto.js script to version 5.1.6 Jun 10, 2023
@mcking65 mcking65 added Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation javascript Pull requests that update Javascript code labels Jun 10, 2023
@mcking65 mcking65 added this to the H1/2023 Content Updates milestone Jun 10, 2023
@mcking65 mcking65 requested a review from ariellalgilmore June 13, 2023 18:13
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Skipto menu on all pages: Update skipto.js script to version 5.1.6 by jongund · Pull Request #2680 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> subtopic: Skipto menu on all pages: Update skipto.js script to version 5.1.6 by jongund · Pull Request #2680 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2680
<jugglinmike> Matt_King: I tested this with screen readers to make sure there have been no regressions, but I'm looking for help to verify that there have been no visual regressions
<jugglinmike> Matt_King: the deadline for review will probably be at least two weeks
<jugglinmike> arigilmore: I can take a look
<jugglinmike> Matt_King: Thanks! I'll assign you

Copy link
Contributor

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

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

LGTM visually on browsers!

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Skipto menu on all pages: Update skipto.js script to version 5.1.6.

The full IRC log of that discussion <jugglinmike> Subtopic: Skipto menu on all pages: Update skipto.js script to version 5.1.6
<jugglinmike> github: https://github.com//pull/2680
<jugglinmike> Matt_King: This was awaiting review
<jugglinmike> Matt_King: ariellalgilmore has approved the visual appearance. That's what I was looking for. Maybe this is ready to go

@mcking65 mcking65 merged commit b2718f4 into main Jun 26, 2023
@mcking65 mcking65 deleted the bugfix/skipto branch June 26, 2023 02:21
@mcking65 mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy and removed Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation labels Jun 30, 2023
@a11ydoer
Copy link
Contributor

Overall it looks good to me. One thing I am curious about is that there is no focus indicator only for complementary landmark. @jongund, Is this perhaps the intended design?

@jongund
Copy link
Contributor Author

jongund commented Jul 11, 2023

@a11ydoer
SkipTo looks for the first focusable element (e.g. link or form control) in the landmark and sets focus to the element. If there is no link or form control, it will look to a header (h1-h6) and set focus to it. It does this by temporally giving a tabindex=-1 and the using .focus method to set focus to the header. If no header is found it will just set focus to the container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skipto needs to be updated to version 5.1.4
6 participants