Remove #AppFrameMainContent link and update SkipToContent link to target #AppFrameMain instead #3912

henryyi · 2021-01-25T20:39:21Z

WHY are these changes introduced?

References https://github.com/Shopify/global-nav/issues/775

The "Skip to Content" link by default targets a hidden anchor element within Frame via #AppFrameMainContent. This presents an accessibility concern since the focus does not advance to actual visible content item. This is also undesirable when Voice Over is activated since it will cause it to focus on and describe the hidden anchor.
By targeting the main container (#AppFrameMain) instead the focus gets moved to the first focusable (or "voiceable") element inside of main.

WHAT is this pull request doing?

Remove the #AppFrameMainContent link from Frame. Update Skip to Content to target #AppFrameMain.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 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

ghost · 2021-01-25T20:39:25Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

github-actions · 2021-01-25T20:41:32Z

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

kyledurand

Tophat looks fine here. Can you build for web and test on shop1 as well?

See: testing in a consuming project
Basically just dev up in web and then run:
yarn run build-consumer web in polaris-react

src/components/Frame/Frame.tsx

kyledurand · 2021-01-25T20:53:04Z

src/components/Frame/Frame.tsx

-        ref={this.skipToMainContentTargetNode}
-        tabIndex={-1}
-      />
-    );


Removing this would be a bit of a breaking change. Are you sure we don't need this anymore?

Yeah this seems half-done

Either: there's still a need for a skipToContentTarget prop - in which case we need to do something with it OR it's not needed anymore, in which case we need to add a @deprecated docblock on the prop and documentation what has happened.

Is there still a use-case for a user-provided skipToMainContentTargetNode?

From what I can tell this is only used as a target for Skip To Content. I did a global search for AppFrameMainContent and only see it referenced in Polaris and GlobalNav (which I updated here https://github.com/Shopify/global-nav/pull/859 as well).
However with that said please let me know if there are other places I should check.

Also the reason this was removed is because focus was still being given to the anchor since it's technically a valid focusable element within <main>. So by removing it, focus should go to a visible element in the content.

@BPScott yes I left the prop since it was needed according to this PR #2080

Reading through this I think this change is fine. skipToContentTarget either points towards APP_FRAME_MAIN or the provided ref prop. This removes the private skipToMainContentTargetNode and the link that is rendered. Is the risk that someone incorrectly referenced that element?

henryyi · 2021-01-27T14:55:12Z

Minor update to remove the unused target ref. I also added preventDefault after focusing on custom targets so that it doesn't alter the hash.

I considered keeping the target ref and placing it on <main> and using focusFirstKeyboardFocusableNode to focus on an item in <main> when the skipToContent link is clicked. However this makes the Voice Over experience worst since it will skip over non-focusable elements that it would usually read. For example if we just link to #AppFrameMain then the screen reader will read the first <h1> in <main>.

Top hatted in shop1 on chrome, ff and safari (macOS)

mathildebuenerd

Looks nice!
🎩 in Storybook on the Frame in an application component with Voiceover, and it works as expected. After clicking the skip link we are moved to the first focusable element in the main, in this case the h1

BPScott · 2021-02-02T02:55:14Z

Removing myself from the reviewers list as I've moved off the polaris team

…get #AppFrameMain instead Allow focus to be placed on a focusable element in <main> instead of invisible link

…tentTarget is clicked

ghost · 2021-02-17T01:33:56Z

🎉 Thanks for your contribution to Polaris React!

…get #AppFrameMain instead (#3912) * Remove #AppFrameMainContent link and update SkipToContent link to target #AppFrameMain instead * Allow focus to be placed on a focusable element in <main> instead of invisible link * Remove unnecessary anchor and add preventDefault when custom skipToContentTarget is clicked

henryyi requested a review from kyledurand January 25, 2021 20:39

kyledurand reviewed Jan 25, 2021

View reviewed changes

kyledurand requested review from BPScott and alex-page January 25, 2021 20:55

henryyi force-pushed the hyi/775-skip-to-content-target branch from c828c14 to 14ed8a7 Compare January 25, 2021 20:57

mathildebuenerd reviewed Jan 29, 2021

View reviewed changes

BPScott removed their request for review February 2, 2021 02:54

alex-page approved these changes Feb 2, 2021

View reviewed changes

henryyi added 3 commits February 16, 2021 17:45

Remove #AppFrameMainContent link and update SkipToContent link to tar…

78bb4c5

…get #AppFrameMain instead Allow focus to be placed on a focusable element in <main> instead of invisible link

Remove unecessary anchor and add preventDefault when custom skipToCon…

5615671

…tentTarget is clicked

Update UNRELEASED.md

ed8f201

henryyi force-pushed the hyi/775-skip-to-content-target branch from 68d975d to ed8f201 Compare February 16, 2021 22:47

henryyi added the Accessibility Needs design, development, or content work relating to accessibility. label Feb 17, 2021

henryyi merged commit db429cd into main Feb 17, 2021

henryyi deleted the hyi/775-skip-to-content-target branch February 17, 2021 01:33

This was referenced Mar 18, 2021

Bump @shopify/polaris from 6.0.1 to 6.2.0 jonpulsifer/hub#81

Closed

Bump @shopify/polaris from 5.12.0 to 6.2.0 jonpulsifer/pulsifer.ca#101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove #AppFrameMainContent link and update SkipToContent link to target #AppFrameMain instead #3912

Remove #AppFrameMainContent link and update SkipToContent link to target #AppFrameMain instead #3912

henryyi commented Jan 25, 2021 •

edited

Loading

ghost commented Jan 25, 2021

github-actions bot commented Jan 25, 2021 •

edited

Loading

kyledurand left a comment

kyledurand Jan 25, 2021

BPScott Jan 25, 2021 •

edited

Loading

henryyi Jan 25, 2021

henryyi Jan 25, 2021 •

edited

Loading

henryyi Jan 25, 2021

alex-page Feb 2, 2021

henryyi commented Jan 27, 2021 •

edited

Loading

mathildebuenerd left a comment

BPScott commented Feb 2, 2021

ghost commented Feb 17, 2021

Remove #AppFrameMainContent link and update SkipToContent link to target #AppFrameMain instead #3912

Remove #AppFrameMainContent link and update SkipToContent link to target #AppFrameMain instead #3912

Conversation

henryyi commented Jan 25, 2021 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

ghost commented Jan 25, 2021

github-actions bot commented Jan 25, 2021 • edited Loading

kyledurand left a comment

Choose a reason for hiding this comment

kyledurand Jan 25, 2021

Choose a reason for hiding this comment

BPScott Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

henryyi Jan 25, 2021

Choose a reason for hiding this comment

henryyi Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

henryyi Jan 25, 2021

Choose a reason for hiding this comment

alex-page Feb 2, 2021

Choose a reason for hiding this comment

henryyi commented Jan 27, 2021 • edited Loading

mathildebuenerd left a comment

Choose a reason for hiding this comment

BPScott commented Feb 2, 2021

ghost commented Feb 17, 2021

henryyi commented Jan 25, 2021 •

edited

Loading

github-actions bot commented Jan 25, 2021 •

edited

Loading

BPScott Jan 25, 2021 •

edited

Loading

henryyi Jan 25, 2021 •

edited

Loading

henryyi commented Jan 27, 2021 •

edited

Loading