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

[Tabs] Fix Tabs focus state issue on close #3994

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

lhoffbeck
Copy link
Contributor

@lhoffbeck lhoffbeck commented Feb 11, 2021

WHY are these changes introduced?

Fixes #3993

(also fixes shopify/web issue #37890)

When the Popover component is handling the close event, it checks for the next focuseable element and focuses that, and otherwise focuses the popover trigger. However, the "next focuseable element" is determined by the nextFocuseableNode utility, which just checks based on element type and doesn't take tabindex values into consideration.

This breaks when used by the Tabs component, because Tabs has a set of buttons with tabindex="-1" that are used to determine appropriate widths when resizing (the TabMeasurer component). Because of this, nextFocuseableNode is determined to be the first of these TabMeasurer buttons, but since it has tabindex="-1" the focus is shifted to the window.

👀 Before 👀

bad.focus.state.mp4

WHAT is this pull request doing?

This PR moves the TabMeasurer's hidden tabs before the visible tabs. **Probably the better fix would be to update the focus utility to exclude buttons with negative tabindex values, but given that this is a core utility that could have unintended knock-on effects. Hoping one of the PR reviewers has more insight into this 🙏 **

👀 After 👀

focus.self.mp4
has.additional.focus.mp4

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Button, Page, Tabs} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <TabsExample />
    </Page>
  );
}

function TabsExample() {
  const tabs = [
    {
      id: 'all-customers-1',
      content: 'All',
      accessibilityLabel: 'All customers',
      panelID: 'all-customers-content-1',
    },
    {
      id: 'accepts-marketing-1',
      content: 'Accepts marketing',
      panelID: 'accepts-marketing-content-1',
    },
    {
      id: 'repeat-customers-1',
      content: 'Repeat customers',
      panelID: 'repeat-customers-content-1',
    },
    {
      id: 'prospects-1',
      content: 'Prospects',
      panelID: 'prospects-content-1',
    },
    {
      id: 'prospects-2',
      content: 'Prospects 2',
      panelID: 'prospects-content-2',
    },
    {
      id: 'prospects-3',
      content: 'Prospects 3',
      panelID: 'prospects-content-3',
    },
    {
      id: 'prospects-4',
      content: 'Prospects 4',
      panelID: 'prospects-content-4',
    },
    {
      id: 'prospects-5',
      content: 'Prospects 5',
      panelID: 'prospects-content-5',
    },
    {
      id: 'prospects-6',
      content: 'Prospects 6',
      panelID: 'prospects-content-6 ',
    },
    {
      id: 'prospects-7',
      content: 'Prospects really long name so this wraps',
      panelID: 'prospects-content-7',
    },
  ];

  return (
    <div>
      <Button>test</Button>
      <Tabs tabs={tabs} selected={0} onSelect={() => {}} />
    </div>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2021

🟢 No significant changes to src/**/*.tsx were detected.

Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

LGTM 🎩 🎉

Update tests to point at visible tabs

fix linting issues
@lhoffbeck lhoffbeck force-pushed the 3993-fix-focus-state-on-tabs-disclosure-close branch from ea9ce29 to c7def16 Compare February 16, 2021 16:33
fix merge issue
@lhoffbeck lhoffbeck merged commit 70a7ae7 into main Feb 16, 2021
@lhoffbeck lhoffbeck deleted the 3993-fix-focus-state-on-tabs-disclosure-close branch February 16, 2021 17:27
sylvhama pushed a commit that referenced this pull request Mar 26, 2021
* 3993 - Fix Tabs focus state issue on close
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tabs] [A11y] Fix focus issue after closing disclosure popover
2 participants