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

feat(cdk-experimental/ui-patterns): tabs ui pattern #30568

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

Conversation

ok7sai
Copy link

@ok7sai ok7sai commented Feb 28, 2025

No description provided.

@ok7sai ok7sai requested a review from a team as a code owner February 28, 2025 18:20
@ok7sai ok7sai requested review from crisbeto and andrewseguin and removed request for a team February 28, 2025 18:20
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Feb 28, 2025
@ok7sai ok7sai force-pushed the ng-aria-tabs branch 2 times, most recently from f82703c to eed8a03 Compare March 4, 2025 18:04
@wagnermaciel wagnermaciel requested a review from jelbourn March 4, 2025 23:43
'class': 'cdk-tabs',
},
})
export class CdkTabs {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little unsure about having CdkTabs as a necessary wrapper / container vs. the tabpanel having a reference/input for the tablist, implying a 1:1 relationship between tablist and tabpanel.

Copy link
Author

Choose a reason for hiding this comment

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

I was going to include the decision making of choosing a top-level wrapper in the design doc for later. Here's a quick comparison.

Passing references

<ul cdkTablist>
  <li cdkTab #tab1="tab">tab1</li>
  <li cdkTab #tab2="tab">tab2</li>
</ul>
<div cdkTabpanel [for]="tab1"></div>
<div cdkTabpanel [for]="tab2"></div>

Note that the template variable referencing can be reversed. The only dealbreaker I can think of is the dynamic generated tabs or tabpanels via control flow may be impossible. Another downside is the templating effort of creating template references for general use case.

The wrapper solution on the other hand can support both implicit and explicit(yet implemented, but likely via a string identifier other than id) tab-tabpanel binding, which I think it's more intuitive. I also found the same pattern used in most similar libraries, so it can be familiar to developers as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we should probably have a discussion on the API(s) we expect an end-developer to define for their own tab implementations, and then how to best accommodate that with the directives. FWIW I think we will have a requirement that there may be elements between the tablist and the tabpanel(s).

For content, I suspect these directives will need to end up being oriented around rendering <ng-template> content.

/** The required inputs to tabs. */
export interface TabInputs extends ListNavigationItem, ListSelectionItem, ListFocusItem {
tablist: Signal<TablistPattern>;
tabpanel: Signal<TabpanelPattern>;
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to me that the tab would need to know about the tabpanel. I was imagining the relationship would be that the tabpanel only knows about the value of the tablist, not necessarily any of the specifics tabs

}

/** A tabpanel associated with a tab. */
export class TabpanelPattern {
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a bigger conversation, but I wonder whether it makes sense to have a tabpanel pattern at all, since it doesn't actually do or render anything.

'class': 'cdk-tabs',
},
})
export class CdkTabs {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we should probably have a discussion on the API(s) we expect an end-developer to define for their own tab implementations, and then how to best accommodate that with the directives. FWIW I think we will have a requirement that there may be elements between the tablist and the tabpanel(s).

For content, I suspect these directives will need to end up being oriented around rendering <ng-template> content.

Comment on lines +40 to +45
* <div cdkTabpanel>Tab content 1</div>
* <div cdkTabpanel>Tab content 2</div>
* <div cdkTabpanel>Tab content 3</div>
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any info on whether having one tabpanel element for each tab is strictly necessary, or can there be one tabpanel that changes attributes/content?

cc @crisbeto I vaguely recall this has come up before but I don't remember the details. Do you happen to recall anything?

Copy link
Author

Choose a reason for hiding this comment

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

Hidden tabpanels should have at least aria-hidden attribute, from the accessibility tree perspective there will be only one tabpanel, so having one or one per tab are likely the same. Most of the ARIA examples have one tabpanel per tab due to the static content.

It's probably fine supporting both ways

<!-- For static tabs -->
<div cdkTabs>
  <ul cdkTablist>
    <li cdkTab>tab 1</li>
    <li cdkTab>tab 2</li>
  </ul>

  <div cdkTabpanel>content 1</div>
  <div cdkTabpanel>content 2</div>
  <!-- Or structural directives -->
  @for (tabpanel of tabpanels) {
    <div cdkTabpanel>{{tabpanel.content}}</div>
  }
</div>

<!-- Or for deferred content -->
<div cdkTabs>
  <ul cdkTablist #tablist>
    <li cdkTab tab="tab1">tab 1</li>
    <li cdkTab tab="tab2">tab 2</li>
  </ul>

  <!-- Just an ideal, CdkTablist does not expose selection yet. -->
  <div cdkTabpanel [tab]="tablist.selected...">{{getContent(...)}}</div>
</div>

@ok7sai ok7sai force-pushed the ng-aria-tabs branch 2 times, most recently from c87bd4c to 91dacdf Compare March 13, 2025 21:30
},
],
})
export class CdkTabpanel {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: capitalize Panel so its CdkTabPanel

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure if we want to keep the naming consistent with corresponding roles https://w3c.github.io/aria/#tabpanel

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for CdkTabPanel. I think it's the more intuitive name here. Worth double checking what other libraries have done

Copy link
Author

Choose a reason for hiding this comment

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

TabPanel is used in React Aria, and Radix uses TabContent instead, so I guess capitalizing Panel and Content make sense. Let me address this.

exportAs: 'cdTabcontent',
hostDirectives: [DeferredContent],
})
export class CdkTabcontent {}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: capitalize Content so its CdkTabContent

Copy link
Author

Choose a reason for hiding this comment

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

I think the decision relies on how we name CdkTabpanel

CdkTabpanel + CdkTabcontent
vs
CdkTabPanel + CdkTabContent

[focusMode]="focusMode"
[selectionMode]="selectionMode"
>
<li cdkTab class="example-tab">tab 1</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we enforce an "id" attribute to connect the tab and its panel/content? It's not easy to read which panel connects with which tab, except by counting them. E.g. consider 8 tabs with complex html here. React Aria does this id connection with tabs https://react-spectrum.adobe.com/react-aria/Tabs.html

Copy link
Author

Choose a reason for hiding this comment

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

Agreed that it gets more difficult to visually matching tabs and tabpanels when the tabs amount increases. The only potential risk with the explicit id matching is the developer provided ids are not guarantee unique, so it relies on developers to use the API correctly.

@wagnermaciel we had the discussion on this before and the conclusion was to keep the API as simple as possible. Do you think if we should support both explicit and implicit matching, or enforce one solution? Now the list selection supports a value for each item, so we can potentially use the value field as the tabs identifier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think enforcing an id would be a valid approach. I think the majority of tabs I've seen have been 3-4 in size so I think there's value in simplicity, but if we think it'll end up being a footgun for users then it's better to take the verbose approach at all times.

<mat-form-field subscriptSizing="dynamic" appearance="outline">
<mat-label>Selection strategy</mat-label>
<mat-select [(value)]="selectionMode">
<mat-option value="explicit">Explicit</mat-option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Explicit with "Active Descendant" doesn't let the user change tabs. Should we somehow disallow this combo, or rely on docs to make sure users dont combine those

Copy link
Author

Choose a reason for hiding this comment

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

Styling the focus state with aria-activedescendant is currently unimplemented. The listbox demo has the same issue AFAIK. This combo should be allowed, and we just need to figure out a way for developers to style the focus state.

@wagnermaciel any thoughts? we can apply a class like cdk-focused or cdk-focus-indicator to directives that are focused but not necessary selected and let developers style based on these classes. Or we can provide inputs like focusedClass="..." to allow developers apply their owned classes (can be useful for applying tailwind classes?)

Copy link
Contributor

Choose a reason for hiding this comment

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

That wouldn't be necessary for people using tailwind
https://tailwindcss.com/docs/hover-focus-and-other-states

I think adding a class makes sense, but I would propose calling it "active" instead of "focused"

Copy link
Author

Choose a reason for hiding this comment

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

Using aria-activedescendant means the child element won't receive the actual focus, so I don't think the tailwind util class like focus: would work. (The same reason today that :focus selector doesn't work for aria-activedescendant + explicit combo)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah shoot, you're right. For now I'd say just roll with adding the extra class. We can have a separate discussion on whether we want to tailor a solution for tailwind

Copy link
Author

Choose a reason for hiding this comment

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

I'd suggest let's hold the changes to the next PR to fix both listbox and tabs at the same time.

'tabindex': '0',
'class': 'cdk-tabpanel',
'[attr.id]': 'pattern.id()',
'[attr.inert]': 'pattern.hidden() ? true : null',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also go ahead and set the style display: none? Is there a reason why the user would want anything different

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend against setting any styles since even innocent seeming ones like display: none can end up with users overriding our styles (e.g. to do some animation). imo we should leave how the tab panel gets visually hidden up to the developer

Copy link
Author

Choose a reason for hiding this comment

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

+1 to Wagner's point. One example can be the tablist based Carousel Pattern, that the left and right hidden panels can be slightly faded and transformed without being visually hidden as long as the keyboard navigation makes sense to the screenreader users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
detected: feature PR contains a feature commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants