1461070 - Specified transition duration and delay are ignored if both are 0s and a targeted property is specified multiple times in transition-property

Status: RESOLVED FIXED

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

Description

[Makoto Mizukami]

Attachment: tc-20180511.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180427210249

Steps to reproduce:

Repro:
1. Open the attached test case.
2. As described in the test case, put your mouse pointer on the box shown.


Actual results:

The height of the box gradually changes after 1s delay.


Expected results:

The height of the box should change immediately.
Note that this expected behaviour can be observed in Edge and Chromium, as expected.

Comment 1

[Makoto Mizukami]

Let me explain more about this issue.

According to the specification, a property to animate can be specified in transition-property multiple times, and the last matching one and corresponding duration/delay shuold be used for the animation [1].
Additionaly, 0s (i.e. no animation) is acceptable as duration and delay, and a user agent must immediately apply changes in a property for which computed duration/delay are 0s.

Based on the prerequisite above, if a property is specified multiple times in transition-property, and duration and delay corresponding to the last item is 0s, the property should not be animated.
For instance, if the following rule is applied to a block element, its height is expected to be changed immediately, without any delay and transition.

    transition-property: height, height, height;
    transition-duration: 2s, 1s, 0s;
    transition-delay: 2s, 1s, 0s;

However, as far as I see with Fx 59.0.3 and Nightly 62.0a1 (2018-05-10), it animate for 1s after 1s delay.

In order to clarify the issue, I used height for the transition-property in this report.
However, the issue can be reproduced with other properties and shorthands, including 'all'.
Thus, in Fx, a rule that a single property would not animate while others do cannot be specified reasonably, even though it should be realized a rule like this:

    transition-property: all, height;
    transition-duration: 1s, 0s;
    transition-delay: 1s, 0s;

Comment 2

[Makoto Mizukami]

I've forgotten to add a reference. :/

[1] https://drafts.csswg.org/css-transitions/#transition-property-property

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Nice test-case :)

This is not a regression from the new style system fwiw.

This shouldn't be terribly hard to fix, I think this condition is not right and we should at least do bookkeeping to flag the property as already specified.

https://searchfox.org/mozilla-central/rev/babf96cf0c37b608e9663e2af73a4d21819c4af1/layout/style/nsTransitionManager.cpp#472

That's why this is specific to the "0" case.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1461070: Skip starting other transitions based on specified, not started transitions.

Needs a test and all that.

Also I wonder if it'd be worth to rewrite that transition-start / stop loop in
Rust code, and put it in the style crate.

It'd help Servo to fix its completely broken transitions code, WDYT?

Review commit: https://reviewboard.mozilla.org/r/243582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243582/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8975234 [details]
Bug 1461070: Skip starting other transitions based on specified, not started transitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243582/diff/1-2/

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8975234 [details]
Bug 1461070: Skip starting other transitions based on specified, not started transitions.

https://reviewboard.mozilla.org/r/243582/#review249482

This looks good to me but if dbaron is available it would be worth getting him to check it (to be sure this really is the intention of the spec).

::: layout/style/nsTransitionManager.cpp:475
(Diff revision 2)
>      // Check the combined duration (combination of delay and duration)
>      // first, since it defaults to zero, which means we can ignore the
>      // transition.

This comment no longer makes sense here.

::: layout/style/nsTransitionManager.cpp:667
(Diff revision 2)
> +  // A later item in transition-property already specified a transition for this
> +  // property, so we ignore this one.

I believe this comment should go before the HasProperty check above.

::: layout/style/nsTransitionManager.cpp:690
(Diff revision 2)
> +  // The combined duration of this transition is 0 or less, so don't start a
> +  // transition.
> +  if (delay + duration <= 0.0f) {

Nit: This comment assumes the following condition is true so it's a bit awkward to read. It would be better to make the comment begin "If the combined..." (or else stick the comment inside the {}).

Comment 7

[Brian Birtles (:birtles)]

(Switching component to CSS Parsing and Computation since we generally use that for CSS transitions/animations specific handling, especially the code in nsTransitionManager/nsAnimationManager.)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8975234 [details]
Bug 1461070: Skip starting other transitions based on specified, not started transitions.

David, mind skimming over the patch per comment 6? Thanks!

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8975234 [details]
Bug 1461070: Skip starting other transitions based on specified, not started transitions.

https://reviewboard.mozilla.org/r/243582/#review250850

I think this seems like a good interpretation of the spec.  I think I wrote that part of the spec looking at this code, and didn't catch this difference, but at an abstract level this behavior seems better.  It, for example, lets developers transition all of the subproperties of a shorthand except for one of them, by listing the one subproperty separately.

Did you check if it matches other implementations?

It's definitely worth adding a web-platform-test.

::: layout/style/nsTransitionManager.cpp:470
(Diff revision 2)
> -  // ones (tracked using |whichStarted|).
> +  // ones (tracked using |propertiesChecked|).
>    bool startedAny = false;
> -  nsCSSPropertyIDSet whichStarted;
> -  for (uint32_t i = aDisp.mTransitionPropertyCount; i-- != 0; ) {
> +  nsCSSPropertyIDSet propertiesChecked;
> +  for (uint32_t i = aDisp.mTransitionPropertyCount; i--; ) {
> +    // We're not going to look at any further transitions, so we can just avoid
> +    // looking at this if we know it will not start any transition.

"transition" -> "transitions"

::: layout/style/nsTransitionManager.cpp:655
(Diff revision 2)
>    dom::Element* aElement,
>    CSSPseudoElementType aPseudoType,
>    CSSTransitionCollection*& aElementTransitions,
>    const ComputedStyle& aOldStyle,
>    const ComputedStyle& aNewStyle,
> -  bool* aStartedAny,
> +  nsCSSPropertyIDSet& propertiesChecked)

This should be aPropertiesChecked, not propertiesChecked.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Yeah, I did check and Blink and WebKit pass the WPT with web animations enabled.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0312bc7d925
Skip starting other transitions based on specified, not already-started transitions. r=birtles,dbaron

Comment 12

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

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

Comment 13

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 14

[Andrei Ciure[:aciure]]

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

Comment 15

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

Upstream PR merged

Comment 16

[Makoto Mizukami]

I confirmed that the fix is available in Nightly build 20180520100821, whicn now behaves as Edge or Chromium does for this case.
Great thanks to all of you for prompt action!