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

feat(cdk/focus-trap) Add ConfigurableFocusTrap classes #18201

Merged
merged 1 commit into from Jan 24, 2020

Conversation

vanessanschmitt
Copy link
Collaborator

@vanessanschmitt vanessanschmitt commented Jan 16, 2020

ConfigurableFocusTrap is part of a new FocusTrap design that will trap more
than just tab focus.

This commit sets up the classes for the new design, and implements the primary
strategy for preventing focus outside the trap (an event listener that
refocuses the trap). Logic to trap screen reader focus and wrap tab
without the hidden tab stops will be in future commits. Migration of
cdkTrapFocus, MatDialog, etc. will also be in future commits.

This commit does not enable ConfigurableFocusTrap anywhere.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 16, 2020
@vanessanschmitt
Copy link
Collaborator Author

@stevenyxu Could you take a look as well?

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

I'm going to re-target this PR onto a branch called focus-trap-v2 so we can get everything in before merging it into master

src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/configurable-focus-trap.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Show resolved Hide resolved
@jelbourn jelbourn changed the base branch from master to focus-trap-v2 January 17, 2020 23:36
@vanessanschmitt
Copy link
Collaborator Author

Relates to #13054

src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/configurable-focus-trap.spec.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/configurable-focus-trap.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/event-listener-inert-strategy.ts Outdated Show resolved Hide resolved
// Don't refocus if target was in an overlay, because the overlay might be associated
// with an element inside the FocusTrap, ex. mat-select.
if (!focusTrap.element.contains(target) &&
target.closest('div.cdk-overlay-pane') === null) {
Copy link
Member

Choose a reason for hiding this comment

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

Closest isn't supported in some browsers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I saw I failed some browser tests for that reason. I copied the polyfill from here: https://github.com/angular/components/blob/master/src/cdk-experimental/popover-edit/polyfill.ts

into my own polyfill.ts file. Is that okay, or should I move the popover-edit polyfill code somewhere it can be shared?

src/cdk/a11y/focus-trap/event-listener-inert-strategy.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/event-listener-inert-strategy.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Outdated Show resolved Hide resolved
src/cdk/a11y/focus-trap/focus-trap-manager.ts Outdated Show resolved Hide resolved
@vanessanschmitt
Copy link
Collaborator Author

I'm going to re-target this PR onto a branch called focus-trap-v2 so we can get everything in before merging it into master

The next commit I was planning was to migrate MatDialog to use ConfigurableFocusTrap. I created cl/289706346 (internal to Google) to try running all the presubmits, and they look pretty good. Should I do things in a different order?

These were the four ConfigurableFocusTrap implementation commits I was planning:

  1. Implement FocusTrapManager and EventListenerFocusTrapInertStrategy.
  2. Update logic to find and cache first/last focusable elements, add document Tab event listener, and remove hidden tab stops.
  3. Implement AriaHiddenScreenReaderStrategy.
  4. Implement DisabledFocusTrapInertStrategy.

I was going to migrate MatDialog (and hopefully cdkTrapFocus, pending what we decide to do about directly referencing EventListenerFocusTrapInertStrategy) after commit 1. Mainly because it fixes a known issue with MatDialog (#13054), which isn't dependent on commits 2-4.

Commit 2 shouldn't be too much code, so we could also migrate after that one. Commit 3 is more code, and MatDialog won't be using that strategy anyways for the time being, because it already has its own logic to apply aria-hidden. Commit 4 is still pending more investigation as to whether it's worth the work of implementing, given the expected bad performance.

What do you think would be a good point to merge into master?

@vanessanschmitt vanessanschmitt force-pushed the focus-trap-1 branch 7 times, most recently from 1300c13 to c422d1b Compare January 22, 2020 01:31
@jelbourn jelbourn changed the base branch from focus-trap-v2 to master January 22, 2020 01:32
@jelbourn jelbourn added target: minor This PR is targeted for the next minor release and removed target: development-branch labels Jan 22, 2020
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Ah, I didn't know you were planning to work on dialog right after this. In that case we can retarget this for master. My thinking was that we would want to have the other strategies done before releasing this publically, and developing on a branch would remove any time pressure; I'm not super worried about it, though, since master is targeting 9.1 and we still haven't finalized 9.0 yet, so there's should be plenty of time.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

I'll run this through TGP tonight

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 22, 2020
ConfigurableFocusTrap is part of a new FocusTrap design that will trap more
than just tab focus.

This commit sets up the classes for the new design, and implements the primary
strategy for preventing focus outside the trap (an event listener that
refocuses the trap). Logic to trap screen reader focus and wrap tab
without the hidden tab stops will be in future commits. Migration of
cdkTrapFocus, MatDialog, etc. will also be in future commits.

This commit does not enable ConfigurableFocusTrap anywhere.
@jelbourn jelbourn merged commit 4e4e0e8 into angular:master Jan 24, 2020
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
ConfigurableFocusTrap is part of a new FocusTrap design that will trap more
than just tab focus.

This commit sets up the classes for the new design, and implements the primary
strategy for preventing focus outside the trap (an event listener that
refocuses the trap). Logic to trap screen reader focus and wrap tab
without the hidden tab stops will be in future commits. Migration of
cdkTrapFocus, MatDialog, etc. will also be in future commits.

This commit does not enable ConfigurableFocusTrap anywhere.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants