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.

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 17, 2021
+12 −24
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78bb4c5
Remove #AppFrameMainContent link and update SkipToContent link to tar…
henryyi Jan 25, 2021
5615671
Remove unecessary anchor and add preventDefault when custom skipToCon…
henryyi Jan 27, 2021
ed8f201
Update UNRELEASED.md
henryyi Feb 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions UNRELEASED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

- Added `focus-visible` polyfill and default styles ([#3695](https://github.com/Shopify/polaris-react/pull/3695))
- Added `removeUnderline` prop to `Button` to remove underline when `plain` and `monochrome` are true([#3998](https://github.com/Shopify/polaris-react/))pull/3998)
- Removed `#AppFrameMainContent` link and updated SkipToContent link to target `#AppFrameMain` instead ([#3912](https://github.com/Shopify/polaris-react/pull/3912))

### Bug fixes

Expand Down
28 changes: 8 additions & 20 deletions src/components/Frame/Frame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {PureComponent, createRef} from 'react';
import React, {PureComponent, createRef, MouseEvent} from 'react';
import {MobileCancelMajor} from '@shopify/polaris-icons';
import {durationSlow} from '@shopify/polaris-tokens';
import {CSSTransition} from 'react-transition-group';
Expand Down Expand Up @@ -62,9 +62,6 @@ interface State {
const GLOBAL_RIBBON_CUSTOM_PROPERTY = '--global-ribbon-height';

const APP_FRAME_MAIN = 'AppFrameMain';

const APP_FRAME_MAIN_ANCHOR_TARGET = 'AppFrameMainContent';

const APP_FRAME_NAV = 'AppFrameNav';
const APP_FRAME_TOP_BAR = 'AppFrameTopBar';
const APP_FRAME_LOADING_BAR = 'AppFrameLoadingBar';
Expand All @@ -81,8 +78,6 @@ class FrameInner extends PureComponent<CombinedProps, State> {
private contextualSaveBar: ContextualSaveBarProps | null = null;
private globalRibbonContainer: HTMLDivElement | null = null;
private navigationNode = createRef<HTMLDivElement>();
private skipToMainContentTargetNode =
this.props.skipToContentTarget || createRef<HTMLAnchorElement>();

componentDidMount() {
this.handleResize();
Expand Down Expand Up @@ -216,7 +211,7 @@ class FrameInner extends PureComponent<CombinedProps, State> {

const skipTarget = skipToContentTarget?.current
? skipToContentTarget.current.id
: APP_FRAME_MAIN_ANCHOR_TARGET;
: APP_FRAME_MAIN;

const skipMarkup = (
<div className={skipClassName}>
Expand Down Expand Up @@ -252,15 +247,6 @@ class FrameInner extends PureComponent<CombinedProps, State> {
/>
) : null;

const skipToMainContentTarget = skipToContentTarget ? null : (
// eslint-disable-next-line jsx-a11y/anchor-is-valid
<a
id={APP_FRAME_MAIN_ANCHOR_TARGET}
ref={this.skipToMainContentTargetNode}
tabIndex={-1}
/>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@BPScott BPScott Jan 25, 2021 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@henryyi henryyi Jan 25, 2021 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?


const context = {
showToast: this.showToast,
hideToast: this.hideToast,
Expand Down Expand Up @@ -288,7 +274,6 @@ class FrameInner extends PureComponent<CombinedProps, State> {
id={APP_FRAME_MAIN}
data-has-global-ribbon={Boolean(globalRibbon)}
>
{skipToMainContentTarget}
<div className={styles.Content}>{children}</div>
</main>
<ToastManager toastMessages={toastMessages} />
Expand Down Expand Up @@ -379,9 +364,12 @@ class FrameInner extends PureComponent<CombinedProps, State> {
this.setState({skipFocused: false});
};

private handleClick = () => {
this.skipToMainContentTargetNode.current &&
this.skipToMainContentTargetNode.current.focus();
private handleClick = (event: MouseEvent<HTMLAnchorElement>) => {
const {skipToContentTarget} = this.props;
if (skipToContentTarget && skipToContentTarget.current) {
skipToContentTarget.current.focus();
event?.preventDefault();
}
};

private handleNavigationDismiss = () => {
Expand Down
7 changes: 3 additions & 4 deletions src/components/Frame/tests/Frame.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,12 @@ describe('<Frame />', () => {
expect(skipToContentLinkText).toStrictEqual('Skip to content');
});

it('sets focus to the main content target anchor element when the skip to content link is clicked', () => {
it('targets the main container element by default', () => {
const frame = mountWithApp(<Frame />);
const skipLink = frame.find('a', {children: 'Skip to content'});

skipLink!.trigger('onClick');
expect(document.activeElement).toBe(
frame.find('a', {id: 'AppFrameMainContent'})!.domNode,
expect(skipLink!.domNode!.getAttribute('href')).toBe(
`#${frame!.find('main')!.domNode!.id}`,
);
});

Expand Down