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.

Sign up for GitHub

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

Jump to bottom

fix(material/stepper): fix nested tabpanel role in stepper #30410

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(material/stepper): fix nested tabpanel role in stepper #30410

wants to merge 4 commits into from
+49 −38

Conversation

essjay05
Copy link
Contributor

@essjay05 essjay05 commented Jan 28, 2025 •
edited
Loading

Updates Angular Components Stepper component to fix the nested aria-role "tabpanel" in the vertical stepper by converting vertical stepper role from "tablist" to "tree" and updating its associated aria attributes.

Before fix screenshot
After fix screenshot

Fixes b/361783174

@jelbourn jelbourn added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Jan 28, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 28, 2025 •
edited
Loading

Deployed dev-app for e862a7f to: https://ng-dev-previews-comp--pr-angular-components-30410-dev-fbsgn7x8.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@essjay05 essjay05 force-pushed the stepper-aria-roles-fix branch from b1e74eb to 49c1419 Compare January 30, 2025 22:27
@essjay05 essjay05 marked this pull request as ready for review January 30, 2025 23:22
@essjay05 essjay05 requested a review from a team as a code owner January 30, 2025 23:22
@essjay05 essjay05 requested review from mmalerba and andrewseguin and removed request for a team January 30, 2025 23:22
@essjay05 essjay05 force-pushed the stepper-aria-roles-fix branch from 3632365 to c71e31b Compare January 31, 2025 16:35
crisbeto
crisbeto reviewed Feb 4, 2025
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Is there a reason why we're changing the entire accessibility pattern, instead of changing the hierarchy of the tab-related roles?

@essjay05
Copy link
Contributor Author

essjay05 commented Feb 5, 2025

Is there a reason why we're changing the entire accessibility pattern, instead of changing the hierarchy of the tab-related roles?

It seems that the vertical stepper seems to match the tree pattern better with its current structure. Otherwise, I believe it would take a larger revamp of the vertical stepper that I would like to request guidance on if that's preferred instead.

1 similar comment
@essjay05
Copy link
Contributor Author

essjay05 commented Feb 5, 2025

Is there a reason why we're changing the entire accessibility pattern, instead of changing the hierarchy of the tab-related roles?

It seems that the vertical stepper seems to match the tree pattern better with its current structure. Otherwise, I believe it would take a larger revamp of the vertical stepper that I would like to request guidance on if that's preferred instead.

@crisbeto
Copy link
Member

crisbeto commented Feb 5, 2025

I see the issue in that we don't have a common container for all the headers in vertical mode so there's no way to set tablist without having the tabpanel be a child, but I'm also hesitant with having to support two different design patterns inside the stepper. A couple of ideas off the top of my head:

  1. Could we get around it with something like aria-owns on the tablist that would point to each of the tabs?
  2. Maybe we can apply a different pattern to both horizontal and vertical? Either the accordion or role="navigation" come to mind.

@essjay05
fix(material/stepper): fix nested tabpanel role in stepper
ca33087 
Updates Angular Components Stepper component to fix the nested
aria-role tabpanel in the vertical stepper.

Fixes b/361783174
@essjay05
fix(material/stepper): changes vertical stepper aria attributes to tr…
f0a73bf 
…ee-related

Updates Angular Components Stepper vertical stepper to use and match correct
aria attributes to match role=tree instead of using tablist/tab/tabpanel since
moving the tabpanel content outside of the mat-step changes the visual look of
the vertical stepper. WIP: update the docs to mention change of roles for vertical
stepper to tree and its appropriate aria-attributes.

Fixes b/361783174
@essjay05
refactor(material/stepper): cleans up unnecessary roles/attributes
ad59f25 
Updates previous fix to Angular Components stepper to remove
unnecessary roles and attributes. Also updates Stepper docs with
latest vertical stepper aria attribute and role changes. Updates
stepper-demo heading level elements for accessibility purposes.
@essjay05
refactor(material/stepper): removes unnecessary aria role
e862a7f 
Cleans up previous changes by removing unnecessary aria role
of group wrapping the step-header and step-content.
@essjay05 essjay05 force-pushed the stepper-aria-roles-fix branch from c71e31b to e862a7f Compare February 14, 2025 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crisbeto crisbeto crisbeto left review comments

@mmalerba mmalerba Awaiting requested review from mmalerba mmalerba is a code owner automatically assigned from angular/components-googlers

@andrewseguin andrewseguin Awaiting requested review from andrewseguin andrewseguin is a code owner automatically assigned from angular/components-googlers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area: material/stepper dev-app preview When applied, previews of the dev-app are deployed to Firebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@essjay05 @crisbeto @jelbourn