feat(dialog): add ariaLabel and focusOnOpen config options #6558
Conversation
crisbeto
added
Accessibility
googlebot
added
the
cla: yes
src/lib/dialog/dialog-config.ts
Outdated
| * Selector for an element to be focused when the dialog is opened. If omitted or the | ||
| * element is not found, the dialog will focus the first focusable element. | ||
| */ | ||
| focusOnOpen?: string | null = null; |
There was a problem hiding this comment.
With this API, it's not immediately clear what selector you'd set here to focus the root dialog element.
There was a problem hiding this comment.
Hmm not sure how we could remedy that. Perhaps supporting :host to align with Angular?
There was a problem hiding this comment.
Is there a strong reason to use a selector at all here since the focus trap already supports marking where you want the focus to start? The main thing that's missing is being able to focus the root element, which could just be a boolean.
There was a problem hiding this comment.
My reasoning to go for a selector instead of the boolean was to make it more flexible so we don't have to tweak it if somebody wants to focus something different from the container. E.g. this allows for the selector to be constructed at runtime, whereas the focus trap attribute needs to be set in advance.
There was a problem hiding this comment.
Maybe two properties, then? Something like focusChild for selector and focusRoot?
There was a problem hiding this comment.
Hmm I'm iffy about having two flags that do more or less the same. What about typing it something like 'root' | string | null. That way it should show up in docs and IDEs that root is a special option.
There was a problem hiding this comment.
I'm not a big fan of magic-string APIs. I'd rather go with boolean | string | null, where the boolean means the host element.
What do you think of making this PR just add the ariaLabel and the focusOnOpen for the root element while leaving the content focusing as a responsibility of the focus trap?
crisbeto
force-pushed
the
dialog-aria-label
branch
from
a110485
to
db9d0db
Compare
|
Rebased and reworked the PR so the |
src/lib/dialog/dialog-config.ts
Outdated
| ariaLabel?: string | null = null; | ||
|
|
||
| /** Whether the dialog should focus the first focusable element on open. */ | ||
| focusOnOpen?: boolean = true; |
There was a problem hiding this comment.
Coming back again- maybe autoFocus instead? That's what I went with for the focus trap directive
| @@ -81,6 +81,11 @@ export class MatDialogConfig<D = any> { | |||
| /** ID of the element that describes the dialog. */ | |||
| ariaDescribedBy?: string | null = null; | |||
|
|
|||
| /** Aria label to assign to the dialog element */ | |||
There was a problem hiding this comment.
A thought occurred to me (for a follow-up PR if you agree): to make the focus go to the root dialog element, we can just expose a tabIndex property on the dialog config. If it's 0, then it's the first focusable element. Thoughts?
There was a problem hiding this comment.
I don't think that would work since the interactivity checker starts looking from the children. Also we always focus the dialog anyway in order to prevent the user from opening multiple dialogs while animating.
crisbeto
force-pushed
the
dialog-aria-label
branch
from
db9d0db
to
42ac121
Compare
|
Addressed the feedback @jelbourn. |
jelbourn
added
pr: lgtm
action: merge
Based on the discussion on angular#6360 (comment), these changes add the ability to set the `aria-label` of a dialog, as well as the element that should be focus when the dialog is opened.
crisbeto
force-pushed
the
dialog-aria-label
branch
from
42ac121
to
d3000e8
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. |
Based on the discussion on #6360 (comment), these changes add the ability to set the
aria-labelof a dialog, as well as the element that should be focus when the dialog is opened.