Fix pagelayout stories axe violations

[radglob]

Adds tabIndex, role="region" and requires either an aria-label or aria-labelledby on the PageLayout.Pane component when overflow is detected. Overflow is determined by checking if the scrollHeight of the pane is greater than the clientHeight.

Closes https://github.com/github/primer/issues/1865.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: bd0e547

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	94.79 KB (+0.21% 🔺)
dist/browser.umd.js	95.38 KB (+0.17% 🔺)

[joshblack]

Thanks for putting this up @radglob! Just wanted to double-check on requirements for this, I believe that for this we would need the following:

• The PageLayout.Pane component needs to be aware of overflow
• When overflow is detected, the PageLayout.Pane component will:
  • Add the tabIndex attribute¹
  • Add role="region"¹
  • Add aria-label with an accessible label or aria-labelledby to an appropriate element¹

A big challenge for this component seems to be getting that aria-label or aria-labelledby to be a valid that's relevant since it only accepts children.

Is that you're understanding as well for this issue @ericwbailey @TylerJDev? Or is there anything missing?

Let me know if that makes sense @radglob or if I'm overthinking this 🤔

Footnotes

1. https://www.tpgi.com/short-note-on-improving-usability-of-scrollable-regions/) ↩ ↩² ↩³

[TylerJDev]

@joshblack - Yup, that is my understanding as well. I'm guessing we'll need to make the accessible name a required prop for pane? If so, we might need to emphasize on why that's needed so that developers give it a good name.

@radglob - Let me know if you want to pair on this issue! Josh listed all the items we'd need to make sure this is accessible, but if you have any questions on the accessible bits let me know! 😁

[joshblack]

Looks great! I think only big thing is to make sure that one story gets the aria-label to stop it from error'ing out. I left a couple other comments too, let me know what you think!

From a package release perspective, how do you feel about emitting a warning instead of an error for the missing aria-label or aria-labelledby? This could prompt people to add the label without potentially causing existing code to fail. Just wanted to get your thoughts on this 👀

[radglob]

Looks great! I think only big thing is to make sure that one story gets the aria-label to stop it from error'ing out. I left a couple other comments too, let me know what you think!

From a package release perspective, how do you feel about emitting a warning instead of an error for the missing aria-label or aria-labelledby? This could prompt people to add the label without potentially causing existing code to fail. Just wanted to get your thoughts on this 👀

I think it makes sense for this to be an error. Since it's an accessibility issue, it might be best to enforce that. Warnings tend to get ignored in my experience.

[joshblack]

Looks great!

[joshblack]

@radglob just a heads up, removed this from the queue as I'm going through the release right now. Will make sure to add it back when #3021 is merged!