Bug 86160

Summary: CSS3 calc: blending involving expressions
Product: WebKit Reporter: Mike Lawther <mikelawther>
Component: CSSAssignee: Mike Lawther <mikelawther>
Status: RESOLVED FIXED    
Severity: Normal CC: agektmr, dino, ojan, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch for landing
mikelawther: commit-queue-
Patch
none
Patch
none
dependent transitions testcase
none
Patch none

Description Mike Lawther 2012-05-10 16:52:45 PDT
Blending (eg for CSS transitions) when a calc expression is involved is not implemented.

Ideally this would involve being able to evaluate any expressions and perform the blend. Currently at the time the blend function operates, the '100% value' is not known, so an expression such as '100% - 10px' cannot be evaluated to a pixel value result.
Comment 1 Mike Lawther 2012-05-20 23:50:57 PDT
Created attachment 142952 [details]
Patch for landing
Comment 2 Mike Lawther 2012-05-20 23:59:52 PDT
Ignore that patch, accidentally added via webkit-patch's parsing of the changelog.
Comment 3 Mike Lawther 2012-06-17 17:38:55 PDT
*** Bug 89064 has been marked as a duplicate of this bug. ***
Comment 4 Mike Lawther 2012-06-24 23:52:37 PDT
Created attachment 149251 [details]
Patch
Comment 5 Mike Lawther 2012-06-25 00:22:37 PDT
Created attachment 149255 [details]
Patch
Comment 6 Tony Chang 2012-06-25 10:53:00 PDT
Comment on attachment 149255 [details]
Patch

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

Code looks OK, but I'd like to see some more tests.

> Source/WebCore/ChangeLog:8
> +        If either endpoint of a blend involves a colc expression, we create a new

Nit: colc -> calc

> Source/WebCore/platform/Length.cpp:223
> +    return Length(CalculationValue::create(blend.release(), CalculationRangeAll));

Is it necessary to return a calculation Length here?  E.g., if you evaluated 'from' and 'to' to create Fixed/Percent Lengths, could you call blend() from the results?  This might avoid having to allocate 7 objects on the heap for each frame of the transition.

> LayoutTests/css3/calc/transitions.html:13
> +#startCalcEndCalc, #startCalcEndPx, #startCalcEndPercent {
> +    width: -webkit-calc(10% + 50px);
> +    width: -moz-calc(10% + 50px);
> +}

Can we add a second test file that tests a calculation that is based on a transitioning element?  E.g.,
#outer's width is transitioning and #inner's width is a calculation with a percentage size (#inner could either be a fixed calculation or a calculation that is transitioning).
Comment 7 Mike Lawther 2012-06-25 16:58:28 PDT
Thanks for the review Tony!

> Is it necessary to return a calculation Length here?  E.g., if you evaluated 'from' and 'to' to create Fixed/Percent Lengths, could you call blend() from the results?  This might avoid having to allocate 7 objects on the heap for each frame of the transition.

In short, yes, it's necessary to return the CalculationValue.

If we have a Length of type Calculated, then it must be an expression involving both percents and fixed (a pure percent or pure fixed expression gets simplified earlier). So at this stage, the expression can't be evaluated to a pixel result, as we don't know what 100% is.

This is also why WebKit can't blend a fixed and a percent - it can't express one of them in terms of the other.

I know 7 mallocs is a bit gnarly :( An alternative I considered was to introduce a new type of expression node that was a BlendNode - then you'd only need to alloc one of them. But I also figured that was an optimisation that could wait until the behaviour was correct.

Another thing I could do is if one of from or to is not an expression, to multiply that with its progress to eliminate some nodes. It would still mean allocating a new Length though.

How about I file a bug for these optimisations? 

> Can we add a second test file that tests a calculation that is based on a transitioning element?  E.g.,
> #outer's width is transitioning and #inner's width is a calculation with a percentage size (#inner could either be a fixed calculation or a calculation that is transitioning).

Cool - I'll cook something up.
Comment 8 Tony Chang 2012-06-25 17:05:13 PDT
Filing a bug for optimizations sounds fine to me. Thanks!
Comment 9 Mike Lawther 2012-06-25 18:45:12 PDT
Created attachment 149420 [details]
dependent transitions testcase

I've been trying to write a dependent transitions testcase as suggested by Tony. I suspect the transition test framework doesn't support this. 

The attached testcase is my simplified attempt - there is no calc involved. Running it using new-run-webkit-tests I get (only showing relevant results):

PASS - "width" property for "outer" element at 0.5s saw something close to: 300
FAIL - "width" property for "innerSimple" element at 0.5s expected: 150 but saw: 250
PASS - "width" property for "innerSimple" element at 1s saw something close to: 450

and running it in the browser I get 

PASS - "width" property for "outer" element at 0.5s saw something close to: 300
PASS - "width" property for "innerSimple" element at 0.5s saw something close to: 150
PASS - "width" property for "innerSimple" element at 1s saw something close to: 450

Am I doing something wrong in this testcase? It looks like in the NRWT case, innerSimple is at 50% (correct), but is being applied to the completed value of outer (500px), which is incorrect :(

So I can write a test involving dependent calc transitions that passes in the browser, but will fail using NRWT :(
Comment 10 Tony Chang 2012-06-25 20:47:25 PDT
IIRC, the transition testing framework only works on the element being transitioned.

Hmm, maybe it's not possible to write a test like I suggested using the transition testing framework.
Comment 11 Dean Jackson 2012-06-26 13:54:10 PDT
(In reply to comment #9)
> 
> I've been trying to write a dependent transitions testcase as suggested by Tony. I suspect the transition test framework doesn't support this. 

Our transition test framework sucks, sorry. It really does.
Comment 12 Mike Lawther 2012-06-26 20:00:43 PDT
Created attachment 149669 [details]
Patch
Comment 13 Mike Lawther 2012-06-26 20:04:11 PDT
Comment on attachment 149669 [details]
Patch

> Nit: colc -> calc

Oops - fixed.

> Filing a bug for optimizations sounds fine to me. Thanks!

Filed https://bugs.webkit.org/show_bug.cgi?id=90037 to track this.

> Can we add a second test file that tests a calculation that is based on a transitioning element?  E.g.,
> #outer's width is transitioning and #inner's width is a calculation with a percentage size (#inner could either be a fixed calculation or a calculation that is transitioning).

Done. There both types of inner elements (fixed calculation and transitioning calculation) in the test.
Comment 14 WebKit Review Bot 2012-06-27 11:23:28 PDT
Comment on attachment 149669 [details]
Patch

Clearing flags on attachment: 149669

Committed r121351: <http://trac.webkit.org/changeset/121351>
Comment 15 WebKit Review Bot 2012-06-27 11:23:33 PDT
All reviewed patches have been landed.  Closing bug.