-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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(dialog): add focus management #1321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a few typos
// the dialog was opened. Wait for the DOM to finish settling before changing the focus so | ||
// that it doesn't end up back on the <body>. | ||
this._ngZone.onMicrotaskEmpty.first().subscribe(() => { | ||
(this._elementFocusedBeforeDialogWasOpened as HTMLElement).focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer as HTMLElement
to (<HTMLElement>this.whatever).focus()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know the as
syntax existed until recently; I do this it is a bit cleaner and more in keeping with TypeScript's normally putting the type at the end. We can chat about it when we're all back in the office.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't feel strongly about it; was just curious
let attachResult = this._portalHost.attachComponentPortal(portal); | ||
|
||
// If were to attempt to focus immediately, then the content of the dialog would not yet be | ||
// ready in stances where change detection has to run first. To deal with this, we simply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in stances
-> in instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
flushMicrotasks(); | ||
|
||
expect(document.activeElement.tagName) | ||
.toBe('INPUT', 'Expected the first tabbale element (input) in the dialog to be focused.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabbale
-> tabbable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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. |
R: @kara or @hansl
This is the final PR necessary before publishing dialog.
(though there's still plenty to do after that)