Bug 86160 – CSS3 calc: blending involving expressions

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

[Mike Lawther]

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.

[Mike Lawther]

Created attachment 142952 [details]
Patch for landing

[Mike Lawther]

Ignore that patch, accidentally added via webkit-patch's parsing of the changelog.

[Mike Lawther]

*** Bug 89064 has been marked as a duplicate of this bug. ***

[Mike Lawther]

Created attachment 149251 [details]
Patch

[Mike Lawther]

Created attachment 149255 [details]
Patch

[Tony Chang]

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).

[Mike Lawther]

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.

[Tony Chang]

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

[Mike Lawther]

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 :(

[Tony Chang]

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.

[Dean Jackson]

(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.

[Mike Lawther]

Created attachment 149669 [details]
Patch

[Mike Lawther]

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.

[WebKit Review Bot]

Comment on attachment 149669 [details]
Patch

Clearing flags on attachment: 149669

Committed r121351: <http://trac.webkit.org/changeset/121351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.