1917071 - Animation.commitStyles() with fill: "forwards" causes transitions to fire

Status: ASSIGNED

(Core :: DOM: Animation, defect)

Description

[John Renner]

Attachment: ffbug.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:130.0) Gecko/20100101 Firefox/130.0

Steps to reproduce:

Minimal working example is attached as a file. This is a description of the problem in general:

1. Start with an element with a transition defined on a property (in the attached example, it's opacity).

2. Animate that property using an animation with fill: "forwards".

3. When that animation finishes, call .commitStyles() and .cancel()

Actual results:

The element briefly flashes back to the pre-animation style, before transitioning to the post-animation style.

For example (as in the attached repro), if we have a 1s transition on opacity, and we animate it from 0 to 1, then when we .commitStyles() and .cancel() it flashes invisible and then transitions to 1 over a second.

Expected results:

The transition should not trigger because style is unchanged. When the style is committed the element should remain at opacity 1.

In Chrome, and Safari you can see this working.

Comment 1

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

The severity field is not set for this bug.
:boris, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Boris Chiou [:boris]]

I can reproduce this easily. Thanks for the testcase.

Comment 3

[Boris Chiou [:boris]]

BTW, I can also reproduce this on Safari Technology Preview. Only Chrome cannot reproduce this.
(I may have to check which behavior is expected per the spec. However, I marked this a bug for now because Chrome doesn't agree Firefox's behavior.)

Comment 4

[John Renner]

Huh, strange I could've sworn I tried it in safar 17, but I also recently updated so I can't go back and check.

Comment 5

[Daniel Holbert [:dholbert]]

FWIW, I tested Safari 17.3 and 16.5 and they both match Firefox here (fading from transparent to opaque twice in a row).

Comment 6

[John Renner]

A quick read of the spec leaves things... a little unclear to me.

According to the web animations spec for commitStyles() ref:

Unlike most other methods defined on this interface, calling this method does trigger a style change event (see § 6.13 Model liveness).

Meaning it's possible for a CSS transition to fire according if ref:

- the element does not have a running transition for the property,
- the before-change style is different from the after-change style for that property, and the values for the property are transitionable,
- the element does not have a completed transition for the property or the end value of the completed transition is different from the after-change style for the property,
- there is a matching transition-property value, and
- the combined duration is greater than 0s, 

So for this bug report, the only interesting condition here is the second bullet point. Is the before-change style different form the after-change style?

The definition of before-change is found in the same section:

define the before-change style as the computed values of all properties on the element as of the previous style change event, except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time.

Now, this bug could be that we're treating JS Animations the same as declarative CSS Animations (that is, we're excluding them from the calculation of before-style and therefore triggering an animation from opacity 0 to 1).

The routine for commitStyles explicitly does not remove the animation from the effect stack, so the computed style of the element, while it writes the computed style to the element's styles. In theory our before-change style should be equal to our computed style which is equal to our after-change style meaning we shouldn't begin a transition.

Comment 7

[Boris Chiou [:boris]]

I cc Brian because he may have more information to clear this behavior. (Sorry for the redundant ni)

Comment 8

[Brian Birtles (:birtles)]

Sorry, I've been away for a few days for personal reasons. John's analysis in comment 6 matches my understanding.

As I understand it, at the point when commitStyles is called, the before-change and after-change style should both have an opacity of 1.

Furthermore, when cancel is called and the next animation frame is composed, the before-change and after-change style should still have an opacity of 1.

So I guess we need to debug why that's not happening and see if it's a bug or if there's more to it.

Comment 9

[Brian Birtles (:birtles)]

canalun and I had a look at this today and so far we can see that the before-change style ends up having opacity: 0 in this case. canalun is going to have a further look into why that is the case.

Comment 10

[Brian Birtles (:birtles)]

I've also verified that this bug goes back to Firefox 69 when commitStyles was implemented so this is not a regression.

Comment 11

[Brian Birtles (:birtles)]

Having a think about this, I think this might partly be a spec issue. Specifically the spec says that commitStyles "does trigger a style change event" but it doesn't say if that happens before or after committing the computed styles.

At least in Firefox we flush styles before committing so I think we end up with a sequence like:

1. Call commitStyles.
   a. Flush styles. Animation layer of cascade has opacity: 1.
   b. Update style attribute setting it to opacity: 1.
2. Call cancel().
3. On the next frame, perform a restyle.

At point 3, we build the before-change style by taking the existing computed values and then updating the styles due to animation per the spec:

define the before-change style as the computed values of all properties on the element as of the previous style change event, except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time.

So we have our existing computed styles and we update just the animations and transitions layers. In Gecko we call this the animation-only restyle. As a result of this we we end up removing the opacity: 1 from the animation layer since the animation has been cancelled.

The opacity: 0 from the author styles is still there and the opacity: 1 inline style is part of the after-change style. As a result the before-change style has a computed opacity of 0 and we end up starting a transition.

At least that's my guess at what is happening. If so this is arguably per-spec but the intention of the spec is definitely that we shouldn't fire a transition here.

I guess we could try flushing styles at the end of commitStyles and if that works (i.e. fixes this issue and doesn't break other tests) try to spec it? Alternatively, perhaps we can do a more localized fix where we update just the computed style of the properties affected by commitStyles? We should also check what Blink does here.

Long-term, we're planning to change the behavior of commitStyles slightly in this issue: https://github.com/w3c/csswg-drafts/issues/5394 However, I don't think that change would affect this case.

Comment 12

[Kanaru Sato [:canalun]]

Brian's guess is correct as far as I checked, thanks! :)

So I'm now trying the following two that Brian mentioned:

1. In commitStyles, if we execute a second flush after updating the inline styles, will it work properly? (It includes performance considerations as we cannot omit the first one due to spec requirements.)

2. How does Blink implements this?

Comment 13

[Kanaru Sato [:canalun]]

Attachment: Bug 1917071 - Flush style at the end of CommitStyles. r=birtles

Currently, CommitStyles commit inline style AFTER flushing style. So, if
the animation is canceled in the same frame as CommitStyles, the added
inline style is not counted as before-change style in the next frame.
This leads to unexpected CSS transition in certain situations.

Comment 14

[Kanaru Sato [:canalun]]

Executing a flush again at the end of CommitStyles looks work :)
At least, it passes wpt about Web Animations.
So I've created a PR, with the intention of sharing the code.

I'll dig into Blink implementation, also asking Blink team some hints :)

Comment 15

[Brian Birtles (:birtles)]

Just updating the status of this bug: the patch is r+ but that's contingent on the proposed spec change being merged.

Google have suggested an alternative interpretation of the spec and a revision to how before/after change styles are defined which I need to review to see if they make sense.

Comment 16

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 17

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

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:canalun, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #15)
Any update here?

Comment 19

[Brian Birtles (:birtles)]

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

(In reply to Brian Birtles (:birtles) from comment #15)
Any update here?

As per comment 15, this is basically blocked on whether or not we adopt the new before/after-change style approach proposed by Google in https://github.com/w3c/csswg-drafts/pull/6688.

I reviewed that proposal and found some issues (https://github.com/w3c/csswg-drafts/pull/6688#issuecomment-2475490626) but there's been no further response from Google other than acknowledging the issue.

I think we should first land bug 1947061 once that's shipped and CSS issue #5394 is resolved we can revisit it. If we need a quick fix here, I think landing this patch without the WPT would be ok.