fix: improve sidebar accessibility structure (#8429) #11038
Conversation
|
Hi @LassadBoubarma! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
⚡️ Lighthouse report for the deploy preview of this PR
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Thanks
We can consider this change but the PR is currently not in a state I can merge, and includes totally unrelated changes.
I don't see where you replaced <aside>
- Applied
aria-hidden="true"to the logo when not intended to be focusable or read
IMHO it shouldn't always be applied, there are cases where the logo should be focusable
- Cleaned up markup to match accessibility expectations
What do you mean exactly?
#Why this is still useful:
Although NVDA did not read the logo before the fix, the original markup was semantically incorrect:
- The outer
<aside>was unnecessary and could cause duplicate landmark announcements in some screen readers
Which <aside>, which screen readers and what do each of them read?
After your fix, what do these screen readers read?
| )} | ||
| aria-label="docs sidebar"> | ||
| {hideOnScroll && ( | ||
| <Logo tabIndex={-1} className={styles.sidebarLogo} aria-hidden="true" /> |
There was a problem hiding this comment.
From my understanding, the aria-hidden="true" shouldn't always be applied.
When the site navbar is shown, then that logo is behind the navbar so it's hidden:
However when the site gets scrolled and the navbar disappears, this logo becomes visible, and is an actual link to the site homepage. In this case I don't believe applying aria-hidden is a good idea
| )}> | ||
| {hideOnScroll && <Logo tabIndex={-1} className={styles.sidebarLogo} />} | ||
| )} | ||
| aria-label="docs sidebar"> |
There was a problem hiding this comment.
All our Aria labels should use our translation framework so that they can be localized
| // We also clear website/build dir | ||
| // returns void, no useful result needed before compilation | ||
| // See also https://github.com/facebook/docusaurus/pull/11037 | ||
| clearPath(outDir), |
There was a problem hiding this comment.
This is not related to accessibility, please remove all the unwanted changes from this PR diff
There was a problem hiding this comment.
Hi, thanks for your feedback and sorry for the late response !
I will just clarify this points :
About the <aside> replacement:
You're totally right — I didn’t actually replace an <aside> in the current markup. What I meant was that I replaced a generic container (previously a <div> or <aside> depending on the context) with a more semantic <nav aria-label="docs sidebar">. Sorry for the confusion there
Regarding aria-hidden="true" on the logo:
Good catch. I see your point now, applying aria-hidden unconditionally isn't ideal. The logo is indeed visible and interactive when the navbar scrolls away, so hiding it at that point isn’t great for accessibility.
Screen reader testing:
I tested with NVDA on Windows. Before the change, the logo was inconsistently handled, and the outer <aside> (when present) sometimes caused duplicate landmarks to be announced.
Translation of aria-label:
Thanks for pointing that out
Unrelated changes:
You're absolutely right — the change to buildLocale.ts doesn't belong in this PR.
Appreciate the thoughtful review!
There was a problem hiding this comment.
Great
Let me know if you plan to improve the PR to meet the requirements
slorber
added
the
domain: a11y
This PR addresses issue #8429 by improving the accessibility structure of the documentation sidebar.
Changes Made:
<div>/<aside>with a semantic<nav aria-label="docs sidebar">aria-hidden="true"to the logo when not intended to be focusable or read#Why this is still useful:
Although NVDA did not read the logo before the fix, the original markup was semantically incorrect:
<aside>was unnecessary and could cause duplicate landmark announcements in some screen readersaria-hidden, leaving its behaviour inconsistent across assistive techThis change makes the sidebar:
Tested locally with NVDA and DOM inspection.