Closed Bug 1461070 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: comm, Assigned: emilio)

Details

(Keywords: parity-chrome, parity-edge, testcase)

Attachments

(2 files)

tc-20180511.html
1.05 KB, text/html
Details
Bug 1461070: Skip starting other transitions based on specified, not started transitions.
59 bytes, text/x-review-board-request
birtles
: review+
dbaron
: review+
Details
Attached file 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.
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;
I've forgotten to add a reference. :/

[1] https://drafts.csswg.org/css-transitions/#transition-property-property
Has STR: --- → yes
Component: Untriaged → CSS Parsing and Computation
Keywords: parity-chrome, parity-edge, testcase
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → DOM: Animation
Ever confirmed: true
Assignee: nobody → emilio
Status: NEW → ASSIGNED
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 {}).
Attachment #8975234 - Flags: review?(bbirtles) → review+
(Switching component to CSS Parsing and Computation since we generally use that for CSS transitions/animations specific handling, especially the code in nsTransitionManager/nsAnimationManager.)
Component: DOM: Animation → CSS Parsing and Computation
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!
Attachment #8975234 - Flags: review?(dbaron)
Priority: -- → P3
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.
Attachment #8975234 - Flags: review?(dbaron) → review+
Yeah, I did check and Blink and WebKit pass the WPT with web animations enabled.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0312bc7d925
Skip starting other transitions based on specified, not already-started transitions. r=birtles,dbaron
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/11074 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/e0312bc7d925
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: