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

Refactor MatTabNav as ARIA tabs with a new MatTabNavPanel component and [tabPanel] input. #24062

Merged
merged 1 commit into from Jan 7, 2022

Conversation

zelliott
Copy link
Collaborator

@zelliott zelliott commented Dec 6, 2021

Fixes #23706.

  • Adds a new MatTabNavPanel component that is intended to be used with MatTabNav. MatTabNavPanel renders the tabpanel, MatTabNav renders the tablist.
  • Adds a new optional [tabPanel] input to MatTabNav that accepts a MatTabNavPanel. Allows MatTabNav to make changes to MatTabNavPanel (i.e. aria-labelledby).
  • If [tabPanel] is provided, MatTabNav acts as an ARIA tabs. Otherwise, it continues to act as links within a navigation landmark (no change from status quo).
  • Adds unit tests for new behavior.
  • Change made in both non-MDC and MDC tabs.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 6, 2021
src/material/tabs/tab-nav-bar/tab-nav-bar.ts Outdated Show resolved Hide resolved
src/material/tabs/tab-nav-bar/tab-nav-bar.ts Outdated Show resolved Hide resolved
src/material/tabs/tab-nav-bar/tab-nav-bar.ts Outdated Show resolved Hide resolved
src/material/tabs/tab-nav-bar/tab-nav-bar.ts Outdated Show resolved Hide resolved
src/material/tabs/tab-nav-bar/tab-nav-bar.ts Outdated Show resolved Hide resolved
@zelliott zelliott changed the title Refactor MatTabNav as ARIA tabs with a new MatTabNavPanel component and [panel] input. Refactor MatTabNav as ARIA tabs with a new MatTabNavPanel component and [tabPanel] input. Dec 7, 2021
@@ -151,6 +183,7 @@ export abstract class _MatTabNavBase
templateUrl: 'tab-nav-bar.html',
styleUrls: ['tab-nav-bar.css'],
host: {
'[attr.role]': 'tabPanel ? "tablist" : null',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@crisbeto This - as well as some of the aria-* attribute host bindings changes below - are breaking changes. For example, if a consumer has added role="<something>" to this element, then after this PR is submitted, this host binding will override their specified role and set it to null (as no tabPanel will have been provided). We can avoid this breaking change by modifying this logic to check to see if a role has been provided, and if so, use that. For example:

'[attr.role]': 'existingRole || (tabPanel ? "tablist" : null)'

We'd want to do the same to the other added aria-* host bindings. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this is a breaking change since our host bindings are implementation details and they won't break people's builds. I also suspect that it's rare for people to actually set these aria- attributes correctly. If we do get issue reports about it, it'll be easy to follow up with a fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay.

Copy link
Collaborator Author

@zelliott zelliott Dec 17, 2021

Choose a reason for hiding this comment

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

Updated the PR per our conversation to fallback to the existing ARIA host attributes if no tabPanel is provided, as opposed to removing those attributes with a null attribute binding. This makes this PR now truly 100% opt-in & backwards compatible. Consumer code should not change if tabPanel isn't provided. This makes it much easier to land internally.

After this PR is merged in, I'll:

  1. Migrate the internal instances of consumers overriding ARIA host attributes to the new tabPanel input & associated component.
  2. Remove the code in this PR that "falls back to the existing ARIA host attributes if no tabPanel is provided" (as it calls the DOM directly and not something we want in the component long-term).
  3. Migrate the remaining internal consumer instances onto the new tabPanel input & associated component.
  4. Remove the code in this PR that falls back to the ARIA link / navigation landmark pattern if no tabPanel is provded.

Re-opening to give @crisbeto a chance to ack this.

@zelliott zelliott marked this pull request as ready for review December 20, 2021 20:30
@zelliott zelliott requested review from andrewseguin and a team as code owners December 20, 2021 20:30
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.

LGTM, the last comment isn't a blocker.

@crisbeto crisbeto added G This is is related to a Google internal issue action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release labels Dec 22, 2021
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@josephperrott josephperrott removed the request for review from a team January 4, 2022 21:46
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Read through the summary and did a quick pass on the code, overall LGTM.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2022
@jelbourn jelbourn added the Accessibility This issue is related to accessibility (a11y) label Mar 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reevaluate tab navigation interaction pattern
6 participants