Skip to content

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Jul 11, 2019

WHY are these changes introduced?

In this PR: #1785 We added focus to the <main> element by adding tabIndex={-1}. This broke the Polaris-rails popover. In rails clicking the popover sends the target and hides the popover if the target isn't inside a popover. Adding the tabIndex on the main element made it the target, therefore always closed the popover.

Fixes #1816

WHAT is this pull request doing?

Adding an <a> tag at the beginning of the content and focusing it instead.

How to 🎩

To test this properly it is best to do it web.

  1. Start web
  2. In polaris-react: yarn build-consumer web.
  3. Tab to the skip to content link and click it. You should skip to the content. Also, use voice over to ensure the correct text is being read.
Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        {/* Add the code you want to test here */}
      </Page>
    );
  }
}

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-1821 July 11, 2019 19:45 Inactive
@dleroux dleroux changed the title [Skip to content] Fix tabIndex issue [WIP] [Skip to content] Fix tabIndex issue Jul 11, 2019
@dleroux dleroux force-pushed the fix-skip-to-content branch from a39b943 to 9b1b56d Compare July 11, 2019 20:05
@BPScott BPScott temporarily deployed to polaris-react-pr-1821 July 11, 2019 20:05 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1821 July 11, 2019 20:30 Inactive
@dleroux dleroux changed the title [WIP] [Skip to content] Fix tabIndex issue [Skip to content] Fix tabIndex issue Jul 11, 2019
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Tested in web locally and it works 🎉

@dleroux dleroux force-pushed the fix-skip-to-content branch from cb15e8c to 35e8cbd Compare July 12, 2019 16:13
@BPScott BPScott temporarily deployed to polaris-react-pr-1821 July 12, 2019 16:14 Inactive
@dleroux dleroux merged commit 87f5965 into master Jul 12, 2019
@kaelig kaelig deleted the fix-skip-to-content branch July 12, 2019 21:36
// eslint-disable-next-line jsx-a11y/anchor-is-valid
<a
id={APP_FRAME_MAIN_ANCHOR_TARGET}
ref={this.skipoToMainContentTargetNode}
Copy link
Contributor

@jgodson jgodson Jul 15, 2019

Choose a reason for hiding this comment

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

skipoToMainContentTargetNode extra o in here (skipo)

@dleroux dleroux temporarily deployed to production July 16, 2019 16:11 Inactive
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.

v3.19.0 interop regression in web

3 participants