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
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Alexis Menard (darktears)
: WebExposed
Depends on: 106083
Blocks: 93136
  Show dependency treegraph
 
Reported: 2012-12-19 08:56 PST by Alexis Menard (darktears)
Modified: 2013-01-04 04:12 PST (History)
13 users (show)

See Also:


Attachments
Patch (4.92 KB, patch)
2012-12-20 03:48 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (18.66 KB, patch)
2012-12-20 04:25 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (43.59 KB, patch)
2012-12-28 11:53 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (43.63 KB, patch)
2012-12-28 12:06 PST, Alexis Menard (darktears)
no flags 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 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 Details | Formatted Diff | Diff
Patch (49.41 KB, patch)
2013-01-02 05:52 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (49.04 KB, patch)
2013-01-03 06:22 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (48.86 KB, patch)
2013-01-03 11:42 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 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 Alexis Menard (darktears) 2012-12-20 03:48:27 PST
Created attachment 180318 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-12-20 04:25:11 PST
Created attachment 180321 [details]
Patch
Comment 3 Alexis Menard (darktears) 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 Dean Jackson 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 Tab Atkins 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 Dean Jackson 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 Alexis Menard (darktears) 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 Alexis Menard (darktears) 2012-12-28 11:53:43 PST
Created attachment 180890 [details]
Patch
Comment 9 Early Warning System Bot 2012-12-28 12:00:55 PST
Comment on attachment 180890 [details]
Patch

Attachment 180890 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15550954
Comment 10 Early Warning System Bot 2012-12-28 12:01:07 PST
Comment on attachment 180890 [details]
Patch

Attachment 180890 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15549880
Comment 11 EFL EWS Bot 2012-12-28 12:02:45 PST
Comment on attachment 180890 [details]
Patch

Attachment 180890 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15546978
Comment 12 Alexis Menard (darktears) 2012-12-28 12:06:59 PST
Created attachment 180891 [details]
Patch
Comment 13 Early Warning System Bot 2012-12-28 12:15:29 PST
Comment on attachment 180891 [details]
Patch

Attachment 180891 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15551883
Comment 14 EFL EWS Bot 2012-12-28 12:23:20 PST
Comment on attachment 180891 [details]
Patch

Attachment 180891 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15559789
Comment 15 Build Bot 2012-12-28 13:03:43 PST
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
Comment 16 Alexis Menard (darktears) 2012-12-28 13:18:58 PST
Created attachment 180895 [details]
Attempt to fix WK2 builds with CoordinatedGraphics
Comment 17 Alexis Menard (darktears) 2012-12-28 13:29:13 PST
Created attachment 180898 [details]
Another attempt to fix WK2 builds with CoordinatedGraphics
Comment 18 Kenneth Rohde Christiansen 2012-12-29 01:35:39 PST
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?
Comment 19 Alexis Menard (darktears) 2013-01-02 05:52:19 PST
Created attachment 181027 [details]
Patch
Comment 20 Simon Fraser (smfr) 2013-01-02 20:59:56 PST
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.
Comment 21 Alexis Menard (darktears) 2013-01-03 06:22:32 PST
Created attachment 181170 [details]
Patch
Comment 22 WebKit Review Bot 2013-01-03 07:46:21 PST
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
Comment 23 Alexis Menard (darktears) 2013-01-03 07:49:52 PST
Comment on attachment 181170 [details]
Patch

Putting cq? again as the test failing is completely unrelated to my change.
Comment 24 Simon Fraser (smfr) 2013-01-03 10:35:03 PST
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.
Comment 25 Alexis Menard (darktears) 2013-01-03 11:42:33 PST
Created attachment 181197 [details]
Patch for landing
Comment 26 WebKit Review Bot 2013-01-03 12:24:06 PST
Comment on attachment 181197 [details]
Patch for landing

Clearing flags on attachment: 181197

Committed r138728: <http://trac.webkit.org/changeset/138728>
Comment 27 WebKit Review Bot 2013-01-03 12:24:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Thiago Marcos P. Santos 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 Chris Dumez 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.