Bug 105442 – Querying transition-timing-function value on the computed style does not return keywords when it should.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexis Menard (darktears)]

Consider :

style.transitionTimingFunction = "ease";

computedStyle.transitionTimingFunction will return cubic-bezier(0.25, 0.1, 0.25, 1) as Opera or FF for example.

It is valid for ease-out, ease-in, ease-in-out, ease where the return value will be similar (e.g. cubic-bezier(x1, y1 , x2 , y2)).

For some reason linear does not return cubic-bezier(0, 0, 1, 1) but "linear" making it inconsistent with other possible values and also inconsistent with other UA.

[Alexis Menard (darktears)]

Created attachment 180318 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 180321 [details]
Patch

[Alexis Menard (darktears)]

[16:17] <smfr> darktears: at some point css-wg talked about returning the "shorthand" functions
[16:17] <smfr> darktears: does the spec say anything about this?
[16:17] <darktears> smfr: not a word 
[16:17] <darktears> smfr: the serialization is not really documented, at least I made them consistent 
[16:18] <darktears>  /s/made/make
[16:18] <smfr> TabAtkins: any opinion on ^^
[16:20] <TabAtkins> smfr: Serialization is undefined, obviously.  I'm fine with always returning a cubic-bezier().

[Dean Jackson]

I disagree here. I think we should return 'linear' when we know it is a linear timing function. The fact that we implement it under the hood as a cubic bezier doesn't need to be exposed.

We originally returned cubic-bezier in all cases, but once we added the steps timing function it meant that developers had to do more than just look for a regular expression if they were going to interpret computed timing functions. So I figured it was better to be clear (and closer to what the developer specified) and return 'linear' where possible. I probably intended to do the same to the 'ease' forms, but they are a little trickier.

You could also imagine a future where we have more timing function forms - like "bounce" or trig functions.

[Tab Atkins]

(In reply to comment #4)
> I disagree here. I think we should return 'linear' when we know it is a linear timing function. The fact that we implement it under the hood as a cubic bezier doesn't need to be exposed.
> 
> We originally returned cubic-bezier in all cases, but once we added the steps timing function it meant that developers had to do more than just look for a regular expression if they were going to interpret computed timing functions. So I figured it was better to be clear (and closer to what the developer specified) and return 'linear' where possible. I probably intended to do the same to the 'ease' forms, but they are a little trickier.
> 
> You could also imagine a future where we have more timing function forms - like "bounce" or trig functions.

If we do that, then we should go ahead and do it for all of them.  We can either guess based on float equality, or just plumb a flag through so that the parser can tell you when it read a keyword.

[Dean Jackson]

(In reply to comment #5)
 
> If we do that, then we should go ahead and do it for all of them.  We can either guess based on float equality, or just plumb a flag through so that the parser can tell you when it read a keyword.

Yes, I agree. Plumbing it through is a bit annoying, but otherwise we might return linear if someone specified cubic-bezier(0, 0, 1, 1)

[Alexis Menard (darktears)]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > If we do that, then we should go ahead and do it for all of them.  We can either guess based on float equality, or just plumb a flag through so that the parser can tell you when it read a keyword.
> 
> Yes, I agree. Plumbing it through is a bit annoying, but otherwise we might return linear if someone specified cubic-bezier(0, 0, 1, 1)

Ok I'll give a shot and rename the bug later if needed.

[Alexis Menard (darktears)]

Created attachment 180890 [details]
Patch

[Early Warning System Bot]

Comment on attachment 180890 [details]
Patch

Attachment 180890 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15550954

[Early Warning System Bot]

Comment on attachment 180890 [details]
Patch

Attachment 180890 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15549880

[EFL EWS Bot]

Comment on attachment 180890 [details]
Patch

Attachment 180890 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15546978

[Alexis Menard (darktears)]

Created attachment 180891 [details]
Patch

[Early Warning System Bot]

Comment on attachment 180891 [details]
Patch

Attachment 180891 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15551883

[EFL EWS Bot]

Comment on attachment 180891 [details]
Patch

Attachment 180891 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15559789

[Build Bot]

Comment on attachment 180891 [details]
Patch

Attachment 180891 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15557836

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
fast/sub-pixel/transformed-iframe-copy-on-scroll.html

[Alexis Menard (darktears)]

Created attachment 180895 [details]
Attempt to fix WK2 builds with CoordinatedGraphics

[Alexis Menard (darktears)]

Created attachment 180898 [details]
Another attempt to fix WK2 builds with CoordinatedGraphics

[Kenneth Rohde Christiansen]

Comment on attachment 180898 [details]
Another attempt to fix WK2 builds with CoordinatedGraphics

View in context: https://bugs.webkit.org/attachment.cgi?id=180898&action=review

LGTM

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
> +    }
> +    if (tf->isStepsTimingFunction()) {
> +        const StepsTimingFunction* stf = static_cast<const StepsTimingFunction*>(tf);
> +        return CSSStepsTimingFunctionValue::create(stf->numberOfSteps(), stf->stepAtStart());
> +    }
> +    return CSSLinearTimingFunctionValue::create();

I would suggest a newline around this if

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1122
>              const TimingFunction* tf = animList->animation(i)->timingFunction().get();
> -            if (tf->isCubicBezierTimingFunction()) {
> -                const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(tf);
> -                list->append(CSSCubicBezierTimingFunctionValue::create(ctf->x1(), ctf->y1(), ctf->x2(), ctf->y2()));
> -            } else if (tf->isStepsTimingFunction()) {
> -                const StepsTimingFunction* stf = static_cast<const StepsTimingFunction*>(tf);
> -                list->append(CSSStepsTimingFunctionValue::create(stf->numberOfSteps(), stf->stepAtStart()));
> -            } else {
> -                list->append(CSSLinearTimingFunctionValue::create());
> -            }
> +            list->append(createAnimationValue(tf));
>          }
>      } else {
>          // Note that initialAnimationTimingFunction() is used for both transitions and animations
>          RefPtr<TimingFunction> tf = Animation::initialAnimationTimingFunction();
> -        if (tf->isCubicBezierTimingFunction()) {
> -            const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(tf.get());
> -            list->append(CSSCubicBezierTimingFunctionValue::create(ctf->x1(), ctf->y1(), ctf->x2(), ctf->y2()));
> -        } else if (tf->isStepsTimingFunction()) {
> -            const StepsTimingFunction* stf = static_cast<const StepsTimingFunction*>(tf.get());
> -            list->append(CSSStepsTimingFunctionValue::create(stf->numberOfSteps(), stf->stepAtStart()));
> -        } else {
> -            list->append(CSSLinearTimingFunctionValue::create());
> -        }
> +        list->append(createAnimationValue(tf.get()));

why not use tf.get() both places. like change 

const TimingFunction* tf = animList->animation(i)->timingFunction().get(); to not include the .get() part?

> Source/WebCore/platform/animation/TimingFunction.h:83
> +    enum CubicBezierTimingFunctionType {
> +        Ease, EaseIn, EaseOut, EaseInOut, Custom
> +    };

I believe we should have those on separate lines

> Source/WebCore/platform/animation/TimingFunction.h:88
> +    
> +    static PassRefPtr<CubicBezierTimingFunction> create(CubicBezierTimingFunctionType type, double x1, double y1, double x2, double y2)
>      {
> -        return adoptRef(new CubicBezierTimingFunction(x1, y1, x2, y2));
> +        return adoptRef(new CubicBezierTimingFunction(type, x1, y1, x2, y2));
>      }

In a way I don't like you call it type, when I can reuse a type with different values (which would then be considered an error). What about calling it ...FunctionDescription?

[Alexis Menard (darktears)]

Created attachment 181027 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 181027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181027&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1079
> +static PassRefPtr<CSSValue> createAnimationValue(const TimingFunction* tf)
> +{
> +    if (tf->isCubicBezierTimingFunction()) {

You should expand 'tf' to timingFunction.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1080
> +        const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(tf);

Avoid abbreviations like ctf.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1082
> +            CSSValueID id = CSSValueInvalid;

Don't use 'id' (it's a reserved word in Objective-C).

> Source/WebCore/css/CSSToStyleMap.cpp:460
> -            animation->setTimingFunction(CubicBezierTimingFunction::create(0.42, 0.0, 1.0, 1.0));
> +            animation->setTimingFunction(CubicBezierTimingFunction::create(CubicBezierTimingFunction::EaseIn, 0.42, 0.0, 1.0, 1.0));

Seems redundant to pass in both the enum, and the values that it corresponds to. Would be better to have an overloaded CubicBezierTimingFunction::create() and ctor that takes the enum only.

> Source/WebCore/platform/animation/TimingFunction.h:105
> +            return m_x1 == ctf->m_x1 && m_y1 == ctf->m_y1 && m_x2 == ctf->m_x2 && m_y2 == ctf->m_y2 && m_cubicBezierTimingFunctionDescription == ctf->m_cubicBezierTimingFunctionDescription;

Can't you test the type first; you only need to test the values for custom functions.

[Alexis Menard (darktears)]

Created attachment 181170 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181170 [details]
Patch

Attachment 181170 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15635832

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Alexis Menard (darktears)]

Comment on attachment 181170 [details]
Patch

Putting cq? again as the test failing is completely unrelated to my change.

[Simon Fraser (smfr)]

Comment on attachment 181170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181170&action=review

> Source/WebCore/platform/animation/TimingFunction.h:81
> +    enum CubicBezierTimingFunctionType {

CubicBezierTimingFunctionType is a bit long-winded, esp. since this is in class scope. I think this would be better as TimingFunctionPreset or something, 'Type' seems a bit too general.

> Source/WebCore/platform/animation/TimingFunction.h:122
> +            return (m_cubicBezierTimingFunctionType != Custom && m_cubicBezierTimingFunctionType == ctf->m_cubicBezierTimingFunctionType) || (m_x1 == ctf->m_x1 && m_y1 == ctf->m_y1 && m_x2 == ctf->m_x2 && m_y2 == ctf->m_y2);

I think this would be clearer as two separate if tests.

[Alexis Menard (darktears)]

Created attachment 181197 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181197 [details]
Patch for landing

Clearing flags on attachment: 181197

Committed r138728: <http://trac.webkit.org/changeset/138728>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

We have many new crashes after this patch on debug bots:

http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138728%20(7648)/results.html

[Chris Dumez]

(In reply to comment #28)
> We have many new crashes after this patch on debug bots:
> 
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138728%20(7648)/results.html

I filed Bug 106083 to track the regression and uploaded a bugfix patch there.