feat(multiple): add options to autoFocus field for dialogs #22780
Conversation
google-cla
bot
added
the
cla: yes
| @@ -20,7 +20,7 @@ export declare const matBottomSheetAnimations: { | |||
|
|
|||
| export declare class MatBottomSheetConfig<D = any> { | |||
| ariaLabel?: string | null; | |||
| autoFocus?: boolean; | |||
| autoFocus?: string; | |||
There was a problem hiding this comment.
We can't just change this to a string since that's a breaking change. Instead, make it boolean | string and map boolean values to the behavior that most closely matches what they did prior to this change.
You can add a comment like:
/** @breaking-change 13.0.0 remove boolean from type */to remind us that we can remove the boolean support 2 versions from now (in alignment with our policy for making breaking changes)
There was a problem hiding this comment.
Additionally, the new type wouldn't be string, but instead include the enum type:
autoFocus?: AutoFocusTarget | string | boolean;I know that AutoFocusTarget is a subtype of string, but adding the enum should improve IDE autocomplete
The removed version would also be v14 at this point
There was a problem hiding this comment.
I ended up adding the comment in these .d.ts files. Although in the code I mainly saw them in .ts files. Is there any convention on where they should go?
There was a problem hiding this comment.
The comment should go in the actual source; these .d.ts files in public_api_guard are automatically generated, so any changes there will be clobbered.
For context: when a TypeScript file is compiled, it generates the .js output file and a .d.ts type definition file. The type definition is produced so that other TypeScript code can type check against it without having to compile everything all together. We take of the type definition file with our public_api_guards to make sure we don't make unintentional changes to the public api. There's a test that compares the existing public api .d.ts file to the one from the PR and fails if it changed, so that any intentional changes have to be explicitly approved.
| @@ -20,7 +20,7 @@ export declare const matBottomSheetAnimations: { | |||
|
|
|||
| export declare class MatBottomSheetConfig<D = any> { | |||
| ariaLabel?: string | null; | |||
| autoFocus?: boolean; | |||
| autoFocus?: string; | |||
There was a problem hiding this comment.
Additionally, the new type wouldn't be string, but instead include the enum type:
autoFocus?: AutoFocusTarget | string | boolean;I know that AutoFocusTarget is a subtype of string, but adding the enum should improve IDE autocomplete
The removed version would also be v14 at this point
| @@ -26,6 +26,15 @@ import {take} from 'rxjs/operators'; | |||
| import {InteractivityChecker} from '../interactivity-checker/interactivity-checker'; | |||
|
|
|||
|
|
|||
| /** Options for how to handle focus upon opening the dialog */ | |||
| export const enum AutoFocusOption { | |||
There was a problem hiding this comment.
I would name this AutoFocusTarget
| if (!this._checker.isFocusable(element)) { | ||
| element.tabIndex = -1; | ||
| // The tabindex attribute should be removed to avoid navigating to that element again | ||
| element.addEventListener('blur', () => element.removeAttribute('tabindex')); |
There was a problem hiding this comment.
This handler should be added outside of the Angular zone. You can look through the codebase for runOutsideAngular for an example.
amysorto
force-pushed
the
auto-focus-options
branch
2 times, most recently
from
14313aa to
ac27351
Compare
amysorto
force-pushed
the
auto-focus-options
branch
5 times, most recently
from
c12cb53 to
8fcac0d
Compare
| */ | ||
| private _autoFocusFirstTabbableElement() { | ||
| private _forceFocus(element: HTMLElement, options?: FocusOptions) { |
There was a problem hiding this comment.
Does it make sense to move this out into an a11y helper? We have an identical function in our g3 app.
EDIT: Plus I now see there are multiple identical implementations throughout this PR.
There was a problem hiding this comment.
My understanding is that these methods will exist in cdk/dialog once that's created in a future PR
There was a problem hiding this comment.
Yeah, I told Amy to do it this way since cdk/dialog is the correct place for this logic. It just has the unfortunate property of not existing yet.
| */ | ||
| private _autoFocusFirstTabbableElement() { | ||
| private _forceFocus(element: HTMLElement, options?: FocusOptions) { | ||
| if (!this._interactivityChecker.isFocusable(element)) { |
There was a problem hiding this comment.
Instead of checking if the element is focusable, we could just call element.focus(), then check to see if element === document.activeElement. If not, then apply tabindex = "-1" and focus again. WDYT?
There was a problem hiding this comment.
That makes sense. Although it seems strange to have different functionality for the same thing. @jelbourn do you have thoughts on this?
There was a problem hiding this comment.
I would probably stick with using InteractivityChecker mostly for consistency, but also because I vaguely recall that Firefox will sometimes(?) not actually focus an element until the next microtask tick.
amysorto
force-pushed
the
auto-focus-options
branch
from
8fcac0d to
76beab4
Compare
jelbourn
added
Accessibility
|
@amysorto needs rebase |
amysorto
force-pushed
the
auto-focus-options
branch
from
76beab4 to
059dbde
Compare
mmalerba
force-pushed
the
auto-focus-options
branch
from
059dbde to
bce7b34
Compare
Before this PR, autoFocus was a boolean that allowed users to specify whether the container element or the first tabbable element is focused on dialog open. Now you can also specify focusing the first header element or use a CSS selector and focus the first element that matches that. If these elements can't be focused, then the container element is focused by default. This applies to other components that are similar to dialog and also have a autoFocus field. Fixes angular#22678
amysorto
force-pushed
the
auto-focus-options
branch
from
bce7b34 to
2e7b61b
Compare
|
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. |
Before this PR, autoFocus was a boolean that allowed users to specify whether the container element or the first tabbable element is focused on dialog open. Now you can also specify focusing the first header element or use a CSS selector and focus the first element that matches that. If these elements can't be focused, then the container element is focused by default.
This applies to other components that are similar to dialog and also have a autoFocus field.