Skip to content

fix(focus-monitor): don't null-out focus until after event is finished with capture & bubble #10721

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 3, 2018
Merged

fix(focus-monitor): don't null-out focus until after event is finished with capture & bubble #10721

merged 2 commits into from
May 3, 2018

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Apr 5, 2018 •
edited
Loading

This fixes an issue where putting cdkMonitorSubtreeFocus on a child with cdkMonitorElementFocus causes the focus origin to be lost for the child element, due to the parent element nulling it out too soon.

@mmalerba mmalerba added pr: needs review target: patch This PR is targeted for the next patch release labels Apr 5, 2018
@mmalerba mmalerba requested a review from crisbeto April 5, 2018 17:26
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 5, 2018
jelbourn
jelbourn reviewed Apr 5, 2018
View reviewed changes
src/cdk/a11y/focus-monitor/focus-monitor.ts Outdated

// Null-out the origin after a setTimeout. This allows the full capture/bubble cycle to complete
// for the current event before nulling it.
setTimeout(() => this._origin = null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be run outside the zone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already is since this happens inside a listener that's outside the zone

jelbourn
jelbourn approved these changes Apr 5, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed cla: yes PR author has agreed to Google's Contributor License Agreement pr: needs review labels Apr 5, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 5, 2018
crisbeto
crisbeto approved these changes Apr 5, 2018
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/cdk/a11y/focus-monitor/focus-monitor.ts Outdated

// Null-out the origin after a setTimeout. This allows the full capture/bubble cycle to complete
// for the current event before nulling it.
setTimeout(() => this._origin = null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could avoid breaking a bunch of tests by using a Promise.resolve().then(...) rather than the timeout. It does make it harder to flush though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, it doesn't wait long enough. Looks like it has to be setTimeout

@mmalerba
Copy link
Contributor Author

mmalerba commented Apr 5, 2018

@jelbourn @crisbeto simplified the async logic slightly as Jake suggested on the internal mailing list

@mmalerba mmalerba force-pushed the fm-delay-null branch 2 times, most recently from 0a5f3bf to 31c19ce Compare April 9, 2018 19:00
@andrewseguin andrewseguin added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 26, 2018
@ngbot
Copy link

ngbot bot commented Apr 26, 2018

Hi @mmalerba! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@andrewseguin andrewseguin assigned mmalerba and unassigned crisbeto Apr 26, 2018
@mmalerba mmalerba removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 1, 2018
@josephperrott josephperrott merged commit 0b7572b into angular:master May 3, 2018
josephperrott pushed a commit that referenced this pull request May 3, 2018
@mmalerba @josephperrott
fix(focus-monitor): don't null-out focus until after event is finishe…
71ac0ae 
…d with capture & bubble (#10721)
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion Awaiting requested review from devversion

@tinayuangao tinayuangao Awaiting requested review from tinayuangao

Assignees

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mmalerba @jelbourn @crisbeto @googlebot @josephperrott @andrewseguin