fix(material/tooltip): decouple removal logic from change detection by crisbeto · Pull Request #19432 · angular/components

crisbeto · 2020-05-24T11:29:17Z

Currently the logic in the tooltip that removes it from the DOM is run either if the trigger is destroyed or the exit animation has finished. The problem is that if the trigger is detached from change detection, but hasn't been destroyed, the exit animation will never run and the element won't be cleaned up. These changes switch to using CSS animations and manipulating the DOM node directly to trigger the animation.

Fixes#19365.

crisbeto · 2020-05-24T11:29:56Z

Caretaker note: these changes have the potential to break people's tests. We should determine whether to move forward, based on how many targets it breaks.

jelbourn · 2020-05-26T19:25:25Z

+ opacity: 0;
+ transform: scale(0);
+
+ // Use a very short animation if animations are disabled so the `animationend` event still fires.


We can check with a presubmit, but this feels like it could cause some issues where people are expecting synchronous behavior

jelbourn · 2020-05-29T16:44:53Z

 * @param delay Amount of milliseconds to the delay showing the tooltip.
 */
- show(delay: number): void {
+ show(delay: number, isUserInteraction?: boolean): void {


Technically this is a breaking change since we export TooltipComponent in the public API (probably unintentionally)

I don't think it's breaking since the new parameter is optional.

jelbourn

LGTM

jelbourn · 2020-06-09T04:59:13Z

Looks like it needs a rebase to presubmit

crisbeto · 2020-08-16T07:18:48Z

I pushed one more change to help address the concerns from #15990 by binding the internal animationend event outside the NgZone. I'm also bumping it to a P2, because this has come up a few times while triaging issues.

mmalerba · 2020-09-02T19:15:23Z

FYI this has 24 failing targets, it would be possible to get in, but would take some effort

mmalerba · 2022-01-20T19:54:28Z

It seems that there's something not being properly flushed when using the harness. There's a test that looks roughly like this:

 const tooltip1 = await getHarness(MatTooltipHarness.with({selector: 'tt1'}));
await (await tooltip1.host()).hover();
expect(await tooltip1.isOpen()).toEqual(true);
await (await tooltip1.host()).mouseAway();
const tooltip2 = await getHarness(MatTooltipHarness.with({selector: 'tt2'}));
await (await tooltip2.host()).hover();
expect(await tooltip2.isOpen()).toEqual(true);
await (await tooltip2.host()).mouseAway();

For the second tooltip the isOpen check fails, if I comment out the tooltip1 stuff then it passes

crisbeto · 2022-03-05T11:01:41Z

I've rebased and added some logic to the test harnesses to simulate the animationend events.

Currently the logic in the tooltip that removes it from the DOM is run either if the trigger is destroyed or the exit animation has finished. The problem is that if the trigger is detached from change detection, but hasn't been destroyed, the exit animation will never run and the element won't be cleaned up. These changes switch to using CSS animations and manipulating the DOM node directly to trigger the animation. Fixesangular#19365.

…ction

…19432) * fix(material/tooltip): decouple removal logic from change detection Currently the logic in the tooltip that removes it from the DOM is run either if the trigger is destroyed or the exit animation has finished. The problem is that if the trigger is detached from change detection, but hasn't been destroyed, the exit animation will never run and the element won't be cleaned up. These changes switch to using CSS animations and manipulating the DOM node directly to trigger the animation. Fixes#19365. * fixup! fix(material/tooltip): decouple removal logic from change detection (cherry picked from commit a5ab8e9)

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#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 ([#23972](angular/components#23972)) | | [2f2b0c7cf4](angular/components@2f2b0c7) | fix | **testing:** dispatch mouseover and mouseout events in UnitTestElement ([#24490](angular/components#24490)) | | [edca54f2d0](angular/components@edca54f) | fix | **testing:** require at least one argument for locator functions ([#23619](angular/components#23619)) | ##### material | Commit | Type | Description | | -- | -- | -- | | [c4993ac171](angular/components@c4993ac) | fix | **button:** avoid setting a tabindex on all link buttons ([#22901](angular/components#22901)) | | [c47d30e0e5](angular/components@c47d30e) | fix | **dialog:** don't wait for animation before moving focus ([#24121](angular/components#24121)) | | [70b8248568](angular/components@70b8248) | fix | **expansion:** able to tab into descendants with visibility while closed ([#24045](angular/components#24045)) | | [d22d73ab8d](angular/components@d22d73a) | fix | **select:** disabled state out of sync when swapping form group with a disabled one ([#17872](angular/components#17872)) | | [911d6b71d4](angular/components@911d6b7) | fix | **slide-toggle:** clear name from host node ([#15505](angular/components#15505)) | | [4b5363d160](angular/components@4b5363d) | fix | **tooltip:** decouple removal logic from change detection ([#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 ([#19072](angular/components#19072)) | | [f66486dc5b](angular/components@f66486d) | fix | **mdc-slider:** fix a few null pointer exceptions ([#23659](angular/components#23659)) | ##### multiple | Commit | Type | Description | | -- | -- | -- | | [6ee0089ce6](angular/components@6ee0089) | fix | don't block child component animations on open ([#24529](angular/components#24529)) | #### Special Thanks Andrew Seguin, Jeri Peier, Kristiyan Kostadinov and Paul Gschwendtner  </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. --- - [ ] 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>

…ngular#19432) * fix(material/tooltip): decouple removal logic from change detection Currently the logic in the tooltip that removes it from the DOM is run either if the trigger is destroyed or the exit animation has finished. The problem is that if the trigger is detached from change detection, but hasn't been destroyed, the exit animation will never run and the element won't be cleaned up. These changes switch to using CSS animations and manipulating the DOM node directly to trigger the animation. Fixesangular#19365. * fixup! fix(material/tooltip): decouple removal logic from change detection

angular-automatic-lock-bot · 2022-04-06T00:03:54Z

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.}

crisbeto requested a review from andrewseguin as a code owner May 24, 2020 11:29

crisbeto assigned jelbourn and andrewseguin May 24, 2020

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 24, 2020

crisbeto force-pushed the 19365/tooltip-animations-refactor branch 2 times, most recently from 2f8fdae to 1be7eebCompare May 24, 2020 12:12

jelbourn reviewed May 29, 2020
View reviewed changes

jelbourn approved these changes Jun 9, 2020
View reviewed changes

jelbourn added lgtm labels Jun 9, 2020

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from 1be7eeb to f31088bCompare June 9, 2020 15:22

crisbeto added action: merge The PR is ready for merge by the caretaker and removed needs rebase labels Jun 9, 2020

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from f31088b to 27181b3Compare June 13, 2020 09:58

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from 27181b3 to b532858Compare July 3, 2020 21:05

mmalerba removed the lgtm label Jul 31, 2020

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from b532858 to 9f1ab9eCompare August 1, 2020 12:00

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from 9f1ab9e to c28838eCompare August 16, 2020 07:17

crisbeto added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Aug 16, 2020

This was referenced Aug 16, 2020
[Tooltip]: ChangeDetection is triggered to often #15990
Closed
bug(mat-sort-header): performance & change detection issue with the whole table #19441
Closed

crisbeto force-pushed the 19365/tooltip-animations-refactor branch 2 times, most recently from a9182a7 to a12d729Compare June 20, 2021 12:18

andrewseguin added needs rebase and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Dec 28, 2021

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from a12d729 to 136cacfCompare January 3, 2022 09:40

crisbeto removed the needs rebase label Jan 3, 2022

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jan 20, 2022

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from 136cacf to 46e4e9aCompare March 5, 2022 11:01

crisbeto added the action: merge The PR is ready for merge by the caretaker label Mar 5, 2022

crisbeto force-pushed the 19365/tooltip-animations-refactor branch 4 times, most recently from 27cf676 to da99bbdCompare March 5, 2022 15:33

crisbeto force-pushed the 19365/tooltip-animations-refactor branch 4 times, most recently from 851cdfe to 01eacdcCompare March 6, 2022 09:34

fixup! fix(material/tooltip): decouple removal logic from change dete…
fb516c0
…ction

crisbeto force-pushed the 19365/tooltip-animations-refactor branch from 01eacdc to fb516c0Compare March 6, 2022 10:16

crisbeto merged commit a5ab8e9 into angular:masterMar 6, 2022

sync-by-unitoBot mentioned this pull request Mar 14, 2022
build(deps): bump @angular/cdk from 12.2.4 to 13.2.6 numbersprotocol/capture-cam#1370
Closed

sync-by-unitoBot mentioned this pull request Mar 22, 2022
build(deps): bump @angular/cdk from 12.2.4 to 13.3.0 numbersprotocol/capture-cam#1403
Closed

angular-automatic-lock-botBot locked and limited conversation to collaborators Apr 6, 2022

Navigation Menu

Uh oh!

fix(material/tooltip): decouple removal logic from change detection - #19432

fix(material/tooltip): decouple removal logic from change detection#19432
crisbeto merged 2 commits into
angular:masterfrom
crisbeto:19365/tooltip-animations-refactor

crisbeto commented May 24, 2020

Uh oh!

crisbeto commented May 24, 2020

Uh oh!

jelbournMay 26, 2020

Uh oh!

Uh oh!

jelbournMay 29, 2020

Uh oh!

crisbetoMay 29, 2020

Uh oh!

jelbourn left a comment

Uh oh!

jelbourn commented Jun 9, 2020

Uh oh!

crisbeto commented Aug 16, 2020

Uh oh!

mmalerba commented Sep 2, 2020

Uh oh!

mmalerba commented Jan 20, 2022

Uh oh!

crisbeto commented Mar 5, 2022

Uh oh!

angular-automatic-lock-botBot commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

crisbeto commented May 24, 2020

Uh oh!

crisbeto commented May 24, 2020

Uh oh!

jelbournMay 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jelbournMay 29, 2020

Choose a reason for hiding this comment

Uh oh!

crisbetoMay 29, 2020

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jun 9, 2020

Uh oh!

crisbeto commented Aug 16, 2020

Uh oh!

mmalerba commented Sep 2, 2020

Uh oh!

mmalerba commented Jan 20, 2022

Uh oh!

crisbeto commented Mar 5, 2022

Uh oh!

angular-automatic-lock-botBot commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants