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

fix(page): skip to content should point to primary content container #2519

Merged
merged 5 commits into from Jul 26, 2019

Conversation

@seanforyou23
Copy link
Member

seanforyou23 commented Jul 15, 2019

What: Currently, skip to content link sends users to an in-page anchor that sits alongside the main page content container. This generally has the desired effect for sighted keyboard users, however, for screen reader users (SRU) this basically leads to a dead end. It's a dead-end in that the target anchor doesn't contain the page content, so nothing further is announced after the element has been focused.

I think what would be better is if we allow the main content container to carry an id, which can be used as the target of the SkipToContent component. Doing this along with supplying a tabindex="-1" for the main content area gives us a solid hook for informing SRU of general route changes and informing them of what content is now available.

For sighted users, I don't think this will cause any difference in behavior (we may want to add a style like outline: none to the primary page container, although I don't recommend this.

Additional issues: #2591

These changes are driven by a technique suggested in https://dequeuniversity.com where in response to dynamic content changes (like navigating to a new "Page" in a SPA) we want to send focus to the new content so that it is announced and SRU receive some feedback about the new content.

Copy link

matthewcarleton left a comment

LGTM! We should also ensure this gets updated in core and documented on the site.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 15, 2019

PatternFly-React preview: https://patternfly-react-pr-2519.surge.sh

Copy link
Collaborator

jschuler left a comment

Looks good, can you also update the page demos

@seanforyou23 seanforyou23 force-pushed the seanforyou23:navigation-enhancements branch from 67ea462 to 6a1458f Jul 23, 2019
@seanforyou23 seanforyou23 requested review from jschuler, dlabaj and redallen Jul 23, 2019
@seanforyou23 seanforyou23 requested a review from matthewcarleton Jul 23, 2019
Copy link
Collaborator

jgiardino left a comment

This is awesome! Thanks for all your work researching this piece and contributing it back.

Copy link
Contributor

tlabaj left a comment

Looks great so far @seanforyou23 do you mind opening a React issue for tracking purposes.

I had one question. If mainContainerId is not set by consumer, does this work as expected? Maybe we need to add in a test for that?

@seanforyou23 seanforyou23 dismissed stale reviews from jgiardino and redallen via 15b8282 Jul 25, 2019
@seanforyou23

This comment has been minimized.

Copy link
Member Author

seanforyou23 commented Jul 25, 2019

@tlabaj good catch, I updated the tests to reflect what we can expect in the current state. If users don't supply a mainContainerId then things will not work as expected. I'd like to make it a required prop at the next available opportunity for breaking changes.

@seanforyou23

This comment has been minimized.

Copy link
Member Author

seanforyou23 commented Jul 25, 2019

I also fixed this error from the Page test

Screen Shot 2019-07-25 at 1 05 40 PM

@seanforyou23 seanforyou23 requested review from tlabaj, redallen and jgiardino Jul 25, 2019
@dlabaj
dlabaj approved these changes Jul 25, 2019
@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Jul 25, 2019

@seanforyou23 let's open a follow up issue to make that required so we can pull that into the next breaking change release.

@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Jul 26, 2019

@seanforyou23 I opened issue #2597 for the breaking change.
Can you open an issue in core to make the updates that @matthewcarleton mentioned please.

@tlabaj
tlabaj approved these changes Jul 26, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@redallen redallen dismissed jschuler’s stale review Jul 26, 2019

Demos updated.

@redallen redallen merged commit 1f709bd into patternfly:master Jul 26, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 26, 2019

Your changes have been released in:

  • @patternfly/react-core@3.77.2
  • @patternfly/react-docs@4.9.8
  • @patternfly/react-inline-edit-extension@2.9.54
  • demo-app-ts@2.13.2
  • @patternfly/react-integration@2.13.1
  • @patternfly/react-table@2.15.2
  • @patternfly/react-topology@2.7.3
  • @patternfly/react-virtualized-extension@1.1.87

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.