Skip to content

perf(cdk/overlay): add event listeners for overlay dispatchers outside of zone #23962

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

Merged
merged 1 commit into from
Feb 3, 2022
Merged

perf(cdk/overlay): add event listeners for overlay dispatchers outside of zone #23962

merged 1 commit into from
Feb 3, 2022

Conversation

arturovt
Copy link
Contributor

The OverlayKeyboardDispatcher adds a keydown event listener, which causes Angular to run change detections on any keydown event, tho its listener may behave as a "noop" (e.g. if there're no keydownEvents observers).

With these changes, the Angular zone will be re-entered only if there're any keydownEvents listeners.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 14, 2021
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.

Also the CI appears to be failing.

@arturovt arturovt requested a review from a team as a code owner November 14, 2021 12:06
@arturovt arturovt requested review from crisbeto and removed request for a team November 14, 2021 12:10
@@ -28,7 +32,11 @@ export class OverlayKeyboardDispatcher extends BaseOverlayDispatcher {

// Lazily start dispatcher once first overlay is added
if (!this._isAttached) {
this._document.body.addEventListener('keydown', this._keydownListener);
this._ngZone
? this._ngZone.runOutsideAngular(() =>
Copy link
Member

Choose a reason for hiding this comment

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

Can you use an if/else statement here, rather than the ternary? Also can you leave an @breaking-change note here as well? That way our tooling will pick it up when we're doing the breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.

@arturovt arturovt changed the title perf(cdk/overlay): add keydown listener outside of zone perf(cdk/overlay): add event listeners for overlay dispatchers outside of zone Nov 15, 2021
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

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Nov 15, 2021
@andrewseguin andrewseguin added needs rebase and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Dec 28, 2021
@arturovt arturovt requested a review from crisbeto January 13, 2022 16:24
@zarend zarend merged commit e761455 into angular:master Feb 3, 2022
zarend pushed a commit that referenced this pull request Feb 3, 2022
@arturovt arturovt deleted the perf/overlay-keyboard-dispatcher branch February 3, 2022 19:14
zarend added a commit that referenced this pull request Feb 3, 2022
zarend added a commit that referenced this pull request Feb 3, 2022
zarend added a commit that referenced this pull request Feb 3, 2022
…s outside of zone (#23962)" (#24353)

This reverts commit e761455.

(cherry picked from commit e6b9679)
@zarend
Copy link
Contributor

zarend commented Feb 9, 2022

Hello @arturovt, this PR was reverted because it caused an issue internally. Could you please make a new PR with this change? That we can run a fresh presubmit check, then assess how best to proceed with this.

@arturovt
Copy link
Contributor Author

arturovt commented Feb 9, 2022

Hey @zarend , alright.

amysorto pushed a commit to amysorto/components that referenced this pull request Feb 15, 2022
@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 Mar 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants