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

UI: Add skip to canvas/sidebar links #15740

Merged
merged 22 commits into from Aug 20, 2021
Merged

UI: Add skip to canvas/sidebar links #15740

merged 22 commits into from Aug 20, 2021

Conversation

darleendenno
Copy link
Contributor

@darleendenno darleendenno commented Aug 3, 2021

Issue: #4667

It is currently quite difficult to test the screen reader accessibility of a component, as you need to navigate past all of the navigation links and then step into the iframe to interact with the component.

What I did

Added three separate links to improve keyboard navigation around Storybook. All links are only visible on keyboard focus.

  1. sidebar/Heading - this link should be the first tabbed content after the address bar; takes the user to the canvas
    • now accepts a skipLinkHref prop to determine when a skip link should render
  2. sidebar/Tree - this link is visible when a story is selected and focused; takes the user to the canvas
  3. preview/FramesRenderer - this link takes the user back to the sidebar

I also added the option to override theme on a per-story basis via global/parameters

Screenshots

Only using keyboard (TAB and then ENTER) to navigate between the sidebar and iframe
skip-to-content-link

What I did not do

  • Improve the entirety of keyboard navigation for Storybook. This is just a small improvement and should stay as such.
  • Make this available for mobile navigation -- that's an entirely different mountain to climb

How to test

  • Open the Storybook available on this PR (see the status check links at the bottom)
  • Use a screen reader and attempt to navigate to the iframe; is this easier than the baseline?
  • Use a keyboard to navigate between stories, the sidebar, and the canvas; is this easier than the baseline?
  • Check out Tree & Heading stories (additional play functionality added 🧪 )

  • Is this testable with Jest or Chromatic screenshots? Yes, additional stories added
  • Does this need a new example in the kitchen sink apps? No
  • Does this need an update to the documentation? No

Questions from the authors (seeking a11y community members for comment)

  • How can this be improved upon?
  • Is there any desire to improve the navigation relationship between sidebar/canvas and the addon panel?

Shoutouts

Thank you to @smithambera, @yannbf, @MichaelArestad, and @ndelangen for your wise collaboration ❤️

@nx-cloud
Copy link

nx-cloud bot commented Aug 3, 2021

Nx Cloud Report

CI ran the following commands for commit 5455df4. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented Aug 3, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit 349eb16.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@darleendenno darleendenno marked this pull request as draft August 3, 2021 04:20
@darleendenno darleendenno marked this pull request as ready for review August 3, 2021 04:28
@darleendenno darleendenno self-assigned this Aug 3, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looking great @darleendenno -- there are a couple rendering errors in Chromatic, can you take a look?

@darleendenno
Copy link
Contributor Author

Looking great @darleendenno -- there are a couple rendering errors in Chromatic, can you take a look?

@shilman both of these errors are in components using the new play functionality -- does chromatic support those interactions?

@shilman
Copy link
Member

shilman commented Aug 3, 2021

@darleendenno Chromatic does not officially support play, but in practice it should work. Something funny is going on here and there are two instances of data-testid="heading--skip" on the page so TestingLib is throwing an error. I'm not sure why yet, but will see if I can repro on my local machine

@darleendenno
Copy link
Contributor Author

@darleendenno Chromatic does not officially support play, but in practice it should work. Something funny is going on here and there are two instances of data-testid="heading--skip" on the page so TestingLib is throwing an error. I'm not sure why yet, but will see if I can repro on my local machine

I see now -- I'll fix it up today. May need a delay or some syntax refactoring. Thanks for catching!

@MichaelArestad
Copy link
Contributor

Looking good! Thank you for making this happen!

Almost all the buttons worked as expected. The exception is the one in the canvas:
image

I can't get it to work. I tried return, space, and clicking with no luck.

I also noticed that I can trigger that button twice. Try shift + tab and tab around it.

skip-link-test-1

FF latest. Mac latest.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Great job--looking fantastic! 💯

@darleendenno
Copy link
Contributor Author

Great job--looking fantastic! 💯

Thank you!! I have one more thing to fix up with theming per @domyen but I am hoping to have it ready early next week 🙏

@yannbf
Copy link
Member

yannbf commented Aug 9, 2021

Super nice @darleendenno! Works great both in normal keyboard navigation as well as with Voice Over 👌

@darleendenno
Copy link
Contributor Author

Ok this is ready for re-review & merging. Let me know what you think @ghengeveld ❤️

@MichaelArestad
Copy link
Contributor

I'm having trouble getting the skip to sidebar button to work with keyboard or by clicking on FF Mac latest. Any ideas?

@darleendenno
Copy link
Contributor Author

I'm having trouble getting the skip to sidebar button to work with keyboard or by clicking on FF Mac latest. Any ideas?

I’ve been having issues on initial render as well. Refresh and try again. May have to file an issue or get some help figuring out why that’s happening.

Copy link
Contributor

@MichaelArestad MichaelArestad left a comment

Choose a reason for hiding this comment

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

IT WORKS! :shipit: Nice work. I had only one minor suggestion. Thanks for making this happen.

lib/ui/src/components/sidebar/Heading.tsx Outdated Show resolved Hide resolved
@shilman shilman merged commit 8f9c293 into next Aug 20, 2021
@shilman shilman deleted the feat/add-skip-to-content branch August 20, 2021 16:50
@shilman shilman added this to the 6.4 PRs milestone Aug 23, 2021
inkblotty added a commit to primer/view_components that referenced this pull request Nov 10, 2021
…in navigation (#884)

Context
Version 6.4 of storybook comes with some improvements in navigation, including "Skip to Canvas" and "Skip to Sidebar" navigation links, enabling keyboard users to skip over the sidebar content.

This change was introduced in this PR (storybookjs/storybook#15740) around Skip to X links

Resolves github/accessibility#379

Notes
6.4 is still in beta, so this will likely need another bump once 6.4 is fully published. However, we want to bump this version for "Skip to" links to enable our auditors who rely on keyboard to navigate our Primer components more easily.
@callamwilliams
Copy link

callamwilliams commented Apr 11, 2022

How can I disable this? For me, it just appears as a 'skip to canvas' button that when clicked moves down 200px and does nothing

Edit:

// manager-head.html
<style>
    .sidebar-header > a {
        display: none;
    }
</style>

@MichaelArestad
Copy link
Contributor

@callamwilliams
Can you share an example of this happening? I would love to get it fixed.

@adamnel
Copy link

adamnel commented Apr 15, 2022

Here's an example of it happening for us https://covalent.teradata.lol

@MichaelArestad
Copy link
Contributor

@adamnel It looks like one of your custom styles made this button visible all the time:
2022-04-18 14 17 40

By default, it will not appear unless using keyboard navigation:
2022-04-18 14 16 40

And both actually do send focus to the canvas. There are not always interactive elements in the canvas, but they do work as expected for folks using keyboard nav.

@kharrop
Copy link

kharrop commented Apr 24, 2022

+1 I'm seeing the above issue where the "Skip to canvas" button is displaying by default after upgrading to the latest version.

@yairEO
Copy link
Contributor

yairEO commented Dec 1, 2022

I have seen this for months also, now I'm on Storybook version 6.5.13 with a custom brand logo set in manager.js

const theme = create({
    base: 'light',
    brandTitle: 'CatalogONE',
    brandImage: 'https://www.amdocs.com/sites/default/files/2022-03/Amdocs-Catalog.png',
})

addons.setConfig({
  isFullscreen: false,
  showNav: true,
  showPanel: true,
  panelPosition: 'bottom',
  enableShortcuts: false,
  isToolshown: true,
  theme,
  selectedPanel: undefined,
  initialActive: 'sidebar',
  sidebar: {
    showRoots: false,
    collapsedRoots: ['other'],
  },
  toolbar: {
    title: { hidden: false, },
    zoom: { hidden: false, },
    eject: { hidden: false, },
    copy: { hidden: false, },
    fullscreen: { hidden: false, },
  },
});

It's especially visible when the sidebar is expanded:

sb-jump-to-canvas-button-always-visible.mp4

image


Had to resort fixing it in .storybook\manager-head.html:

<style>
/* hide annoying "skip to canvas" button which is rendered over the logo (brand) image  */
.sidebar-header > a {
    display: none;
}
</style>

@yamanoku yamanoku mentioned this pull request Feb 9, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants