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(animations): cleanup DOM elements when the root view is removed #41001

Closed
wants to merge 1 commit into from
Closed

fix(animations): cleanup DOM elements when the root view is removed #41001

wants to merge 1 commit into from

Conversation

arturovt
Copy link
Contributor

PR Checklist

PR Type

  • Bugfix

@google-cla google-cla bot added the cla: yes label Feb 25, 2021
@arturovt
Copy link
Contributor Author

This is a replacement of this PR (#39982).

@atscott cc. There were some presubmit failures, but you mentioned in that comment that you manually fixed some issues (#39982 (comment)). Could you try to re-run the presubmit?

@atscott
Copy link
Contributor

atscott commented Feb 25, 2021

triggered global tests to see what still needs to be fixed (if any).

@ngbot ngbot bot added this to the Backlog milestone Feb 26, 2021
@atscott
Copy link
Contributor

atscott commented Mar 1, 2021

Global tests are all green. This should be good to go now 👍

@atscott atscott added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Mar 1, 2021
@ngbot
Copy link

ngbot bot commented Mar 1, 2021

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google3" is failing
    pending missing required labels: target: *
    pending status "pullapprove" is pending
    pending 2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@arturovt
Copy link
Contributor Author

arturovt commented Mar 1, 2021

@atscott oh great 👍

Well, I see the Presubmit Failed 😄

@arturovt @atscott
fix(animations): cleanup DOM elements when the root view is removed
904ec3f 
Currently, when importing `BrowserAnimationsModule`, Angular uses `AnimationRenderer`
as the renderer. When the root view is removed, the `AnimationRenderer` defers the actual
work to the `TransitionAnimationEngine` to do this, and the `TransitionAnimationEngine`
doesn't actually remove the DOM node, but just calls `markElementAsRemoved()`.

The actual DOM node is not removed until `TransitionAnimationEngine` "flushes".

Unfortunately, though, that "flush" will never happen, since the root view is being
destroyed and there will be no more flushes.

This commit adds `flush()` call when the root view is being destroyed.

BREAKING CHANGE:
DOM elements are now correctly removed when the root view is removed. It
is possible that tests could be accidentally relying on the old behavior by
trying to find an element that was not removed in a previous test. If
this is the case, the failing tests should be updated to ensure they
have proper setup code which initializes elements they rely on.
@atscott
Copy link
Contributor

atscott commented Mar 1, 2021

@atscott oh great 👍

Well, I see the Presubmit Failed 😄

There were some unrelated failures. I just need to force it green :)

crisbeto
crisbeto approved these changes Mar 1, 2021
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

@zarend zarend closed this in a31da48 Mar 1, 2021
@arturovt arturovt deleted the fix/39955 branch March 2, 2021 06:13
@arturovt arturovt mentioned this pull request Mar 2, 2021
Closed
zarend added a commit to zarend/angular that referenced this pull request Mar 2, 2021
@zarend
Revert "fix(animations): cleanup DOM elements when the root view is r…
32a7060 
…emoved (angular#41001)"

This reverts commit a31da48.
@zarend zarend mentioned this pull request Mar 2, 2021
Closed
@atscott
Copy link
Contributor

atscott commented Mar 2, 2021 •
edited

After a bit more investigation, the failure was legitimate - the SSR code was configured to destroy the platform (internal link to the code) before returning the generated HTML which resulted in the Angular elements being cleaned up as well. I missed this in the global tests because I hadn't configured them to run with Ivy enabled.
We'll have to re-open this PR, rerun the tests, and evaluate what this change means for SSR clients -- it could be quite a bit more breaking than anticipated if others have their server rendering set up in a similar way.

zarend added a commit that referenced this pull request Mar 2, 2021
@zarend
Revert "fix(animations): cleanup DOM elements when the root view is r…
96f15f0 
…emoved (#41001)" (#41056)

This reverts commit a31da48.

PR Close #41056
@arturovt arturovt mentioned this pull request Mar 2, 2021
Closed
3 tasks
This was referenced Mar 3, 2021
Merged
Merged
@dependabot dependabot bot mentioned this pull request Mar 10, 2021
Closed
This was referenced Mar 15, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Closed
Closed
Closed
Closed
Merged
Merged
Merged
Merged
Closed
Closed
Closed
@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 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@atscott atscott atscott approved these changes

@crisbeto crisbeto crisbeto approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: animations breaking changes cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@arturovt @atscott @mhevery @crisbeto @zarend