Adds new component TabPanels

[owenniblock]

This Work

Closes https://github.com/github/primer/issues/3043

Adds a new TabPanels component to mirror the Primer View Component TabPanels. We will need to retrospectively go back and update PVC to use the new <tab-container> which supports shadow DOM.

Screenshots

Changelog

• Adds new TabPanels alpha component to provide non-navigational tabs with pages.

New

• TabPanels component

Changed

• Seems like we were missing some snapshots for Dialog-v1

Removed

Rollout strategy

• [ ] Patch release
• [x] Minor release
• [ ] Major release; if selected, include a written rollout or migration plan
• [ ] None; if selected, include a brief description as to why

Testing & Reviewing

I'd particularly like guidance on the following:

• If any of my CSS could be improved
• If there are any additional features of tab-container-element which we should support
• If I've missed any tests

Merge checklist

• [x] Added/updated tests
• [x] Added/updated documentation
• [x] Added/updated previews (Storybook)
• [x] Changes are SSR compatible
• [x] Tested in Chrome
• [x] Tested in Firefox
• [x] Tested in Safari
• [x] Tested in Edge
• [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: a7376becdab982026f455e18901956a5e6d19323

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions[bot]]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	87.37 KB (-0.17% 🔽)
packages/react/dist/browser.umd.js	87.78 KB (+0.05% 🔺)

[owenniblock]

Status: rerunning

• dotcom - primer integration workflow ✅
• dotcom integration PR

Error:

Details:

 /workspace/github/node_modules/@primer/react/node_modules/@github/tab-container-element/dist/index.js:1
 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { TabContainerElement } from './tab-container-element.js';

SyntaxError: Cannot use import statement outside a module

[owenniblock]

We'll need to retrospectively update the PVC components to use the new shadow DOM version of @github/tab-container-element. @keithamus do we want to track that as part of the Epic? Or separately?

[owenniblock]

@joshblack thanks for those extensive notes, that's super helpful! I'll start going through them over the next few days. Will let you know if pairing / a chat would help.

[owenniblock]

it'd be great if this lived in src/drafts/index.ts

Done!

[owenniblock]

Re Accessibility stuff:

I've been thinking that it would be good to bring this to an Accessibility Office Hours (maybe next week). Here are my notes on the things you raised 🙂

make the aria-label or aria-labelledby required

I like this idea! We're aiming to make this component as close to the Primer View Component version as possible (which also has an optional aria-label I believe). We could back-port the required feature which would be a breaking change in PVC so might need to tread carefully there (I'd appreciate @keithamus' thoughts here also).

Should the element with role="tabpanel" have aria-labelledby set to the corresponding tab?

Yes! It used to. Maybe we lost that functionality during the conversion to shadow DOM magic. I'll check!

UPDATE: this was my fault :) I have updated the component so that this is required. I considered trying to set them automagically but decided to keep it simple.

Could we make the tabindex on the role="tabpanel" dynamic with respect to if it has interactive content? So if there is no interactive content in the panel, it has a tabindex. If it does have interactive content, then it does not

I like this idea, we'd need to make a change to @github/tab-container-element to support this (which consequently would mean that the same change gets reflected in PVC).

While it's only supported in JAWS, would it be worth adding aria-controls to the role="tab" elements?

Not sure, I'll investigate! 👍

I had some issues in JAWS getting the tab panel to announce 🤔 Curious what the expected flow would be here (or if my test procedure was wrong!)

Could be related to the missing labelledby 🤔 I'll do some digging. I don't have JAWS on my Windows machine (but I guess I should!)

Would it be possible for us to address the axe issue with role="tab" not being in role="tablist" so that downstream users won't also have to disable this rule?

This is tricky because it's not actually an issue. The Accessibility Tree is correct but because it uses shadow dom and slots etc, axe doesn't recognise it correctly. It might be possible to contribute a fix to axe so that it works with components leveraging shadow dom but I'm not certain.

[owenniblock]

EDIT: I've added some logic to work out which panels should be hidden and add the hidden attribute to them. Will test it a bit and then update the docs.

There seems to be a flash of unstyled content on slower network speeds or in browsers like Firefox

Ah, @keithamus maybe we need to have users set hidden to the correct tab panels initially 🤔 I was hoping we wouldn't have to but I'm pretty that that's what's causing this. I think we might need to make a small change to @github/tab-container-element so that it removes hidden from the panel in the DOM when adding it to the shadow slot (unless it already does this, I will check!)

[keithamus]

There seems to be a flash of unstyled content on slower network speeds or in browsers like Firefox

Ah, @keithamus maybe we need to have users set hidden to the correct tab panels initially 🤔 I was hoping we wouldn't have to but I'm pretty that that's what's causing this.

We could explore CSS options here. E.g. tab-container:not(:defined) [role=panel]:not(:first-child) { display: none } would probably get us 99% of the way there. (There'd be a potential flash if e.g. the second tab was selected, perhaps some more CSS could fix that).

I think we might need to make a small change to @github/tab-container-element so that it removes hidden from the panel in the DOM when adding it to the shadow slot (unless it already does this, I will check!)

It does this already.

[keithamus]

• Could we make the tabindex on the role="tabpanel" dynamic with respect to if it has interactive content? So if there is no interactive content in the panel, it has a tabindex. If it does have interactive content, then it does not

This seems very tricky to properly determine, and seems like it would create an undesirable experience? For example in use on the CommentEditor the Preview pane sometimes has interactive content if there is a link or code block in the message, while other times it doesn't. I'm not certain we want this but I'd love to hear more.

• There seem to be hydration errors when SSR'd

We might have to add suppressHydrationWarning for this component as the custom element will set things like tabindex, which is not possible to do without mutating the DOM (unless we also reapply the same logic to the React component but that's doing work twice). We can selectively add it to the tabs and panels or just on the whole component.

Would it be possible to configure the tablist itself? For example, if we wanted to wrap it in a nav tab for a navigation tabs setup (or if this not preferred?)

This is not preferred. For those uses people should reach for TabNav. We should document accordingly. Having said that this is entirely possible in the custom element, you can supply your own role=tablist and it'll respect that.

• It'd be great if we could make onTabContainerChange to simply onChange

Arguably we should do this in the custom element too.

• Do we want to support a version of controlled state instead of uncontrolled?

The only piece of (un)controlled state is the active tab, right? We could add this as a prop and stick it in a useEffect that calls selectTab (alternatively we can make a prop in the custom element to drive the same behaviour).

[owenniblock]

@joshblack I spent some time with @keithamus this morning experimenting with CSS options for the flash on content issue (thanks Keith!)

Would love you to give it a test, we've had to compromise in a couple of places because the structure of the tablist & additional content elements doesn't exist yet. So there are the following limitations:

• The underline of the tabs only covers the tabs themselves and not the full width of the component (also noticed that there's a very small gap in the line between each tab, might look at that 🤔)
• If you have 11 or more tabs and load the component with the 11th (or higher) tab selected, the tab panel doesn't show until the component loads.

I'll need to retest in various browsers, so let me know if this approach seems viable to you and then I'll do that 🙂

[owenniblock]

Signed up for Wednesday's Accessibility Office Hours to check it all looks OK 🤞

[iansan5653]

A small accessibility concern I noticed in the demo - it doesn't look like we are wrapping the tabs in a tablist container. This is a required parent for the tab role. This might need to be something we fix in the custom element?

[mperrotti]

Just a heads-up, we just started a project to wrangle all of our different kinds of navigation. The current direction I'm exploring removes TabNav and TabPanels and just uses UnderlineNav and UnderlinePanels.

Google doc for context WIP Explorations in Figma

[owenniblock]

A small accessibility concern I noticed in the demo - it doesn't look like we are wrapping the tabs in a tablist container. This is a required parent for the tab role. This might need to be something we fix in the custom element?

@iansan5653 this is actually handled in the custom element, the tablist role is added in the Shadow DOM and isn't required in the Light DOM. I recently fixed an issue where I was setting the tablist role on a slot element (which is invalid) and now this passes accessibility checks. We should get additional feedback from screen reader users soon as well.

[owenniblock]

@mperrotti I'll look through those items. Is there a good place for feedback and/or discussion? I have some concerns that we'll lose important nuances around the underlying semantics of the controls we already have. For example I know we added a separate component in Rails (TabPanels and TabNav) because folks didn't know to only use TabNav for navigation elements. I'll dig into this in more detail wherever is the best place for the discussion 🙂

[mperrotti]

@owenniblock - you can add feedback in the Figma or send me feedback on Slack. I also brought this to a11y office hours earlier this week https://github.com/github/accessibility/issues/5623#issuecomment-1967930432

We could still have "nav" vs "panels" distinction for the nuances around the underlying semantics.

[iansan5653]

A small accessibility concern I noticed in the demo - it doesn't look like we are wrapping the tabs in a tablist container. This is a required parent for the tab role. This might need to be something we fix in the custom element?

@iansan5653 this is actually handled in the custom element, the tablist role is added in the Shadow DOM and isn't required in the Light DOM. I recently fixed an issue where I was setting the tablist role on a slot element (which is invalid) and now this passes accessibility checks. We should get additional feedback from screen reader users soon as well.

Got it, thanks! I was confused by the shadow DOM / slots in the browser devtools, but looking at the accessibility tree confirms that there is in fact a tablist :+1:

[owenniblock]

We were going to change the events to onChange and onChanged - do we want to do that as part of this PR?

[owenniblock]

Integration fails

   routes/HistoryComparisonPage.tsx(95,15): error TS2339: Property 'relative-time' does not exist on type 'JSX.IntrinsicElements'.
  routes/HistorySummaryPage.tsx(86,21): error TS2339: Property 'relative-time' does not exist on type 'JSX.IntrinsicElements'.
  npm ERR! Lifecycle script `tsc` failed with error: 
  npm ERR! Error: command failed 
  npm ERR!   in workspace: @github-ui/repos-rules 
  npm ERR!   at location: /workspace/github/ui/packages/repos-rules 

[joshblack]

One separate question I wanted to ask was how should a selected tab present in WHCM? I think the border radius can tell you what tab is selected but it's a little subtle so wanted to double-check.

This 100% feels like a separate issue but wanted to bring it up to see what you all thought since I was going through it during testing.

[github-actions[bot]]

Uh oh! @joshblack, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/4241#issuecomment-1984789441.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

[owenniblock]

@joshblack @iansan5653 there's a lot of feedback here to look at so I'm going to attempt to summarise where I'm at in a big long comment 😅 Please let me know if I missed some feedback that's important to address.

Here are the things I think I've resolved / can answer:

From @joshblack

There seems to be a focus stealing bug on page load so that the selected tab becomes focused on page load. Let me know if this is environment specific for me or if you're seeing it too

This was because it was running selectedTab - I've changed this so that when you set defaultTabIndex, it doesn't do this. However if you programatically set the tab using selectedTabIndex this still occurs. This is intentional because new content appearing on the page should be announced in some way. Focusing on the selected tab both communicates to the user that the tab has been selected as well as positioning the user close to the tab panel content.

I wanted to ask what the takeaway was from office hours around the tabindex for tabpanel behavior as I couldn't remember. APG recommends this but I know these aren't always the best recommendation.

Takeaway from discussions with Tetralogical was that having all tabpanels as focusable elements was the best option.

For uncontrolled/controlled, one pattern we have is that default* props are used to initialize state. So something like defaultIndex would initialize the state value for the first render but nothing after. For the controlled case, selectedIndex would be used and would change the state

I think this is now sorted!

When using it controlled with selectedTabIndex, it seems like changing this value will cause focus to shift to the newly selected tab. For controlled, I don't think we would want this behavior as it's not really intended to shift folks over but to change the state value in some way for when the user interacts with the component

This is intentional - the use-case I can think of here is that the user presses a keyboard shortcut to select a tab (Preview for example) the user then should be directed to that changed content. If you can think of a scenario where this wouldn't be the case, please let me know.

From @iansan5653

I feel like if we have an id prop in the type definition, we have to respect it since then it becomes part of the external contract. But I agree it's not essential for consumers to set the ids themselves. Maybe we could 'lie' about / hide the props by excluding id from the prop types for the Tab and Panel components. It would still get assigned when cloned because the underlying styled component would still handle it.

EDIT: Moved to done

From @joshblack

Could we could make aria-label (and I guess aria-labelledby) required in the types for TabPanels so that one of these must be set?

EDIT: Moved to done :-) I'll experiment with those types to make it neater before we merge.

From @joshblack

For the controlled case, what's the intended way to synchronize the current selected tab with the index being provided? It seems like onTabContainerChange provides the tab but not the index so would that mean we would need to look it up in some way?

EDIT: Moved to done! This should be sorted when I update to v4.5.0 of tab-container-element

Here are some things I want to look at before we merge:

EDIT: I did everything in this list. Let me know if anything should be moved here from the next section.

Here are some things I think we should discuss and/or resolve before moving this out of draft

The pattern of the API (Tab/Tab/Tab,Panel/Panel/Panel or Tab/Panel/Tab/Panel/Tab/Panel or a single component for both). The second and third options will currently have a larger unstyled content flash/UI drift because the tabs need to be moved together into a tablist etc. We might be able to resolve this using Declarative Shadow DOM, I suggest we release what we've got as a draft, work on the DSD stuff and then settle on a final API pattern for the component before moving to alpha.

From @joshblack

TypeScript types seem to be missing for common props that might be passed into the elements, e.g. things like onClick. Let me know if this is more my setup versus something else

I want to look into this if I have time, but I don't think it should stop us from releasing the draft to unblock the rest of the Comment Editor work.

From both :-)

Ordering stuff

I'd like to continue with using index and position for the draft. Perhaps we can hammer our this decision when we discuss the final version of the API 🤔

I think this technique would make it so that people couldn't wrap Tab in a custom component as then it wouldn't be able to infer the props from the children

I'd like to include this as part of the API discussion after draft is released.

I'm noticing that the border will flash with our styles since we don't have access to the tablist yet and so it can only span our buttons that are inline-block. Is there any way we could have that node defined?

Also wrapped up with the DSD stuff, I think this should be one of the first things we look at after the initial draft release. If we get DSD working it will open up some different patterns with how we structure the underlying light DOM content without having to worry about styling issues.

One separate question I wanted to ask was how should a selected tab present in WHCM? I think the border radius can tell you what tab is selected but it's a little subtle so wanted to double-check.

I wonder if this is a Primer CSS issue - would be interested to see if the same issue exists with TabNav and if not, unpick why that's the case 🤔

[owenniblock]

Integration test for dotcom passes ✅

[owenniblock]

Not sure why design reviewers is on this PR, I've added engineer reviewers 🤷

[owenniblock]

CI was green when I reran the failing stuff so going to try one last time to merge...