-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: Reset TabPanels size variables on TabPanel so nested Tabs do not inherit them #10293
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
base: main
Are you sure you want to change the base?
Changes from all commits
9391cdf
4bb8d2a
6fd0892
c8b3e58
b805a3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -251,6 +251,44 @@ export interface TabPanelRenderProps { | |||||||||||||||||||||||||
| export const TabsContext = createContext<ContextValue<TabsProps, HTMLDivElement>>(null); | ||||||||||||||||||||||||||
| export const TabListStateContext = createContext<TabListState<any> | null>(null); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| interface CSSPropertyDefinition { | ||||||||||||||||||||||||||
| name: string; | ||||||||||||||||||||||||||
| syntax: string; | ||||||||||||||||||||||||||
| inherits: boolean; | ||||||||||||||||||||||||||
| initialValue: string; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| interface CSSWithRegisterProperty { | ||||||||||||||||||||||||||
| registerProperty: (definition: CSSPropertyDefinition) => void; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let supportsTabPanelSizePropertyRegistration = registerTabPanelSizeProperties(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| function registerTabPanelSizeProperties(): boolean { | ||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||
| typeof CSS === 'undefined' || | ||||||||||||||||||||||||||
| typeof (CSS as unknown as CSSWithRegisterProperty).registerProperty !== 'function' | ||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let css = CSS as unknown as CSSWithRegisterProperty; | ||||||||||||||||||||||||||
|
Comment on lines
+270
to
+275
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to work just fine?
Suggested change
|
||||||||||||||||||||||||||
| for (let name of ['--tab-panel-width', '--tab-panel-height']) { | ||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||
| css.registerProperty({ | ||||||||||||||||||||||||||
| name, | ||||||||||||||||||||||||||
| syntax: '*', | ||||||||||||||||||||||||||
| inherits: false, | ||||||||||||||||||||||||||
| initialValue: 'auto' | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Tabs organize content into multiple sections and allow users to navigate between them. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
|
|
@@ -627,11 +665,20 @@ function TabPanelInner( | |||||||||||||||||||||||||
| let domProps = isSelected | ||||||||||||||||||||||||||
| ? mergeProps(DOMProps, tabPanelProps, focusProps, renderProps) | ||||||||||||||||||||||||||
| : mergeProps(DOMProps, renderProps); | ||||||||||||||||||||||||||
| let style = renderProps.style; | ||||||||||||||||||||||||||
| if (!supportsTabPanelSizePropertyRegistration) { | ||||||||||||||||||||||||||
| style = { | ||||||||||||||||||||||||||
| '--tab-panel-width': 'unset', | ||||||||||||||||||||||||||
| '--tab-panel-height': 'unset', | ||||||||||||||||||||||||||
| ...renderProps.style | ||||||||||||||||||||||||||
| } as React.CSSProperties; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||
| <dom.div | ||||||||||||||||||||||||||
| {...domProps} | ||||||||||||||||||||||||||
| ref={ref} | ||||||||||||||||||||||||||
| style={style} | ||||||||||||||||||||||||||
| data-focused={isFocused || undefined} | ||||||||||||||||||||||||||
| data-focus-visible={isFocusVisible || undefined} | ||||||||||||||||||||||||||
| // @ts-ignore | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ import {Orientation} from '@react-types/shared'; | |
| import {OverlayArrow} from '../src/OverlayArrow'; | ||
| import React, {useState} from 'react'; | ||
| import {RouterProvider} from 'react-aria/private/utils/openLink'; | ||
| import {Tab, TabList, TabPanel, TabProps, Tabs} from '../src/Tabs'; | ||
| import {Tab, TabList, TabPanel, TabPanels, TabProps, Tabs} from '../src/Tabs'; | ||
| import {Tooltip, TooltipTrigger} from '../src/Tooltip'; | ||
| import './styles.css'; | ||
|
|
||
|
|
@@ -144,3 +144,39 @@ export const NestedTabs: TabsStory = () => ( | |
| <TabPanel id="bar">Bar</TabPanel> | ||
| </Tabs> | ||
| ); | ||
|
|
||
| // With non-inheriting panel size variables, inner TabPanels size to their own content. | ||
| // Without it, they inherit the outer TabPanels' pixel vars. | ||
| export const NestedTabsSizeTransition: TabsStory = () => ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This story is really hard to use due to the scrolling container that centers it. |
||
| <Tabs> | ||
| <TabList aria-label="Outer tabs" style={{display: 'flex', gap: 8}}> | ||
| <CustomTab id="nested">Nested tabs</CustomTab> | ||
| <CustomTab id="large">Large panel</CustomTab> | ||
| </TabList> | ||
| <TabPanels className="animated-tabpanels"> | ||
| <TabPanel id="nested"> | ||
| <Tabs> | ||
| <TabList aria-label="Inner tabs" style={{display: 'flex', gap: 8}}> | ||
| <CustomTab id="one">One</CustomTab> | ||
| <CustomTab id="two">Two</CustomTab> | ||
| </TabList> | ||
| <TabPanels className="animated-tabpanels"> | ||
| <TabPanel id="one"> | ||
| <div style={{width: 140, padding: 8}}>One</div> | ||
| </TabPanel> | ||
| <TabPanel id="two"> | ||
| <div style={{width: 180, padding: 8}}> | ||
| Two | ||
| <br /> | ||
| Small inner panel | ||
| </div> | ||
| </TabPanel> | ||
| </TabPanels> | ||
| </Tabs> | ||
| </TabPanel> | ||
| <TabPanel id="large"> | ||
| <div style={{width: 480, height: 240, padding: 8}}>Large outer panel</div> | ||
| </TabPanel> | ||
| </TabPanels> | ||
| </Tabs> | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -695,6 +695,52 @@ describe('Tabs', () => { | |
| expect(innerTabs[1]).toHaveTextContent('Two'); | ||
| }); | ||
|
|
||
| it('falls back to resetting tab panel transition variables for nested tabpanels', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests aren't necessary, they can't actually test anything |
||
| let {getByTestId} = render( | ||
| <Tabs> | ||
| <TabList aria-label="Outer tabs"> | ||
| <Tab id="foo">Foo</Tab> | ||
| <Tab id="bar">Bar</Tab> | ||
| </TabList> | ||
| <TabPanels | ||
| data-testid="outer-tabpanels" | ||
| style={{ | ||
| '--tab-panel-width': '320px', | ||
| '--tab-panel-height': '240px' | ||
| }}> | ||
| <TabPanel id="foo" data-testid="outer-tabpanel"> | ||
| <Tabs> | ||
| <TabList aria-label="Inner tabs"> | ||
| <Tab id="one">One</Tab> | ||
| <Tab id="two">Two</Tab> | ||
| </TabList> | ||
| <TabPanels data-testid="inner-tabpanels"> | ||
| <TabPanel id="one">One</TabPanel> | ||
| <TabPanel id="two">Two</TabPanel> | ||
| </TabPanels> | ||
| </Tabs> | ||
| </TabPanel> | ||
| <TabPanel id="bar">Bar</TabPanel> | ||
| </TabPanels> | ||
| </Tabs> | ||
| ); | ||
|
|
||
| // Wait a tick for MutationObserver in useHasTabbableChild to fire. | ||
| // This avoids React's "update not wrapped in act" warning. | ||
| await waitFor(() => Promise.resolve()); | ||
|
|
||
| let outerTabPanels = getByTestId('outer-tabpanels'); | ||
| let outerTabPanel = getByTestId('outer-tabpanel'); | ||
| let innerTabPanels = getByTestId('inner-tabpanels'); | ||
|
|
||
| expect(outerTabPanels.style.getPropertyValue('--tab-panel-width')).toBe('320px'); | ||
| expect(outerTabPanels.style.getPropertyValue('--tab-panel-height')).toBe('240px'); | ||
| expect(outerTabPanel.style.getPropertyValue('--tab-panel-width')).toBe('unset'); | ||
| expect(outerTabPanel.style.getPropertyValue('--tab-panel-height')).toBe('unset'); | ||
| expect(innerTabPanels.style.getPropertyValue('--tab-panel-width')).toBe(''); | ||
| expect(innerTabPanels.style.getPropertyValue('--tab-panel-height')).toBe(''); | ||
| }); | ||
|
|
||
| it('can add tabs and keep the current selected key', async () => { | ||
| let onSelectionChange = jest.fn(); | ||
| function Example(props) { | ||
|
|
@@ -829,6 +875,62 @@ describe('Tabs', () => { | |
| expect(tabPanels).toHaveStyle({width: '100px'}); | ||
| }); | ||
|
|
||
| it('should allow tab panel styles to override fallback transition variable resets', () => { | ||
| let {getByTestId} = render( | ||
| <Tabs> | ||
| <TabList aria-label="test"> | ||
| <Tab id="a">A</Tab> | ||
| <Tab id="b">B</Tab> | ||
| </TabList> | ||
| <TabPanels> | ||
| <TabPanel | ||
| id="a" | ||
| data-testid="tabpanel" | ||
| style={{ | ||
| '--tab-panel-width': '50px', | ||
| '--tab-panel-height': '75px' | ||
| }}> | ||
| A | ||
| </TabPanel> | ||
| <TabPanel id="b">B</TabPanel> | ||
| </TabPanels> | ||
| </Tabs> | ||
| ); | ||
|
|
||
| let tabPanel = getByTestId('tabpanel'); | ||
| expect(tabPanel.style.getPropertyValue('--tab-panel-width')).toBe('50px'); | ||
| expect(tabPanel.style.getPropertyValue('--tab-panel-height')).toBe('75px'); | ||
| }); | ||
|
|
||
| it('should merge fallback transition variable resets with style render props', () => { | ||
| let {getByTestId} = render( | ||
| <Tabs> | ||
| <TabList aria-label="test"> | ||
| <Tab id="a">A</Tab> | ||
| <Tab id="b">B</Tab> | ||
| </TabList> | ||
| <TabPanels> | ||
| <TabPanel | ||
| id="a" | ||
| className={() => 'selected'} | ||
| data-testid="tabpanel" | ||
| style={() => ({ | ||
| opacity: 1 | ||
| })}> | ||
| A | ||
| </TabPanel> | ||
| <TabPanel id="b">B</TabPanel> | ||
| </TabPanels> | ||
| </Tabs> | ||
| ); | ||
|
|
||
| let tabPanel = getByTestId('tabpanel'); | ||
| expect(tabPanel).toHaveAttribute('class', 'selected'); | ||
| expect(tabPanel.style.getPropertyValue('--tab-panel-width')).toBe('unset'); | ||
| expect(tabPanel.style.getPropertyValue('--tab-panel-height')).toBe('unset'); | ||
| expect(tabPanel).toHaveStyle({opacity: '1'}); | ||
| }); | ||
|
|
||
| it('should detect block-size in transition for TabPanels', async () => { | ||
| let originalGetComputedStyle = window.getComputedStyle; | ||
| window.getComputedStyle = el => ({ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary