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

The SkipToMainContent is being assigned to the Layout wrapper by default #9086

Closed
7 tasks done
kemilbeltre opened this issue Jun 20, 2023 · 4 comments
Closed
7 tasks done
Labels
bug An error in the Docusaurus core causing instability or issues with its execution domain: a11y Related to accessibility concerns of the default theme

Comments

@kemilbeltre
Copy link

kemilbeltre commented Jun 20, 2023

Have you read the Contributing Guidelines on issues?

Prerequisites

  • I'm using the latest version of Docusaurus.
  • I have tried the npm run clear or yarn clear command.
  • I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
  • I have tried creating a repro with https://new.docusaurus.io.
  • I have read the console error message carefully (if applicable).

Description

SkipMainContent shouldn't be assigned by default because the Layout wrapper is always considering that the content is the main content, not the whole website (header, main, footer). The problem with that is we can't customize where we want the user to go when hitting SkipToMain content.

Example:

image

Here I'm not considering the main content as the first section, and we go to a section that is not the main.

Reproducible demo

No response

Steps to reproduce

  1. Go to https://typescript-eslint.io/
  2. Skip to the main content

Expected behavior

The SkipToContentFallbackId should be added in the main section, not in the Layout wrapper.

Example: https://benmyers.dev/blog/skip-links/

Actual behavior

The SkipToContent is added to the website Layout wrapper, not to the main section.

Your environment

  • Public source code:
  • Public site URL:
  • Docusaurus version used:
  • Environment name and version (e.g. Chrome 89, Node.js 16.4):
  • Operating system and version (e.g. Ubuntu 20.04.2 LTS):

Self-service

  • I'd be willing to fix this bug myself.
@kemilbeltre kemilbeltre added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Jun 20, 2023
@Josh-Cena
Copy link
Collaborator

In #6411 (comment), @slorber said that "focus main in priority" is intended, but I'm not sure if "no customizability at all" is desirable. Leaving it to @slorber to decide.

@Josh-Cena Josh-Cena added status: needs more information There is not enough information to take action on the issue. and removed status: needs triage This issue has not been triaged by maintainers labels Jun 20, 2023
@slorber
Copy link
Collaborator

slorber commented Jun 29, 2023

The SkipToContentFallbackId should be added in the main section, not in the Layout wrapper.

Not possible: the main section does not always exist so we need a fallback in this case, because many users will not care much about semantic html.

This is code that you will eventually find in the wild, and we want the skip to main content feature to keep working on it:

<Layout>
  <div className="title">My custom page</div>
  <div className="main">...</div>
</Layout>

Also we want a reasonable skip to main content target by default in case JS is disabled: an anchor that is guaranteed to always be present can only be added in the <Layout>.

Unfortunately, we can't have multiple ids so it's hard to make it work best in all possible situations (js, nojs, main, no-main...).


The problem with that is we can't customize where we want the user to go when hitting SkipToMain content.

The current code:

export const SkipToContentFallbackId = '__docusaurus_skipToContent_fallback';

/**
 * Returns the skip to content element to focus when the link is clicked.
 */
function getSkipToContentTarget(): HTMLElement | null {
  return (
    // Try to focus the <main> in priority
    // Note: this will only work if JS is enabled
    // See https://github.com/facebook/docusaurus/issues/6411#issuecomment-1284136069
    document.querySelector('main:first-of-type') ??
    // Then try to focus the fallback element (usually the Layout children)
    document.getElementById(SkipToContentFallbackId)
  );
}

We can make things more flexible and allow you to skip to something you decide that is not main.
This would only work with JS enabled though, hope it's not a problem.

How can you communicate your custom skip to content target to Docusaurus? Should we expert a constant id?

export const SkipToContentTargetId = '__docusaurus_skipToContentTarget';

Here I'm not considering the main content as the first section, and we go to a section that is not the main.

Before we rush on anything: can you explain why you want to do that?

Please give a concrete example of your markup and explain why you don't want to target <main>.

Can you back up your use case with reference material that proves that what you try to do is not a semantic html and accessibility antipattern?

Once we have your example, we can ping some accessibility experts/users and ask them what they think.

@slorber slorber added the domain: a11y Related to accessibility concerns of the default theme label Jun 29, 2023
@kemilbeltre
Copy link
Author

I don't have a specific argument for considering this a good practice or not, but I think it is possible to have multiple "main content" or a different section before main and we could consider that section as the "main content" instead of the default.

I think it's better to have something flexible than to limitate users to use it or not.

@slorber
Copy link
Collaborator

slorber commented Jul 20, 2023

I think it's better to have something flexible than to limitate users to use it or not.

I agree, with one exception: when the flexibility we give can lead users to antipatterns.

If we don't know it's a good practice (or at least not a bad practice) to have multiple main contents, then we are not sure that it's a good idea to support this use-case IMHO, that's why I'm asking you to justify why you need


but I think it is possible to have multiple "main content"

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main

A document mustn't have more than one main element

https://www.w3schools.com/tags/tag_main.asp

Note: There must not be more than one

element in a document. The element must NOT be a descendant of an <article>, <aside>, <footer>, <header>, or <nav> element.


or a different section before main and we could consider that section as the "main content"

Why would anyone respecting semantic html want to do that? If it's the main content, then why can't it be inside a <main> tag?


It looks to me that what we do is right: we should encourage users to have at most one main element, and that's the element the skip to content button should lead to if present.

Giving the flexibility to do bad things should have a really strong use case.

Going to close for now until a strong use-case is provided.

@slorber slorber closed this as not planned Won't fix, can't repro, duplicate, stale Jul 20, 2023
@slorber slorber removed the status: needs more information There is not enough information to take action on the issue. label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution domain: a11y Related to accessibility concerns of the default theme
Projects
None yet
Development

No branches or pull requests

3 participants