fix(focus-trap): focus initial element when zone stabilizes #4867
Conversation
* Focuses the first element when the zone stabilizes, instead of when the microtask queue is empty. This avoids issues where the element might be focused before Angular is done doing change detection. * Only runs the `onStable` callback if the zone is unstable. * Avoids an extra DOM lookup by combining a couple of selectors. Fixes angular#4864.
googlebot
added
the
cla: yes
src/lib/core/a11y/focus-trap.ts
Outdated
| ...Array.prototype.slice.call(this._element.querySelectorAll(`[cdk-focus-${bound}]`)), | ||
| ]; | ||
| // Contains the deprecated version of selector, for temporary backwards comparability. | ||
| let markers = this._element.querySelectorAll(`[cdk-focus-region-${bound}], ` + |
There was a problem hiding this comment.
I did this to make sure the new version takes precedence over the deprecated version, idk maybe it was overkill though
There was a problem hiding this comment.
It's probably overkill since the deprecated and the supported directives do exactly the same.
src/lib/core/a11y/focus-trap.ts
Outdated
|
|
||
| markers.forEach((el: HTMLElement) => { | ||
| Array.prototype.forEach.call(markers, (el: HTMLElement) => { |
There was a problem hiding this comment.
could we do a for of loop here to avoid the weird syntax? or does that have the same issue of not working with NodeList?
There was a problem hiding this comment.
Loops work fine. Changed.
There was a problem hiding this comment.
I actually meant the for (let val of iterable) style, but this is fine too
mmalerba
added
pr: lgtm
action: merge
|
LGTM |
|
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. |
onStablecallback if the zone is unstable.Fixes #4864.
Note: Since change detection is all being run manually in unit tests, I wasn't able to write a test that would fail properly when we expect it to.