Skip to content

Commit

Permalink
fix(tabs-motion): remove focus from tabpanel (uber#3763)
Browse files Browse the repository at this point in the history
* fix(tabs-motion): remove focus from tabpanel

Also fixes an issue where focusVisible wasn't propagating properly to root components.

* fix: update e2e spec
  • Loading branch information
sandgraham authored Sep 15, 2020
1 parent 43a6ea3 commit 130570d
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 37 deletions.
5 changes: 1 addition & 4 deletions src/tabs-motion/__tests__/tabs-motion.e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,12 @@ describe('tabs', () => {
expect(await isActiveEl(tabs[1])).toBeTruthy();
});

it('*tab* moves focus to tabpanel and then tab content', async () => {
it('*tab* moves focus to tab content', async () => {
await mount(page, 'tabs-motion-focus');
const tabs = await getTabs();
await tabs[1].focus();
expect(await isActiveEl(tabs[1])).toBeTruthy();
await page.keyboard.press('Tab');
const tabPanel = await page.$('#tab-panel');
expect(await isActiveEl(tabPanel)).toBeTruthy();
await page.keyboard.press('Tab');
const tabContent = await page.$('#tab-content');
expect(await isActiveEl(tabContent)).toBeTruthy();
});
Expand Down
1 change: 0 additions & 1 deletion src/tabs-motion/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ interface TabOverrides {
}>;
TabPanel?: Override<{
$pad?: boolean;
$focusVisible?: boolean;
}>;
}

Expand Down
8 changes: 2 additions & 6 deletions src/tabs-motion/styled-components.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,9 @@ export const StyledTabHighlight = styled<{
},
);

export const StyledTabPanel = styled<{$pad: boolean, $focusVisible?: boolean}>(
export const StyledTabPanel = styled<{$pad: boolean}>(
'div',
({$theme, $pad = true, $focusVisible = false}) => {
({$theme, $pad = true}) => {
const style: StyleObject = {
flexGrow: 1, // only used in vertical orientation
outline: 'none',
Expand All @@ -234,10 +234,6 @@ export const StyledTabPanel = styled<{$pad: boolean, $focusVisible?: boolean}>(
style.paddingBottom = $theme.sizing.scale600;
style.paddingLeft = $theme.sizing.scale600;
}
if ($focusVisible) {
style.outline = `3px solid ${$theme.colors.accent}`;
style.outlineOffset = '-3px';
}
return style;
},
);
19 changes: 0 additions & 19 deletions src/tabs-motion/tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -453,36 +453,17 @@ function InternalTabPanel({
TabPanelOverrides,
StyledTabPanel,
);
// Keyboard focus styling
const [focusVisible, setFocusVisible] = React.useState(false);
const handleFocus = React.useCallback((event: SyntheticEvent<>) => {
if (isFocusVisible(event)) {
setFocusVisible(true);
}
}, []);
const handleBlur = React.useCallback(
(event: SyntheticEvent<>) => {
if (focusVisible !== false) {
setFocusVisible(false);
}
},
[focusVisible],
);
return (
<TabPanel
data-baseweb="tab-panel"
key={key}
role="tabpanel"
id={getTabPanelId(uid, key)}
aria-labelledby={getTabId(uid, key)}
tabIndex={isActive ? '0' : null}
aria-expanded={isActive}
hidden={!isActive}
$focusVisible={focusVisible}
{...sharedStylingProps}
{...TabPanelProps}
onFocus={forkFocus(TabPanelProps, handleFocus)}
onBlur={forkBlur(TabPanelProps, handleBlur)}
>
{isActive || renderAll ? children : null}
</TabPanel>
Expand Down
9 changes: 2 additions & 7 deletions src/utils/focusVisible.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,8 @@ export function teardown(doc) {

//$FlowFixMe
export function isFocusVisible(event) {
const {target, currentTarget} = event;

// Ensure nested handlers do not all return true
if (target !== currentTarget) return false;

try {
return target.matches(':focus-visible');
return event.target.matches(':focus-visible');
} catch (error) {
// browsers not implementing :focus-visible will throw a SyntaxError
// we use our own heuristic for those browsers
Expand All @@ -128,7 +123,7 @@ export function isFocusVisible(event) {

// no need for validFocusTarget check. the user does that by attaching it to
// focusable events only
return hadKeyboardEvent || focusTriggersKeyboardModality(target);
return hadKeyboardEvent || focusTriggersKeyboardModality(event.target);
}

/**
Expand Down

0 comments on commit 130570d

Please sign in to comment.