Bug 105442 - Querying transition-timing-function value on the computed style does not return keywords when it should.
: Querying transition-timing-function value on the computed style does not retu...
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: WebExposed
: 106083
: 93136
  Show dependency treegraph
 
Reported: 2012-12-19 08:56 PST by
Modified: 2013-01-04 04:12 PST (History)


Attachments
Patch (4.92 KB, patch)
2012-12-20 03:48 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.66 KB, patch)
2012-12-20 04:25 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.59 KB, patch)
2012-12-28 11:53 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.63 KB, patch)
2012-12-28 12:06 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Attempt to fix WK2 builds with CoordinatedGraphics (48.97 KB, patch)
2012-12-28 13:18 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Another attempt to fix WK2 builds with CoordinatedGraphics (49.00 KB, patch)
2012-12-28 13:29 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (49.41 KB, patch)
2013-01-02 05:52 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (49.04 KB, patch)
2013-01-03 06:22 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (48.86 KB, patch)
2013-01-03 11:42 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-12-19 08:56:51 PST
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.
------- Comment #1 From 2012-12-20 03:48:27 PST -------
Created an attachment (id=180318) [details]
Patch
------- Comment #2 From 2012-12-20 04:25:11 PST -------
Created an attachment (id=180321) [details]
Patch
------- Comment #3 From 2012-12-20 12:31:09 PST -------
[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().
------- Comment #4 From 2012-12-20 13:15:27 PST -------
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.
------- Comment #5 From 2012-12-20 14:41:16 PST -------
(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.
------- Comment #6 From 2012-12-20 17:53:05 PST -------
(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)
------- Comment #7 From 2012-12-21 04:57:53 PST -------
(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.
------- Comment #8 From 2012-12-28 11:53:43 PST -------
Created an attachment (id=180890) [details]
Patch
------- Comment #9 From 2012-12-28 12:00:55 PST -------
(From update of attachment 180890 [details])
Attachment 180890 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15550954
------- Comment #10 From 2012-12-28 12:01:07 PST -------
(From update of attachment 180890 [details])
Attachment 180890 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15549880
------- Comment #11 From 2012-12-28 12:02:45 PST -------
(From update of attachment 180890 [details])
Attachment 180890 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15546978
------- Comment #12 From 2012-12-28 12:06:59 PST -------
Created an attachment (id=180891) [details]
Patch
------- Comment #13 From 2012-12-28 12:15:29 PST -------
(From update of attachment 180891 [details])
Attachment 180891 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15551883
------- Comment #14 From 2012-12-28 12:23:20 PST -------
(From update of attachment 180891 [details])
Attachment 180891 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15559789
------- Comment #15 From 2012-12-28 13:03:43 PST -------
(From update of attachment 180891 [details])
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
------- Comment #16 From 2012-12-28 13:18:58 PST -------
Created an attachment (id=180895) [details]
Attempt to fix WK2 builds with CoordinatedGraphics
------- Comment #17 From 2012-12-28 13:29:13 PST -------
Created an attachment (id=180898) [details]
Another attempt to fix WK2 builds with CoordinatedGraphics
------- Comment #18 From 2012-12-29 01:35:39 PST -------
(From update of attachment 180898 [details])
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?
------- Comment #19 From 2013-01-02 05:52:19 PST -------
Created an attachment (id=181027) [details]
Patch
------- Comment #20 From 2013-01-02 20:59:56 PST -------
(From update of attachment 181027 [details])
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.
------- Comment #21 From 2013-01-03 06:22:32 PST -------
Created an attachment (id=181170) [details]
Patch
------- Comment #22 From 2013-01-03 07:46:21 PST -------
(From update of attachment 181170 [details])
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
------- Comment #23 From 2013-01-03 07:49:52 PST -------
(From update of attachment 181170 [details])
Putting cq? again as the test failing is completely unrelated to my change.
------- Comment #24 From 2013-01-03 10:35:03 PST -------
(From update of attachment 181170 [details])
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.
------- Comment #25 From 2013-01-03 11:42:33 PST -------
Created an attachment (id=181197) [details]
Patch for landing
------- Comment #26 From 2013-01-03 12:24:06 PST -------
(From update of attachment 181197 [details])
Clearing flags on attachment: 181197

Committed r138728: <http://trac.webkit.org/changeset/138728>
------- Comment #27 From 2013-01-03 12:24:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #28 From 2013-01-04 01:43:46 PST -------
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
------- Comment #29 From 2013-01-04 04:12:15 PST -------
(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.