fix: add predicate function to configurable focus trap and allow focus to escape sidenav

[crisbeto]

Split up into the following commits:

1. Adds an option to the ConfigurableFocusTrap which can be used to pass in a predicate function that will determine whether focus is allowed to escape.
2. Uses the new option from the first commit to allow for focus to move between the sidenav and the sidenav container.

[mmalerba]

Nice, I like this approach

[mmalerba]

Name bikesheding: WDYT about allowFocusEscape?

[crisbeto]

I'm not too attached to the name, but I wanted to have "predicate" in there since we do the same in a few other places. To me, allowFocusEscape sounds like something you'd call on the focus trap to allow focus to escape, e.g. focusTrap.allowFocusEscape().

[jelbourn]

Looking at @mmalerba's issue report #21955, I'm not 100% sure that we don't want that behavior. For a sidenav with mode="over", my expectation is that the sidenav behaves like a modal dialog with focus completely trapped inside; clicking outside the nav should close it and restore focus like a dialog. For mode="push" I think it would probably be the same. Even though the sidenav doesn't take over the screen in the same way, the interaction seems like it should be the same.

cc @stevenyxu for thoughts

[mmalerba]

@jelbourn The weird thing is we have an API to disable the backdrop, so with that disabled it doesn't close and just feels broken

[jelbourn]

@mmalerba what if we changed the sidenav to close on outside click instead of backdrop click? I think we do that with dialog now, right?

[mmalerba]

I suspect there's a good number of folks disabling the backdrop so they can use it in over mode as a persistent sidenav (that's exactly what the team that brought this to my attention was doing). Making the change you suggest would break their use case.

We could try to make that change, I just don't have high hopes for it being something we could easily get in.

[stevenyxu]

I'm not 100% full on all the context here, but in general you just need to either be modal and trap focus or not. "over" without a backdrop is weird IMO, but if you have it you should decide whether it's modal. It really sounds like non-modal is the intent, and I'd agree the visual language implies that. In this case, why not disable the focus trap altogether instead of keeping it enabled with an escape hatch as in this PR?

[jelbourn]

I'd agree that if they're using it as a persistent sidenav (non-modal) then there should be no focus-trapping whatsoever. I suspect we could do a better job documenting this.

[jelbourn]

Does it make sense to have focus trapping enabled when autoFocus is false? I think a reasonable change here would be to disable the focus trap when autoFocus is off since without both of them this doesn't conform to expected modal interaction anyway.

[stevenyxu]

I can't speak directly to the sidenav case because I don't have experience with that widget, but in the dialog case, I would prefer not to bind focus trapping with requiring autoFocus. This is mainly because from our evaluation so far the current implementation autoFocus is too often "broken by default" that we are considering turning it off by default.

In the common case where user-provided cdkFocusInitial is not set, autoFocus sets focus on the first tabbable element

components/src/cdk/a11y/focus-trap/focus-trap.ts

Line 230 in 6cb0e03

return this.focusFirstTabbableElement(options);

This can skip over critical content that sits before it. A better default to avoid skipping over content is to focus on the whole container, the standard behavior when autoFocus is false. We also considered asking everyone to focus on the heading, but this causes double-announcing on many SRs (because the dialog is aria-labelledby the heading, so the SR announces both the heading and containing dialog's name i.e. the heading's name twice). In what I believe should be the uncommon case where users feel so strongly that some widget should receive initial focus (and care is taken to ensure that autofocusing doesn't skip over critical information like form instructions), they should just be able to use cdkFocusInitial. We're still in early stages in exploring how to actually execute on this change, but if we do it and it's stable, we'll investigate options to upstream it to Angular Components, though I suspect there will be headwinds due to the breakingness of the change if we choose to touch the default.

Back closer to the problem at hand, there are four-ish cases:

• autoFocus: true and modal dialog/sidenav (over and push): This is the most common use case and autoFocus is error-prone but technically fine in many cases.
• autoFocus: true and non-modal sidenav (side): Non-modal dialogs are controversial in general, but with the right thought this can be made to work and arguably should be supported. If we do, it seems better just to turn off the focus trap entirely rather than set a predicate system.
• autoFocus: false and modal dialog/sidenav: This is what we're converging to at this moment, and I'd like to keep support for this if possible.
• autoFocus: false and non-modal sidenav: Same thing as above with non-modal dialogs. Kinda finicky, but can be fine if you make sure now that there's a good logical mechanism to send focus to/from the sidenav.

[jelbourn]

The autoFocus for sidenav unfortunately works differently from MatDialog; if it's off, it just doesn't do anything.

Here's what I propose:

• Introduce a new sidenav option called "modal". When this is set, we fully treat the sidenav as a modal role="dialog" element. When this is true, autoFocus works the same way as MatDialog.
• Default modal to true for mode="over" and "mode="push". Default modal to false for mode="side".

[mmalerba]

it sounds like we probably just want to close this one?

[jelbourn]

I would be inclined to close this and take a different approach.

[angular-automatic-lock-bot]

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.}