feat(cdk/focus-trap) Add ConfigurableFocusTrap classes #18201
Conversation
googlebot
added
the
cla: yes
vanessanschmitt
force-pushed
the
focus-trap-1
branch
from
6b0bea0 to
003ecb5
Compare
|
@stevenyxu Could you take a look as well? |
vanessanschmitt
force-pushed
the
focus-trap-1
branch
3 times, most recently
from
b0150cb to
e81545f
Compare
vanessanschmitt
force-pushed
the
focus-trap-1
branch
from
e81545f to
0a5d097
Compare
|
Relates to #13054 |
| // 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) { |
There was a problem hiding this comment.
Closest isn't supported in some browsers.
There was a problem hiding this comment.
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?
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:
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
force-pushed
the
focus-trap-1
branch
7 times, most recently
from
1300c13 to
c422d1b
Compare
jelbourn
added
target: minor
There was a problem hiding this comment.
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.
jelbourn
added
pr: lgtm
action: merge
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.
vanessanschmitt
force-pushed
the
focus-trap-1
branch
from
c422d1b to
d9dd65c
Compare
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.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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.