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

[WIP]fix(styles): horizontal nav a11y #5087

Closed
wants to merge 1 commit into from
Closed

Conversation

N1XUS
Copy link
Contributor

@N1XUS N1XUS commented Jan 3, 2024

Related Issue

Closes none

Description

  • Updated accessibility attributes for horizontal navigation;
  • Updated two-click area example with correct dropdown arrow styling

@N1XUS N1XUS added the A11y Accessibility concerns or improvements label Jan 3, 2024
@N1XUS N1XUS added this to the Sprint 128 milestone Jan 3, 2024
@N1XUS N1XUS requested a review from InnaAtanasova January 3, 2024 10:19
@N1XUS N1XUS self-assigned this Jan 3, 2024
Copy link

netlify bot commented Jan 3, 2024

Deploy Preview for fundamental-styles ready!

Name Link
🔨 Latest commit b5e8c63
🔍 Latest deploy log https://app.netlify.com/sites/fundamental-styles/deploys/6595349ce561750008129fe2
😎 Deploy Preview https://deploy-preview-5087--fundamental-styles.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@N1XUS N1XUS changed the title fix(styles): horizontal nav a11y [ci visual] fix(styles): horizontal nav a11y Jan 3, 2024
@@ -1,19 +1,18 @@
<div style="height: 500px">
<nav class="fd-navigation fd-navigation--horizontal" aria-label="Horizontal Navigation">
<div class="fd-navigation__container">
<ul class="fd-navigation__list" role="menubar" aria-label="Horizontal Navigation">
<ul class="fd-navigation__list" role="tablist" aria-orientation="horizontal" aria-label="Horizontal Navigation">
Copy link
Contributor

Choose a reason for hiding this comment

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

the default aria-orientation for role="tablist" is horizontal, you don't need to specify it (just FYI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UI5 does

>
<div
class="fd-navigation__item"
aria-current="page"
aria-selected="false"
aria-expanded="false"
role="menuitem"
role="tab"
Copy link
Contributor

Choose a reason for hiding this comment

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

Role tab needs the following:

  • aria-labelledby: this should be leading to the span that wraps the text
  • aria-controls: referring to the tabpanel that the tab controls
  • aria-posinset and aria-setsize
  • aria-selected
  • aria-expanded
  • aria-haspopup: if the tab has a popup menu

Copy link
Contributor

Choose a reason for hiding this comment

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

>
<div
class="fd-navigation__item"
aria-current="page"
aria-selected="false"
aria-expanded="false"
role="menuitem"
role="tab"
>
<a class="fd-navigation__link" role="link" href="#">
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, since we now have Tabs, I am not sure if we need the role link and the href

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, technically it's still a navigation, and link can be applied to it, no?

@droshev
Copy link
Contributor

droshev commented Mar 9, 2024

waiting for the signed off a11y design spec

@droshev droshev changed the title fix(styles): horizontal nav a11y [WIP]fix(styles): horizontal nav a11y Mar 14, 2024
@droshev
Copy link
Contributor

droshev commented Mar 14, 2024

can be reopened once the a11y design spec is finalized

@droshev droshev closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11y Accessibility concerns or improvements blocked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants