fix(aside): add spread prop, update docs with accessibility labe…

[jendowns]

Closes #566

Currently the Aside component renders an <aside> element that is missing required accessibility labels. This <aside> element needs either an aria-label attribute (to provide a label) or aria-labelledby attribute (to use an existing element, like a heading, as a label) per Checkpoint 2.4.1

This PR proposes adding ariaLabel and ariaLabelledBy props so that users can provide either of these required accessibility labels, depending on their context/situation updates examples and documentation to include aria-label and aria-labelledBy

If one of these accessibility labels are NOT provided, then the Aside component will render a <div> element inside of an <aside>.

Changelog

New

• add ariaLabel and ariaLabelledBy props with documentation

Changed

• update docs
• add temporary style override to fix [Notification]: low-contrast styles block inline notification's pointer events carbon#4804

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/carbon-design-system/gatsby-theme-carbon/jywbjd77e
🌍 Preview: https://gatsby-theme-carbon-git-fork-jendowns-566aside-label.carbon-design-system.now.sh

[jendowns]

@dakahn I was wondering if you could take a look at this when you get a chance and let me know if you agree with my assumptions + my approach here? Thank you! 🙏

[vpicone]

a

[vpicone]

@jendowns interested to hear what @dakahn thinks, but I think this might be an issue with DAP.

under ARIA13: Using aria-labelledby to name regions and landmarks I'd argue that the aside element falls under the category of landmarks not needing a label:

• No label is required for landmark regions where the purpose is obvious from the role (e.g., main, banner, search, or contentinfo).

[jendowns]

@vpicone Here's some more info I found in my research, outside of DAP:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Complementary_role#Labeling_landmarks

If there is more than one complementary landmark role or <aside> element in a document, provide a label for each landmark using the aria-label attribute, or, if the aside has an appropriately descriptive title, point to it with the aria-labelledby attribute. This label will allow an assitive technology user to be able to quickly understand the purpose of each landmark.

It's possible that DAP flags all aside regardless of whether there is more than one on a page, but generally I think this would be a useful thing to consider since there is a high probability of more than one being on a page? (In our case, we have 2 per page in most circumstances that come to mind)

[vpicone]

@jendowns Oooooo you're super right. In hindsight the components I listed generally usually show up once a page where that's definitely not the case here.

What do you think about just passing all of the props through without the check? I'd like to be more consistant with this but in theory they should be able to just treat it like a regular aside element. We could add an InlineNotification to the docs with a link to the MDN guidance on labeling for multiple asides.

As is, we'd be retroactively converting everyone's aside to a div since they weren't passing in labels before. So even if there's only one on the page (presumably valid a11y wise), it'd get turned into a div which is pretty rough. If we pass the props through, the DAP violation is on the site developer and with this change they'd be empowered to fix it.

[dakahn]

@vpicone I like that --- sounds good. Thanks for doing the research leg work here @jendowns

[jendowns]

Thank you @dakahn & @vpicone!

Just made the following changes to the PR --

• removed the div logic and always render as an aside
• added an InlineNotification to the Aside doc page linking to a11y info etc.
• went ahead and added ...rest spread for extra props

However in the process I noticed that links inside low-contrast InlineNotifications are broken. I opened a PR here to fix it in Carbon core: carbon-design-system/carbon#4804

And in the meantime, I added an override in this library to fix the issue until the fix is released & absorbed here:

// TODO: remove when fix is released. See https://github.com/carbon-design-system/carbon/issues/4804
.bx--inline-notification--low-contrast:before {
  pointer-events: none !important;
}

Let me know what you think!

[dakahn]

Very cool! 🏄 Thanks again @jendowns

[vpicone]

Since we're not using a using a div, we don't have to de-structure these or switch the case, we can just pass them through right?

That way devs use them just like they would on a native aside jsx element.

[vpicone]

Thanks a bunch for the docs, looks great.

Can we just pass these on through and update the examples to use the normal jsx aria props?

[jendowns]

@vpicone I'm reluctant to do that because with the props explicitly defined in this way, their usage is more normalized. They are elevated to be a more vital part of the component structure.

If they aren't called out in this way--as recommended & documented props--then we are kinda back to square one where the accessibility of this component is dependent upon the knowledge of the user / or the user's commitment to checking with DAP. 🤔 (Admittedly the added documentation to the Aside page is super helpful but seems like the defined props would also help?)

Plus I'm thinking of other examples within Carbon core (specifically the React library) where aria props are explicitly defined even if they aren't necessarily required in a specific situation.

What do you think?

[vpicone]

@jendowns I think the react library opting for non-standard, camel-case props was a mistake. I don’t think developers should need to memorize/track which components use native/standard aria capitalization and which ones get morphed by Carbon.

What do you think about leaving them in as prop-types/in the prop table. Like you said they’re pretty critical here. But I think we should use the native capitalization. They also shouldn’t be required as their only required when multiple exist on a page and there’s not a great way for us to make that assertion at run time.

[jendowns]

Thanks @vpicone! I just made the changes you suggested above. Let me know what you think 🎉

[vpicone]

@jendowns brilliant, thanks!