Skip to content

3199 resize window tabs flicker #3202

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

Merged
LFDanLu merged 9 commits into from
Jul 23, 2022
Merged

3199 resize window tabs flicker #3202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5a6dd9b
fix flickering line in collapsed mode
tomsontom Jun 8, 2022
6c6829a
fix tests to pass an aria label
tomsontom Jun 8, 2022
a7c137e
fix remarks and rename property
tomsontom Jun 13, 2022
cfd64ca
Update packages/@react-spectrum/tabs/src/Tabs.tsx
tomsontom Jun 28, 2022
95e339f
Merge branch 'main' into 3199-Resize-window-tabs-flicker
snowystinger Jun 28, 2022
aa4f521
fixed overflow behavior
tomsontom Jun 29, 2022
595d89b
Merge branch '3199-Resize-window-tabs-flicker' of github.com:tomsonto…
tomsontom Jun 29, 2022
07e92c7
Merge branch 'main' into 3199-Resize-window-tabs-flicker
snowystinger Jul 18, 2022
ac1196d
Merge branch 'main' into 3199-Resize-window-tabs-flicker
LFDanLu Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 38 additions & 38 deletions packages/@react-spectrum/tabs/src/Tabs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

import {classNames, SlotProvider, unwrapDOMRef, useDOMRef, useStyleProps} from '@react-spectrum/utils';
import {DOMProps, DOMRef, Node, Orientation} from '@react-types/shared';
import {filterDOMProps, useValueEffect} from '@react-aria/utils';
import {filterDOMProps} from '@react-aria/utils';
import {FocusRing} from '@react-aria/focus';
import {Item, Picker} from '@react-spectrum/picker';
import {ListCollection, SingleSelectListState} from '@react-stately/list';
Expand All @@ -36,7 +36,7 @@ interface TabsContext<T> {
tabListState: TabListState<T>,
setTabListState: (state: TabListState<T>) => void,
selectedTab: HTMLElement,
collapse: boolean
collapsed: boolean
},
refs: {
wrapperRef: MutableRefObject<HTMLDivElement>,
Expand Down Expand Up @@ -64,7 +64,7 @@ function Tabs<T extends object>(props: SpectrumTabsProps<T>, ref: DOMRef<HTMLDiv

let {direction} = useLocale();
let {styleProps} = useStyleProps(otherProps);
let [collapse, setCollapse] = useValueEffect(false);
let [collapsed, setCollapsed] = useState(false);
let [selectedTab, setSelectedTab] = useState<HTMLElement>();
const [tabListState, setTabListState] = useState<TabListState<T>>(null);

Expand All @@ -77,34 +77,21 @@ function Tabs<T extends object>(props: SpectrumTabsProps<T>, ref: DOMRef<HTMLDiv
}
}
// collapse is in the dep array so selectedTab can be updated for TabLine positioning
}, [children, tabListState?.selectedKey, collapse, tablistRef]);
}, [children, tabListState?.selectedKey, collapsed, tablistRef]);

let checkShouldCollapse = useCallback(() => {
let computeShouldCollapse = () => {
if (wrapperRef.current) {
let tabsComponent = wrapperRef.current;
let tabs = tablistRef.current.querySelectorAll('[role="tab"]');
let lastTab = tabs[tabs.length - 1];

let end = direction === 'rtl' ? 'left' : 'right';
let farEdgeTabList = tabsComponent.getBoundingClientRect()[end];
let farEdgeLastTab = lastTab?.getBoundingClientRect()[end];
let shouldCollapse = direction === 'rtl' ? farEdgeLastTab < farEdgeTabList : farEdgeTabList < farEdgeLastTab;

return shouldCollapse;
}
};

if (orientation !== 'vertical') {
setCollapse(function* () {
// Make Tabs render in non-collapsed state
yield false;

// Compute if Tabs should collapse and update
yield computeShouldCollapse();
});
if (wrapperRef.current && orientation !== 'vertical') {
let tabsComponent = wrapperRef.current;
let tabs = tablistRef.current.querySelectorAll('[role="tab"]');
let lastTab = tabs[tabs.length - 1];

let end = direction === 'rtl' ? 'left' : 'right';
let farEdgeTabList = tabsComponent.getBoundingClientRect()[end];
let farEdgeLastTab = lastTab?.getBoundingClientRect()[end];
let shouldCollapse = direction === 'rtl' ? farEdgeLastTab < farEdgeTabList : farEdgeTabList < farEdgeLastTab;
setCollapsed(shouldCollapse);
}
}, [tablistRef, wrapperRef, direction, orientation, setCollapse]);
}, [tablistRef, wrapperRef, direction, orientation, setCollapsed]);

useEffect(() => {
checkShouldCollapse();
Expand All @@ -118,14 +105,14 @@ function Tabs<T extends object>(props: SpectrumTabsProps<T>, ref: DOMRef<HTMLDiv

// When the tabs are collapsed, the tabPanel should be labelled by the Picker button element.
let collapsibleTabListId = useId();
if (collapse && orientation !== 'vertical') {
if (collapsed && orientation !== 'vertical') {
tabPanelProps['aria-labelledby'] = collapsibleTabListId;
}
return (
<TabContext.Provider
value={{
tabProps: {...props, orientation, density},
tabState: {tabListState, setTabListState, selectedTab, collapse},
tabState: {tabListState, setTabListState, selectedTab, collapsed},
refs: {tablistRef, wrapperRef},
tabPanelProps
}}>
Expand Down Expand Up @@ -255,7 +242,7 @@ export function TabList<T>(props: SpectrumTabListProps<T>) {
const tabContext = useContext(TabContext);
const {refs, tabState, tabProps, tabPanelProps} = tabContext;
const {isQuiet, density, isDisabled, isEmphasized, orientation} = tabProps;
const {selectedTab, collapse, setTabListState} = tabState;
const {selectedTab, collapsed, setTabListState} = tabState;
const {tablistRef, wrapperRef} = refs;
// Pass original Tab props but override children to create the collection.
const state = useTabListState({...tabProps, children: props.children});
Expand All @@ -268,12 +255,18 @@ export function TabList<T>(props: SpectrumTabListProps<T>) {
setTabListState(state);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.disabledKeys, state.selectedItem, state.selectedKey, props.children]);
let stylePropsForVertical = orientation === 'vertical' ? styleProps : {};

let collapseStyle : React.CSSProperties = collapsed && orientation !== 'vertical' ? {maxWidth: 'calc(100% + 1px)', overflow: 'hidden', visibility: 'hidden', position: 'absolute'} : {maxWidth: 'calc(100% + 1px)'};
let stylePropsFinal = orientation === 'vertical' ? styleProps : {style: collapseStyle};

if (collapsed && orientation !== 'vertical') {
Copy link
Copy Markdown
Member

@snowystinger snowystinger Jul 18, 2022

Choose a reason for hiding this comment

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

question for second reviewer, should we move this into our aria hook? it seems very specific to our implementation in rendering, so I think it's fine here

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu Jul 22, 2022

Choose a reason for hiding this comment

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

I agree, the collapse behavior is specific to Spectrum, I'd rather not force these styles on aria hook consumers.

tabListProps['aria-hidden'] = true;
}

let tabListclassName = classNames(styles, 'spectrum-TabsPanel-tabs');
const tabContent = (
<div
{...stylePropsForVertical}
{...stylePropsFinal}
{...tabListProps}
ref={tablistRef}
className={classNames(
Expand Down Expand Up @@ -309,7 +302,8 @@ export function TabList<T>(props: SpectrumTabListProps<T>) {
'spectrum-TabsPanel-collapseWrapper',
styleProps.className
)}>
{collapse ? <TabPicker {...props} {...tabProps} id={tabPanelProps['aria-labelledby']} state={state} className={tabListclassName} /> : tabContent}
<TabPicker {...props} {...tabProps} visible={collapsed} id={tabPanelProps['aria-labelledby']} state={state} className={tabListclassName} />
{tabContent}
</div>
);
}
Expand Down Expand Up @@ -359,7 +353,8 @@ interface TabPickerProps<T> extends Omit<SpectrumPickerProps<T>, 'children'> {
density?: 'compact' | 'regular',
isEmphasized?: boolean,
state: SingleSelectListState<T>,
className?: string
className?: string,
visible: boolean
}

function TabPicker<T>(props: TabPickerProps<T>) {
Expand All @@ -372,7 +367,8 @@ function TabPicker<T>(props: TabPickerProps<T>) {
'aria-label': ariaLabel,
density,
className,
id
id,
visible
} = props;

let ref = useRef();
Expand All @@ -394,6 +390,8 @@ function TabPicker<T>(props: TabPickerProps<T>) {
'aria-label': ariaLabel
};

const style : React.CSSProperties = visible ? {} : {visibility: 'hidden', position: 'absolute'};

// TODO: Figure out if tabListProps should go onto the div here, v2 doesn't do it
return (
<div
Expand All @@ -408,7 +406,9 @@ function TabPicker<T>(props: TabPickerProps<T>) {
'spectrum-Tabs--emphasized': isEmphasized
},
className
)}>
)}
style={style}
aria-hidden={visible ? undefined : true}>
<SlotProvider
slots={{
icon: {
Expand All @@ -425,7 +425,7 @@ function TabPicker<T>(props: TabPickerProps<T>) {
items={items}
ref={ref}
isQuiet
isDisabled={isDisabled}
isDisabled={!visible || isDisabled}
Comment thread
snowystinger marked this conversation as resolved.
selectedKey={state.selectedKey}
disabledKeys={state.disabledKeys}
onSelectionChange={state.setSelectedKey}>
Expand Down
14 changes: 7 additions & 7 deletions packages/@react-spectrum/tabs/test/Tabs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function renderComponent(props = {}, itemProps) {
let {items = defaultItems} = props;
return render(
<Provider theme={theme}>
<Tabs {...props} items={items}>
<Tabs aria-label="Tab Sample" {...props} items={items}>
<TabList>
{item => (
<Item {...itemProps} key={item.name} title={item.name || item.children} />
Expand Down Expand Up @@ -222,7 +222,7 @@ describe('Tabs', function () {
it('does not generate conflicting ids between multiple tabs instances', function () {
let tree = render(
<Provider theme={theme}>
<Tabs>
<Tabs aria-label="Tab Sample">
<TabList>
{defaultItems.map(item => (
<Item key={item.name} title={item.name || item.children} />
Expand All @@ -236,7 +236,7 @@ describe('Tabs', function () {
))}
</TabPanels>
</Tabs>
<Tabs>
<Tabs aria-label="Tab Sample 2">
<TabList>
{defaultItems.map(item => (
<Item key={item.name} title={item.name || item.children} />
Expand Down Expand Up @@ -315,7 +315,7 @@ describe('Tabs', function () {

tree.rerender(
<Provider theme={theme}>
<Tabs disabledKeys={[defaultItems[0].name]} onSelectionChange={onSelectionChange} items={defaultItems.slice(0, 1)}>
<Tabs aria-label="Tab Example" disabledKeys={[defaultItems[0].name]} onSelectionChange={onSelectionChange} items={defaultItems.slice(0, 1)}>
<TabList>
{item => (
<Item key={item.name} title={item.name || item.children} />
Expand Down Expand Up @@ -498,7 +498,7 @@ describe('Tabs', function () {

rerender(
<Provider theme={theme}>
<Tabs items={newItems} orientation="vertical">
<Tabs aria-label="Tab Example" items={newItems} orientation="vertical">
<TabList>
{item => (
<Item key={item.name} title={item.name || item.children} />
Expand Down Expand Up @@ -617,7 +617,7 @@ describe('Tabs', function () {
it('tabpanel should have tabIndex=0 only when there are no focusable elements', async function () {
let {getByRole, getAllByRole} = render(
<Provider theme={theme}>
<Tabs maxWidth={500}>
<Tabs aria-label="Tab Example" maxWidth={500}>
<TabList>
<Item>Tab 1</Item>
<Item>Tab 2</Item>
Expand Down Expand Up @@ -652,7 +652,7 @@ describe('Tabs', function () {
it('TabPanel children do not share values between panels', () => {
let {getByDisplayValue, getAllByRole, getByTestId} = render(
<Provider theme={theme}>
<Tabs maxWidth={500}>
<Tabs aria-label="Tab Example" maxWidth={500}>
<TabList>
<Item>Tab 1</Item>
<Item>Tab 2</Item>
Expand Down