X Tutup
The Wayback Machine - https://web.archive.org/web/20220321101803/https://github.com/angular/components/pull/23972
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.

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

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

Merged
merged 1 commit into from Mar 9, 2022

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 17, 2021

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 label Nov 17, 2021
@crisbeto crisbeto marked this pull request as ready for review Nov 17, 2021
@crisbeto crisbeto requested a review from jelbourn as a code owner Nov 17, 2021
Copy link
Member

@jelbourn jelbourn left a comment

lgtm

@github-actions
Copy link

@github-actions github-actions bot commented Dec 22, 2021

// `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

@mmalerba mmalerba Jan 21, 2022

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

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

@mmalerba mmalerba Feb 4, 2022

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

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
Copy link
Member Author

@crisbeto 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
18 checks passed
amysorto added a commit that referenced this issue Mar 9, 2022
…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)
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Mar 18, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
X Tutup