1607723 - Opacity and transform transitions fail when triggered by some events

Status: RESOLVED WORKSFORME

(Core :: CSS Transitions and Animations, defect, P3)

Description

[Kevin Doughty]

Attachment: firefoxTransitionBugTest.html

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

Steps to reproduce:

When CSS Transitions of compositor accelerated properties transform and opacity are triggered a second time by non mouse events, interrupting a previous transition, they fail to animate and instead jump to destination values. Events known to cause failure are keydown, keypress, and wheel. Also, animationend fails in certain cases that need to be investigated further. There are other events that could possibly fail but have not been checked, for instance touches.

The attached file shows keydown and wheel failure, and mousedown success.

Steps to reproduce bug:

1. Reload the page.
2. Press a key.
3. Wait for transition to be half finished.
4. Press a key again.

Alternate steps to reproduce bug:

1. Reload page.
2. Scroll the smallest detectable distance on trackpad.
3. Wait for transition to be half finished.
4. Scroll the smallest detectable distance again.

Failing events can be mixed to cause the bug, i.e. a keypress then wheel, or wheel then keydown.

Events like onsubmit depend on if the button was clicked by the mouse, which does not cause the bug, or if the button had focus and was activated by pressing the enter key, which does cause the bug. A submit button clicked with the javascript function click() during a window.setTimeout will trigger the bug.

The bug does not occur if the console is open.
The bug does not occur if the first transition was triggered by a mouse down event.
The bug does not occur if the second transition is triggered by a mouse down event.
The bug does not occur if any non-compositor accelerated property is also transitioned.
The bug does not occur if the mouse cursor is moved at all during the first transition.

A simplified account of a mouse event transitioning opacity successfully is as follows:

1. The initial mouse event results in traversal.rs recalc_style_at with a hint of RESTYLE_SELF which changes opacity from 1 to 0. This is discrete, non-animated state, which causes nsTransitionManager::ConsiderInitiatingTransition to start the transition.
2. A second call to recalc_style_at with a hint of RESTYLE_CSS_TRANSITIONS changes opacity from 0 back to 1. Despite being an integer, this is animated state, before the transition has begun.
3. A third call to recalc_style_at with a hint of RESTYLE_CSS_TRANSITIONS advances opacity from 1 to a single tick later, something like 0.992249. This seems untidy but presumably has a reason. It is not the focus of this bug report and can probably be disregarded.
4. Time passes as the animation plays until approximately halfway through.
5. A second mouse event results in a traversal.rs recalc_style_at with a RESTYLE_CSS_TRANSITIONS hint. The presShell opacity value changes from the previous 0.992249 to a midway value like 0.457836.
6. Another call to recalc_style_at with a RESTYLE_SELF hint determines the new discrete, non-animated state. The opacity value changes from the midway value of 0.457836 to the non-animated value of 1. This results in nsTransitionManager::ConsiderInitiatingTransition to start the reversal transition.
7. A call to recalc_style_at with a RESTYLE_CSS_TRANSITIONS hint reverts the presShell opacity value from the non-animated 1 to the same previously known midway value of 0.457836.
8. Another call to recalc_style_at with a RESTYLE_CSS_TRANSITIONS hint advances opacity a single tick again, to something like 0.461742. Again, this can probably be disregarded.
9. Time passes as the animation reverses direction.
10. When finished, a final call to recalc_style_at with a RESTYLE_CSS_TRANSITIONS hint is called to change the opacity value from the animated 0.457836 to the final value 1.

A simplified account of a key event transitioning opacity unsuccessfully is as follows:

1. The initial key event results in traversal.rs recalc_style_at with a hint of RESTYLE_SELF which changes opacity from 1 to 0. This is discrete, non-animated state, which causes nsTransitionManager::ConsiderInitiatingTransition to start the transition, identically to the mouse event version.
2. A second call to recalc_style_at with a hint of RESTYLE_CSS_TRANSITIONS changes opacity from 0 back to 1. Again, this is animated state, before the transition has begun, and identical to the mouse event version.
3. The redundant call to recalc_style with a RESTYLE_CSS_TRANSITIONS hint advancing the value one tick does not happen, so perhaps it should not be disregarded after all. Instead, time passes as the animation plays until approximately halfway through.
4. A second key event results in a recalc_style_at call with a RESTYLE_SELF hint. This is also different from the mouse version. The opacity value does not change, as the old value was the animated 1 and the new value is the non-animated destination value of 1.
5. A call to nsTransitionManager::ConsiderInitiatingTransition does not find a change in value and does not trigger a transition reversal.
6. One last call to recalc_style_at with a RESTYLE_CSS_TRANSITIONS hint produces the same, final opacity value of 1. All animation has now ended.

A wheel event fails similarly to key events. However, it causes a method to get called that also gets called for mouse events, from PresShell.cpp, EventHandler::HandleEventUsingCoordinates. But this does not result in a flush because of a switch statement in EventHandler::FlushPendingNotifications that only handles mousedown and mouseup. Key events do not use coordinates but wheel events do for some reason and might be fixed here, although it has not been attempted.

One potential cause of this bug is that PresShell.cpp DoFlushPendingNotifications does not seem to get called with aType >= FlushType::Style, which otherwise should initiate the RESTYLE_CSS_TRANSITIONS hint.

There is a potential fix that works for key and wheel events, but fails with one of two animationend events (that still need to be investigated). In PresShell.cpp, MOZ_STACK_CLASS nsPresShellEventCB, in the HandleEvent override, the follow lines may be added:

mozilla::ChangesToFlush flush(FlushType::None, true);
mPresShell->FlushPendingNotifications(flush);

It is not known if this is the best way to fix the bug. Since it did not work for all animationend events, it may not be the best choice, and further investigation is needed.

Actual results:

A transition reversal does not occur.

Expected results:

A transition reversal should have occurred.

Comment 1

[Kevin Doughty]

s/but fails with one of two animationend events/but fails with one of two animationend event example cases that have not been uploaded yet

Comment 2

[Emilio Cobos Álvarez [:emilio]]

4. A second key event results in a recalc_style_at call with a RESTYLE_SELF hint. This is also different from the mouse version. The opacity value does not change, as the old value was the animated 1 and the new value is the non-animated destination value of 1.

That seems like the bug. If the transition is mid-way, computing the style on the main thread should yield the interpolated value.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I think a key difference here may be that for mouse click and such we'd go through FlushThrottledStyles, but it's just a guess.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

This is probably due to my fault. I did confirm that the transitions works perfectly on the old style system.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I think a key difference here may be that for mouse click and such we'd go through FlushThrottledStyles, but it's just a guess.

Kinda. FlushThrottlesStyles flushes for all throttles animations in the document, but in this particular case we just need to flush the throttled animation on the element in question. So I'd say in the case of mouse click, it accidentally works as expected. :/

Assigning to myself tentatively. Boris, if you want to take this, feel free to take over me. Right now I have no idea where is the right place to do. I will think it during traveling Tokyo today.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I've thought a bit. It seems really hard to solve this... The style change which triggers the transition is a class name change on the target element. That means that we can't tell whether the computed style value for the CSS property (opacity for example) is going to be changed by the class name change until we traverse the element in the normal styling, but we need to flush the throttled styling before it.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Why? We flush the throttled restyles in some other cases, like here. Maybe that condition should be less optimistic, as aRoot is null for the regular full document styling?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(collision happens during writing this,)

Maybe, a right place is here in EffectCompositor::PreTraverseInSubTree. Even if it's a suboptimal, we need to flush throttled style if the each target element is dirty. I know this way doesn't fix if the transition is triggered by CSSOM change, but we have already an open bug for the case. (I couldn't find it now though)

Comment 8

[Kevin Doughty]

Attachment: firefoxTransitionBugTestTimeout.html

Comment 9

[Kevin Doughty]

I was not able to fix the bug with the following changes:

    const NonOwningAnimationTarget& target = aIter.Key();
    if (!flushThrottledRestyles && !aIter.Data()) {
      Element *element = target.mElement;
      //Element *element = target.mElement->GetParentElement();
      //Element *element = aRoot;
      if (element) {
        nsPresContext* presContext = nsContentUtils::GetContextForContent(element);
        if (presContext) {
          presContext->PresShell()->SetNeedThrottledAnimationFlush();
        }
      }
      return returnTarget;
    }

I tried the target element, its parent, and aRoot, because why not.

I added a better test case that uses timeout instead, as I understand it events are not relevant except by accident.

Here are some verbose logs that I've been looking at, if anyone is interested. It used target.mElement for flushing but there was no difference:
https://pastebin.com/raw/sKGR05H5

Comment 10

[Emilio Cobos Álvarez [:emilio]]

That is still returning an empty returnTarget instead of target, so it will do nothing. If you remove this line (and the whole condition around it I guess) I suspect it'd fix the bug. But that's not quite the right fix.

Comment 11

[Kevin Doughty]

It does fix the bug, for key, wheel, and timeout. I returned target but did not remove the condition around it. I look forward to seeing what you do for the actual fix and final patch. Here are the updated logs:
https://pastebin.com/raw/rdC1MpND

Comment 12

[Kevin Doughty]

s/for key, wheel, and timeout/for everything key, wheel, timeout, and the animationend case that I never uploaded

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I've already pushed a try run. The result is not so interesting since I am almost 100% sure there is no automated tests affected by this change. The only one my concern is performance regression. A fun part of this is writing automated tests (a reftest I assume).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d210d1c294fcce6eb48a69277b8a222a3a6dd2e7

Comment 14

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 15

[Timea Cernea [:tbabos][inactive]]

Unfortunately, mozregression couldn't bisect further, but here is the regression-range:
6:25.19 INFO: Last good revision: 93dd2e456c0ecca00fb4d28744e88078a77deaf7 (2017-09-06)
6:25.19 INFO: First bad revision: b4c1ad9565ee9d00d96501c4a83083daf25c1413 (2017-09-07)
6:25.19 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=93dd2e456c0ecca00fb4d28744e88078a77deaf7&tochange=b4c1ad9565ee9d00d96501c4a83083daf25c1413

Comment 16

[Kevin Doughty]

Closed as fixed, don't know when