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

fix(theme-classic): fix SkipToContent without JS , refactor, make it public theming API #8204

Merged
merged 7 commits into from Oct 19, 2022

Conversation

mturoci
Copy link
Contributor

@mturoci mturoci commented Oct 11, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Allow skipping to content despite disallowed JS.

Test Plan

Manual.

Test links

Deploy preview: https://deploy-preview-8204--docusaurus-2.netlify.app/

Related issues/PRs

Closes #6411

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 11, 2022
@Josh-Cena Josh-Cena changed the title fix(theme-classic): Use native href for skip to content #6411. fix(theme-classic): use native href for skip to content Oct 11, 2022
@netlify
Copy link

netlify bot commented Oct 11, 2022

[V2]

Name Link
🔨 Latest commit 4c51eba
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63501e519c44d20008fbeae5
😎 Deploy Preview https://deploy-preview-8204--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Oct 11, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 78 🟢 98 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 78 🟢 100 🟢 100 🟢 100 🟢 90 Report

@slorber
Copy link
Collaborator

slorber commented Oct 12, 2022

Thanks, that looks like a good solution

Shall the existing JS focus be dumped entirely in favor of a simple anchor? (IMHO using native href is always preferred compared to JS so this is a good direction).

Note I don't know exactly why we need this JS code. I didn't add this code but it looks most of it was added here: #4162

    const targetElement: HTMLElement | null =
      document.querySelector('main:first-of-type') ??
      document.querySelector(`.${ThemeClassNames.wrapper.main}`);

The main:first-of-type looks like a sensible JS fallback in case third-party theme authors do not add the required id?

Maybe we could check if the id target exists in the DOM, and only execute the sensible JS fallback + preventDefaults in case it doesn't? What if the user implements a custom page, not applying our layout component


Regarding this code:

  useLocationChange(({location}) => {
    if (containerRef.current && !location.hash && action === 'PUSH') {
      programmaticFocus(containerRef.current);
    }
  });

I don't know for sure what's the best a11y behavior here, but to me the goal is to "reset" the focus so that the first tabbed items becomes the "skipToContent" link after a SPA navigation.

If you don't do this, then when navigating from the docs sidebar, the clicked doc sidebar item would stay focused

And browsing the main content on the new page may become more complicated?

CleanShot 2022-10-12 at 19 55 46

@lex111 @locriacyber @Mythra @JoshuaKGoldberg @BenDMyers if you have any opinion on how skip to content is supposed to be integrated into a SPA vs MPA, now is the best time :)

@slorber slorber added the domain: a11y Related to accessibility concerns of the default theme label Oct 12, 2022
@Mythra
Copy link

Mythra commented Oct 12, 2022

Hi, happy to be pinged in a11y stuff again.

First off I can't overstate how much I love the change of just using a default anchor link. It's something I wanted originally (and mentioned in one of the first prs/issues), but wasn't chosen at the time because there were some worries about themes potentially not providing the right ID. So getting us closer to that is awesome.

As for "how skip to content link should work", I'll point to the WCAG guidelines: https://www.w3.org/TR/WCAG20-TECHS/G1.html these are what most people have to comply too, and does a great job of covering universally disabilities that may need to use this behavior. The first tabbed item should always be the skip to content link when on what is a new "page".

So if you're in an SPA app where moving to a new page with the same navigation elements around that a user should have to tab through, then yes focus should first go to skip to content link. If it's within a page fragment where the focus is already where it needs to be past all the navigation? No need to reset all the way back up. At least that's what I'd expect as someone who uses a screen reader daily (and my reading of the spec!), but there may be others who have separate opinions.

@slorber
Copy link
Collaborator

slorber commented Oct 13, 2022

Thanks @Mythra , I understand better

The first tabbed item should always be the skip to content link when on what is a new "page".

For a SPA, the concept of a "new page" is kind of ambiguous. We can have nested layouts where navigating only change a nested container of the main layout, but not the "shell" around it. Is this considered as a "new page" when this red square change or not?

CleanShot 2022-10-13 at 11 43 53@2x

So if you're in an SPA app where moving to a new page with the same navigation elements around that a user should have to tab through, then yes focus should first go to skip to content link.

My understanding is that we are in this case here, so it makes sense to use JS to give focus back to the skip to content link when navigating any sidebar item.

The good news is that using JS doesn't seem to be a problem:

  • JS disabled: the focus will be reset natively (due to non-SPA navigation) so a single anchor link will work like in any other MPA
  • JS enabled: our current code to give the focus will work to reset it

If it's within a page fragment where the focus is already where it needs to be past all the navigation? No need to reset all the way back up.

Not sure to understand in what case this happens. Maybe a system where you can tab through items on a page, and tabs are persisted in the url/querystring? Technically it is a navigation (push or replace) but we probably don't want to reset focus in such case?

@mturoci mturoci requested review from slorber and removed request for Josh-Cena October 13, 2022 15:54
@mturoci
Copy link
Contributor Author

mturoci commented Oct 18, 2022

If it's within a page fragment where the focus is already where it needs to be past all the navigation? No need to reset all the way back up.

@Mythra I am also not sure what is meant here. Is for example tabs navigation a good example for this?

@slorber I addressed your review comments, please let me know if there is anything else I can do to get this merged. Thanks!

# Conflicts:
#	packages/docusaurus-theme-common/src/index.ts
@slorber
Copy link
Collaborator

slorber commented Oct 19, 2022

Thanks @mturoci, I did some additional refactors needed for sharing this behavior across themes, and also added some useful comments so that we still understand how it works later 😅

Otherwise the solution looks good enough 👍

@slorber slorber added pr: bug fix This PR fixes a bug in a past release. to backport This PR is planned to be backported to a stable version of Docusaurus labels Oct 19, 2022
@slorber slorber changed the title fix(theme-classic): use native href for skip to content fix(theme-classic): fix SkipToContent without JS , refactor, make it public API Oct 19, 2022
@slorber slorber changed the title fix(theme-classic): fix SkipToContent without JS , refactor, make it public API fix(theme-classic): fix SkipToContent without JS , refactor, make it public theming API Oct 19, 2022
@slorber slorber merged commit aa4fa66 into facebook:main Oct 19, 2022
slorber pushed a commit that referenced this pull request Oct 28, 2022
…public theming API (#8204)

Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
# Conflicts:
#	packages/docusaurus-theme-common/src/index.ts
@slorber slorber added the backported This PR has been backported to a stable version of Docusaurus label Nov 2, 2022
@slorber slorber removed the to backport This PR is planned to be backported to a stable version of Docusaurus label Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA domain: a11y Related to accessibility concerns of the default theme pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a11y: jump to main content doesn't work without JS
4 participants