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

Manage tab order and focus placement with Overlay #728

Merged
merged 4 commits into from
Jul 14, 2020
Merged

Manage tab order and focus placement with Overlay #728

merged 4 commits into from
Jul 14, 2020

Conversation

Westbrook
Copy link
Contributor

@Westbrook Westbrook commented Jun 22, 2020 •
edited
Loading

Description

  • adds inline and modal style overlays as described in Throw/Manage focus when an overlay is open #710 (comment)
  • adds focus throwing via receivedFocus
  • adds tab trapping via composed shadow DOM
  • leverages new API in sp-dropdown and extension
    • updates timing to not include arbitrary requestAnimationFrame calls
  • adds stories and tests to the functionality
    • sets overlay story element to "submit" on enter press
  • updates sp-radio-group to appropriately manage keyboard interactions

Demo available at: https://westbrook-overlay-focus--spectrum-web-components.netlify.app/storybook/index.html?path=/story/action-menu--icon-only

Related Issue

fixes #709
fixes #710
fixes #723
fixes #727

Motivation and Context

  • increase the keyboard accessibility of our components and UI powered by our APIs
  • code cleanliness

How Has This Been Tested?

  • added unit testing
  • updated screenshot testing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch from 76fe2c1 to 2e8ea74 Compare June 22, 2020 21:29
@Westbrook Westbrook mentioned this pull request Jun 22, 2020
Closed
@Westbrook Westbrook changed the title Westbrook/overlay focus Manage tab order and focus placement with Overlay Jun 22, 2020
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch 5 times, most recently from 6a83090 to db62b88 Compare June 23, 2020 00:47
@Westbrook Westbrook requested a review from cuberoot June 23, 2020 01:21
@Westbrook Westbrook mentioned this pull request Jun 26, 2020
Closed
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch 3 times, most recently from 36e68ca to 1c494cd Compare July 6, 2020 19:04
@Westbrook Westbrook mentioned this pull request Jul 6, 2020
Closed
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch 2 times, most recently from 45e03cb to 0f3599b Compare July 8, 2020 15:01
andrewhatran
andrewhatran reviewed Jul 8, 2020
View reviewed changes
packages/overlay/src/overlay-stack.ts
private overlayHolder!: HTMLElement;

private initTabTrapping(): void {
this.document.body.attachShadow({ mode: 'open' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about the body shadowRoot collision issue that we discussed offline.

What are your thoughts on cancelling the tabTrapping initiation if a shadowRoot already exists in the body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a bad idea for safety... document that users with other tools leveraging a shadowRoot on the <body> element just don't get the trapping functionality. Certainly better than just going and breaking apps that have this for some reason.

andrewhatran
andrewhatran reviewed Jul 8, 2020
View reviewed changes
packages/overlay/src/overlay-stack.ts
@@ -142,7 +230,7 @@ export class OverlayStack {

private async hideAndCloseOverlay(
overlay?: ActiveOverlay,
animated = true
animated?: boolean
Copy link
Collaborator

@andrewhatran andrewhatran Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a distinction between animated: false and animated: undefined for overlays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, yes. The overlay.hide(animated); that receives this argument also does animated = true in its method signature, which means that undefined in this method actually means true as it did before when given a default, only this way we can get test coverage on both lines...

@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch 3 times, most recently from 0e78aa7 to e1bf93c Compare July 11, 2020 00:00
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch from e1bf93c to ed32c9b Compare July 13, 2020 01:35
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch from ed32c9b to b69c0c5 Compare July 13, 2020 17:09
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch from b69c0c5 to 4ffef31 Compare July 13, 2020 17:30
andrewhatran
andrewhatran reviewed Jul 13, 2020
View reviewed changes
packages/overlay/src/overlay-stack.ts
private initTabTrapping(): void {
/* istanbul ignore if */
if (this.document.body.shadowRoot) {
this.canTabTrap = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

@andrewhatran andrewhatran self-requested a review July 13, 2020 20:19
andrewhatran
andrewhatran previously approved these changes Jul 13, 2020
View reviewed changes
Copy link
Collaborator

@andrewhatran andrewhatran left a comment

Choose a reason for hiding this comment

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

Lgtm 🥇

@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch 3 times, most recently from c2adf10 to 1593130 Compare July 13, 2020 23:18
@Westbrook Westbrook force-pushed the westbrook/overlay-focus branch from 1593130 to d20458b Compare July 14, 2020 00:00
@Westbrook Westbrook merged commit d9e0776 into main Jul 14, 2020
@Westbrook Westbrook deleted the westbrook/overlay-focus branch July 14, 2020 00:25
@Westbrook Westbrook mentioned this pull request Jul 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewhatran andrewhatran andrewhatran left review comments

@yalagand yalagand Awaiting requested review from yalagand

@msdewey msdewey Awaiting requested review from msdewey

@adixon-adobe adixon-adobe Awaiting requested review from adixon-adobe

@cuberoot cuberoot Awaiting requested review from cuberoot

Assignees
No one assigned
Labels
a11y Issues related to accessibility Component: Action Menu Component: Overlay Component: Picker
Projects
None yet
Milestone
No milestone
2 participants
@Westbrook @andrewhatran