Skip to content
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

fix(cdk/overlay): backdrop timeouts not being cleared in some cases #23972

Merged
merged 1 commit into from
Mar 9, 2022
Merged

fix(cdk/overlay): backdrop timeouts not being cleared in some cases #23972

merged 1 commit into from
Mar 9, 2022

Conversation

crisbeto
Copy link
Member

When we start the detach sequence of a backdrop, we bind a transitionend event as well as a 500ms timeout in case the event doesn't fire. If the event fires before 500ms, we clear the timeout so users don't have to flush it in tests, however we had the following problems:

  1. Transparent backdrops don't have a transition so they were always being cleaned up after the 500ms timeout.
  2. We weren't clearing the timeout if the overlay was disposed while the backdrop transition is running. This meant that the timeout would still be in memory, even though the element was removed from the DOM.
  3. We had a memory leak where the click and transitionend events weren't being removed from the backdrop if the overlay is disposed while detaching.

Basically all Material components had one of these issues. These changes resolve the problems by:

  1. Clearing the timeout when the overlay is disposed.
  2. Setting a 1ms transition on the transparent overlay so its transitionend can fire (almost) instantly.
  3. Removing the click and transitionend events when the backdrop is disposed.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 17, 2021
@crisbeto crisbeto marked this pull request as ready for review November 17, 2021 12:59
@crisbeto crisbeto requested a review from jelbourn as a code owner November 17, 2021 12:59
@crisbeto crisbeto added G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Nov 17, 2021
jelbourn
jelbourn approved these changes Nov 17, 2021
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 the action: merge The PR is ready for merge by the caretaker label Nov 17, 2021
@josephperrott josephperrott added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Dec 22, 2021
@github-actions
Copy link

Deployed dev-app to: https://ng-comp-dev--pr-23972-a880f87e34fa15d448cdfd1c99520f4a-4aj27tou.web.app

@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
mmalerba
mmalerba reviewed Jan 21, 2022
View reviewed changes
src/cdk/overlay/_index.scss
// `rgba(0, 0, 0, 0)`, we work around the inconsistency by not setting the background at
// all and using `opacity` to make the element transparent.
&, &.cdk-overlay-backdrop-showing {
opacity: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why was the opacity: 0 removed? this seems to be breaking screenshots in some apps

Copy link
Member Author

@crisbeto crisbeto Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's been a while since I opened the PR, but I think it was because the opacity would actually cause the transition to run, even though it's animating from transparent to transparent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there may be something not quite right about it. When I presubmitted this I wound up with some apps that had opaque backdrops covering everything

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jan 21, 2022
@crisbeto crisbeto removed the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Mar 4, 2022
@crisbeto
fix(cdk/overlay): backdrop timeouts not being cleared in some cases
2923a85 
When we start the detach sequence of a backdrop, we bind a `transitionend` event as well as a 500ms timeout in case the event doesn't fire. If the event fires before 500ms, we clear the timeout so users don't have to flush it in tests, however we had the following problems:
1. Transparent backdrops don't have a transition so they were always being cleaned up after the 500ms timeout.
2. We weren't clearing the timeout if the overlay was disposed while the backdrop transition is running. This meant that the timeout would still be in memory, even though the element was removed from the DOM.
3. We had a memory leak where the `click` and `transitionend` events weren't being removed from the backdrop if the overlay is disposed while detaching.

Basically all Material components had one of these two issues. These changes resolve the problems by:
1. Clearing the timeout when the overlay is disposed.
2. Setting a 1ms transition on the transparent overlay so its `transitionend` can fire (almost) instantly.
3. Removing the `click` and `transitionend` events when the backdrop is disposed.
@crisbeto crisbeto force-pushed the overlay-backdrop-timeouts branch from a880f87 to 2923a85 Compare March 4, 2022 10:33
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Mar 4, 2022
@crisbeto
Copy link
Member Author

crisbeto commented Mar 4, 2022

I've pushed another change to try and make it easier to land.

@amysorto amysorto merged commit bbe6355 into angular:master Mar 9, 2022
amysorto pushed a commit that referenced this pull request Mar 9, 2022
@crisbeto @amysorto
fix(cdk/overlay): backdrop timeouts not being cleared in some cases (#…
39929a8 
…23972)

When we start the detach sequence of a backdrop, we bind a `transitionend` event as well as a 500ms timeout in case the event doesn't fire. If the event fires before 500ms, we clear the timeout so users don't have to flush it in tests, however we had the following problems:
1. Transparent backdrops don't have a transition so they were always being cleaned up after the 500ms timeout.
2. We weren't clearing the timeout if the overlay was disposed while the backdrop transition is running. This meant that the timeout would still be in memory, even though the element was removed from the DOM.
3. We had a memory leak where the `click` and `transitionend` events weren't being removed from the backdrop if the overlay is disposed while detaching.

Basically all Material components had one of these two issues. These changes resolve the problems by:
1. Clearing the timeout when the overlay is disposed.
2. Setting a 1ms transition on the transparent overlay so its `transitionend` can fire (almost) instantly.
3. Removing the `click` and `transitionend` events when the backdrop is disposed.

(cherry picked from commit bbe6355)
@sync-by-unito sync-by-unito bot mentioned this pull request Mar 14, 2022
Closed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 18, 2022
Update angularmaterial monorepo to v13.2.6 (#1214)
fc919a2 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/cdk](https://github.com/angular/components) | dependencies | patch | [`13.2.5` -> `13.2.6`](https://renovatebot.com/diffs/npm/@angular%2fcdk/13.2.5/13.2.6) |
| [@angular/material](https://github.com/angular/components) | dependencies | patch | [`13.2.5` -> `13.2.6`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/13.2.5/13.2.6) |

---

### Release Notes

<details>
<summary>angular/components</summary>

### [`v13.2.6`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#&#8203;1326-suede-spaghetti-2022-03-09)

[Compare Source](angular/components@13.2.5...13.2.6)

##### cdk

| Commit | Type | Description |
| -- | -- | -- |
| [39929a815d](angular/components@39929a8) | fix | **overlay:** backdrop timeouts not being cleared in some cases ([#&#8203;23972](angular/components#23972)) |
| [2f2b0c7cf4](angular/components@2f2b0c7) | fix | **testing:** dispatch mouseover and mouseout events in UnitTestElement ([#&#8203;24490](angular/components#24490)) |
| [edca54f2d0](angular/components@edca54f) | fix | **testing:** require at least one argument for locator functions ([#&#8203;23619](angular/components#23619)) |

##### material

| Commit | Type | Description |
| -- | -- | -- |
| [c4993ac171](angular/components@c4993ac) | fix | **button:** avoid setting a tabindex on all link buttons ([#&#8203;22901](angular/components#22901)) |
| [c47d30e0e5](angular/components@c47d30e) | fix | **dialog:** don't wait for animation before moving focus ([#&#8203;24121](angular/components#24121)) |
| [70b8248568](angular/components@70b8248) | fix | **expansion:** able to tab into descendants with visibility while closed ([#&#8203;24045](angular/components#24045)) |
| [d22d73ab8d](angular/components@d22d73a) | fix | **select:** disabled state out of sync when swapping form group with a disabled one ([#&#8203;17872](angular/components#17872)) |
| [911d6b71d4](angular/components@911d6b7) | fix | **slide-toggle:** clear name from host node ([#&#8203;15505](angular/components#15505)) |
| [4b5363d160](angular/components@4b5363d) | fix | **tooltip:** decouple removal logic from change detection ([#&#8203;19432](angular/components#19432)) |

##### material-experimental

| Commit | Type | Description |
| -- | -- | -- |
| [8414646d79](angular/components@8414646) | fix | **mdc-card:** remove extra margin if header doesn't have an avatar ([#&#8203;19072](angular/components#19072)) |
| [f66486dc5b](angular/components@f66486d) | fix | **mdc-slider:** fix a few null pointer exceptions ([#&#8203;23659](angular/components#23659)) |

##### multiple

| Commit | Type | Description |
| -- | -- | -- |
| [6ee0089ce6](angular/components@6ee0089) | fix | don't block child component animations on open ([#&#8203;24529](angular/components#24529)) |

#### Special Thanks

Andrew Seguin, Jeri Peier, Kristiyan Kostadinov and Paul Gschwendtner

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1214
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@sync-by-unito sync-by-unito bot mentioned this pull request Mar 22, 2022
Closed
forsti0506 pushed a commit to forsti0506/components that referenced this pull request Apr 3, 2022
@crisbeto @forsti0506
fix(cdk/overlay): backdrop timeouts not being cleared in some cases (a…
bee1b92 
…ngular#23972)

When we start the detach sequence of a backdrop, we bind a `transitionend` event as well as a 500ms timeout in case the event doesn't fire. If the event fires before 500ms, we clear the timeout so users don't have to flush it in tests, however we had the following problems:
1. Transparent backdrops don't have a transition so they were always being cleaned up after the 500ms timeout.
2. We weren't clearing the timeout if the overlay was disposed while the backdrop transition is running. This meant that the timeout would still be in memory, even though the element was removed from the DOM.
3. We had a memory leak where the `click` and `transitionend` events weren't being removed from the backdrop if the overlay is disposed while detaching.

Basically all Material components had one of these two issues. These changes resolve the problems by:
1. Clearing the timeout when the overlay is disposed.
2. Setting a 1ms transition on the transparent overlay so its `transitionend` can fire (almost) instantly.
3. Removing the `click` and `transitionend` events when the backdrop is disposed.
@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 Apr 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mmalerba mmalerba mmalerba left review comments

@jelbourn jelbourn jelbourn approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround 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
@crisbeto @jelbourn @mmalerba @amysorto @josephperrott @andrewseguin