1605610 - Using Angular 8 transitions (page :enter transition) with a Firefox browser means that the displayed page does not carry out the transition

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[timo]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

I started creating an Angular 7 page. (Now it is Angular 8.)
I added an ":enter" transition to start a short fade in animation after a page is loaded.
In my Angular project I use the following transition code:

import { trigger, animate, transition, style } from '@angular/animations';

export const fadeInAnimation =
    trigger('fadeInAnimation', [
        // route 'enter' transition
        transition(':enter', [
            style({ opacity: 0 }),
            animate('.3s', style({ opacity: 1 }))
        ]),
    ]);

(The animation currently do not run in chromium browser(unfortunately), so it seems that the problem only occurs in Firefox)

Actual results:

When I enter the web page the content is shown. After a page reload it is not shown anymore (opacity: 0). Due to the transition which was not executed? After doing a reload with CTRL+F5 the content is back (cache problem?).

Expected results:

The loading animation should run for 0.3 seconds when the page loads.
The content should not be hidden.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

There is an element that has opacity: 0 in the computed style which shouldn't (or I can't see why it should at least).

Url is https://pubtestpanel.binsky.org/login (from #introduction).

I still haven't

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Err, I should finish my comments... I still haven't figured out what angular does under the hood, and my bisection has been flaky because reproducing it in older builds takes a whole lot of retries. But it seems it's not reproducible on 57.

So this is probably a regression.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ok, so took some time to look at this, and I still don't know what's going on... We set opacity: 0 off a transition, but we end up never removing that rule.

I know why this doesn't transition at all on other browsers though, which is https://bugs.webkit.org/show_bug.cgi?id=184915 / https://bugs.chromium.org/p/chromium/issues/detail?id=420679. The <app-root>, <app-login> and co probably want to be display: block. I think that should fix the opacity in other browsers too. IB splits are quite annoying...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok, so the effect is removed from here:

(rr) bt
#0  PLDHashTable::RawRemove(PLDHashTable::Slot&) (this=0x7f520cc492f0, aSlot=...) at /home/emilio/src/moz/gecko/xpcom/ds/PLDHashTable.cpp:650
#1  0x00007f520301cae8 in PLDHashTable::RawRemove(PLDHashEntryHdr*) (this=0x20, aEntry=<optimized out>) at /home/emilio/src/moz/gecko/xpcom/ds/PLDHashTable.cpp:626
#2  0x00007f520301ca73 in PLDHashTable::RemoveEntry(PLDHashEntryHdr*) (this=0x7f520cc492f0, aEntry=0x1) at /home/emilio/src/moz/gecko/xpcom/ds/PLDHashTable.cpp:620
#3  0x00007f52043337c2 in nsTHashtable<nsRefPtrHashKey<mozilla::dom::KeyframeEffect> >::RemoveEntry(nsRefPtrHashKey<mozilla::dom::KeyframeEffect>*) (this=0x7f520cc492f0, aEntry=0x7f51eb6b6a14)
    at /home/emilio/src/moz/gecko/obj-debug/dist/include/nsTHashtable.h:214
#4  nsTHashtable<nsRefPtrHashKey<mozilla::dom::KeyframeEffect> >::EnsureRemoved(mozilla::dom::KeyframeEffect*) (this=0x7f520cc492f0, aKey=<optimized out>) at /home/emilio/src/moz/gecko/obj-debug/dist/include/nsTHashtable.h:204
#5  0x00007f520432d61f in mozilla::EffectSet::RemoveEffect(mozilla::dom::KeyframeEffect&) (this=0x7f520cc492f0, aEffect=...) at /home/emilio/src/moz/gecko/dom/animation/EffectSet.cpp:191
#6  mozilla::dom::KeyframeEffect::UnregisterTarget() (this=0x7f520cce4b20) at /home/emilio/src/moz/gecko/dom/animation/KeyframeEffect.cpp:829
#7  0x00007f5204320786 in mozilla::dom::KeyframeEffect::NotifyAnimationTimingUpdated(mozilla::PostRestyleMode) (this=0x7f520cce4b20, aPostRestyle=mozilla::PostRestyleMode::IfNeeded)
    at /home/emilio/src/moz/gecko/dom/animation/KeyframeEffect.cpp:161
#8  0x00007f520431c571 in mozilla::dom::Animation::Cancel(mozilla::PostRestyleMode) (this=0x7f51ee436c40, aPostRestyle=mozilla::PostRestyleMode::IfNeeded) at /home/emilio/src/moz/gecko/dom/animation/Animation.cpp:511
#9  0x00007f5204609298 in mozilla::dom::Animation_Binding::cancel(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) (cx=0x7f51f031b000, obj=..., void_self=0x7f51ee436c40, args=...) at AnimationBinding.cpp:1043
(rr) p DumpJSStack()
0 zUnb/hk</n.prototype._resetDomPlayerState() ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1026835]
    this = [object Object]
1 zUnb/hk</n.prototype.destroy() ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1027031]
    this = [object Object]
2 zUnb/jx</n.prototype.destroy() ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1013133]
    this = [object Object]
3 zUnb/Lx</n.prototype._flushAnimations/</<() ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1008580]
4 zUnb/hk</n.prototype._onFinish/<(n = [function], "1", "function(){return e(t&&fS(t,\"done\",n))},function(){n.destroy();var l=t.players.indexOf(n);t.players.splice(l,1)}") ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1025441]
5 forEach(callbackfn = [function]) ["self-hosted":235:12]
    this = function(){return e(t&&fS(t,"done",n))},function(){n.destroy();var l=t.players.indexOf(n);t.players.splice(l,1)}
6 zUnb/hk</n.prototype._onFinish() ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1025413]
    this = [object Object]
7 zUnb/hk</n.prototype._buildPlayer/<("[object AnimationPlaybackEvent]") ["https://pubtestpanel.binsky.org/main.4553fbebf9ffc1b556ce.js":1:1025841]
    this = [object Animation]
8 0TWp/</</s</e.prototype.invokeTask(e = "[object Object]", t = "[object Object]", n = "[object Animation]", r = "[object AnimationPlaybackEvent]") ["https://pubtestpanel.binsky.org/polyfills.374f5fe0e936f313a3cb.js":1:7812]
    this = [object Object]
9 0TWp/</</i</t.prototype.runTask(e = "[object Object]", t = "[object Animation]", n = "[object AnimationPlaybackEvent]") ["https://pubtestpanel.binsky.org/polyfills.374f5fe0e936f313a3cb.js":1:2994]
    this = [object Object]
10 0TWp/</</u</t.invokeTask(e = "[object Object]", t = "[object Animation]", n = "[object AnimationPlaybackEvent]") ["https://pubtestpanel.binsky.org/polyfills.374f5fe0e936f313a3cb.js":1:8895]
    this = [object Object]
11 _(e = "[object Object]", t = "[object Animation]", n = "[obje

So it seems like they're using programmatic animations, and cancel() is not triggering a restyle properly.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

That took quite a while to get right, it's a very specific codepath that needs to get hit for this to trigger the bug.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a340b42d6822f6d001d400d6a83c531e24823fc contains a fix and a test, let's see what the try server says about it, it's somewhat tricky code...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

That was... not very green ;(

Here's another try with some fixes on top: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3779b2d288a082b13b6338a3298b8520b54bb98f

Comment 8

[Emilio Cobos Álvarez (:emilio)]

That looks a bit closer, but somehow I managed to break scroll restoration... Here's a less scary patch that is more targeted and should also fix the bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61818a8bd82432eca021c3d1ebf9580a80a61383

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1605610 - Ensure to not create transition rules for elements that don't have any transition effect. r=boris,hiro,birtles

In particular, make it not create bogus non-empty transition rules when
EffectSet's cascade information isn't up-to-date.

There are multiple places where that can happen as described in bug 1606176,
but it's not clear that what this code is doing matches the spec to begin
with...

Anyhow, in this particular call site, this is only used to filter from
transition rules effects that are from running animations, to implement:

https://drafts.csswg.org/css-transitions/#application:

Implementations must add this value to the cascade if and only if that
property is not currently undergoing a CSS Animation ([CSS3-ANIMATIONS])
on the same element.

(Which other browsers don't implement, IIRC).

In the test-case, the EffectSet cascade info is empty, so we hit the "skip
everything" for animations (wrong), and "skip nothing for transitions" (also
wrong). This creates a transition rule node which then we never remove
(understandably, as the node never had a transition!).

This fixes the observables of this test-case, by never skipping animation
effects (you don't have to), and by checking the animation cascade level (so
that we don't create transition rule nodes with declarations coming from
animations). So I think this is strictly more correct than what we were doing.

If we hit the proposed assertion after this change, this code may still create
transition rules that incorrectly override animations, but will never mint one
out of the blue which we'd then fail to remove (which is the problem the
test-case is hitting).

This is a pre-existing issue, plus this is a part of the platform where there's
no interop already for a variety of reasons, so I'm not too concerned about it
for the purpose of this bug...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Ok, the above patch should be green, is pretty minimal, fixes the bug, and should be an strict improvement in correctness... I filed follow-up work in bug 1606176, because this area is quite a can of worms, looks like... :-(

I should probably also try to debug what's up with scroll restoration in the other attempt (which should be pretty-close to green modulo the assertion I added but doesn't hold as described in the bug above)... But probably worth a different bug / patch too.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:heycam, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d74a47ca0f77
Ensure to not create transition rules for elements that don't have any transition effect. r=hiro

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21070 for changes under testing/web-platform/tests

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/21070
* Community-TC (pull_request) (https://community-tc.services.mozilla.com/tasks/groups/Y5XXfP8RTuSwsval_SRXjQ)

Comment 15

[Emilio Cobos Álvarez (:emilio)]

The test is expected to fail in current nightly, so it's good that taskcluster complains. My patch should make it pass consistently... James, any better way to deal with comment 14 other than waiting for my patch to reach central and re-kick the PR?

Also it seems the bot overrode the ni? for cam, which is a bit unfortunate and should probably be fixed. Though in this case I guess I can prioritize the bug myself :-)

Comment 16

[James Graham [:jgraham]]

There's no clearly better way. We can force merge it or we can rerun the checks tomorrow with a new nightly; that's pretty much it. So far no one got as far as writing some system to allow overriding the checks results, or scheduling a retry.

The needinfo flags thing does seem like a bug; the code definitely looks like it's trying to handle this case but perhaps the bugzilla API doesn't work the way we think. I filed https://github.com/mozilla/wpt-sync/issues/610

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d74a47ca0f77

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This doesn't appear to be a new issue - what are your thoughts on uplifting to Beta for 73 vs. letting the fix ride 74 to release, Emilio?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9117938 [details]
Bug 1605610 - Ensure to not create transition rules for elements that don't have any transition effect. r=boris,hiro,birtles

Beta/Release Uplift Approval Request

• User impact if declined: In some edge cases involving web animations style may be wrong, hiding elements permanently and such.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple fix that avoids getting into a bogus state.
• String changes made/needed: none

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(I think given it's early in the cycle and the fix is not invasive / risky we should try to get it into 73)

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9117938 [details]
Bug 1605610 - Ensure to not create transition rules for elements that don't have any transition effect. r=boris,hiro,birtles

Fixes a Web Animation bug which can cause rendering issues. Thanks for including a test with this. Approved for 73.0b3.

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2dec3c77c091

Comment 25

[Cristian Fogel [:cfogel]]

Confirmed issue on 73.0b/72.0.1 ussing the test case provided by Emilio.
Fix verified on 73.0b3/74.0a1 (2020-01-09) on Windows 10, Ubuntu 18.04, macOS 10.15.