Bug 84551 – CSS3 calc: Implement CSSOM support

• Component: CSS
• Status: RESOLVED FIXED

[Lea Verou]

Check the few lines of JavaScript in the testcase, it should be self-explanatory. IE and Firefox show the function, WebKit drops everything.

[Nathaniel Higgins]

Yup, WebKit drops it in Chrome Version 20.0.1105.0 dev

[Mike Lawther]

Thanks for the textcase.

[Mike Lawther]

Created attachment 143464 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 143464 [details]
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:82
> +    if (expression[0] != '(')
> +        result.append('(');

Do we have a test case for this? But I'm somewhat puzzled by this.
What we had something like (100px) + 200px ?

> LayoutTests/css3/calc/cssom.html:10
> +    if (window.layoutTestController) {
> +        window.layoutTestController.dumpAsText();
> +    }    
> +

No curly brackets around a single statement.
Also it's unnecessary to indent the script content like this.

> LayoutTests/css3/calc/cssom.html:30
> +    document.body.style.width = '100%';

Wouldn't be it cleaner to create a new element of which you change the style?

[Mike Lawther]

Created attachment 144714 [details]
Patch

[Mike Lawther]

Comment on attachment 143464 [details]
Patch

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

>> Source/WebCore/css/CSSCalculationValue.cpp:82
>> +        result.append('(');
> 
> Do we have a test case for this? But I'm somewhat puzzled by this.
> What we had something like (100px) + 200px ?

I've added a test case for what you suggest. LIne 265 and 271 wrap every expression with an operator in brackets. So the expression "(100px) + 200px" will come out as "(100px + 200px)" - hence in this case we will *not* need to wrap in an extra pair of brackets, This result of "(100px) + 200px" => "(100px + 200px" is the same as what Firefox does.

The only time we need the extra brackets is when there is only a single term in the expression, eg "100px" will come out as "100px".

>> LayoutTests/css3/calc/cssom.html:10
>> +
> 
> No curly brackets around a single statement.
> Also it's unnecessary to indent the script content like this.

Done.

>> LayoutTests/css3/calc/cssom.html:30
>> +    document.body.style.width = '100%';
> 
> Wouldn't be it cleaner to create a new element of which you change the style?

Done.

[Mike Lawther]

>This result of "(100px) + 200px" => "(100px + 200px" is the same as what Firefox does.

Argh - I meant

  This result of "(100px) + 200px" => "(100px + 200px)" is the same as what Firefox does.

(I missed a closing bracket the first time).

[Ryosuke Niwa]

Comment on attachment 144714 [details]
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:82
> +    if (expression[0] != '(')
> +        result.append('(');

(In reply to comment #6)
> The only time we need the extra brackets is when there is only a single term in the expression, eg "100px" will come out as "100px".

It would be good to codify this information. e.g.
bool expressionHasSingleTerm = expression[0] != '(';
if (expressionHasSingleTerm)
...

[Mike Lawther]

Created attachment 144726 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 144726 [details]
Patch for landing

Clearing flags on attachment: 144726

Committed r118900: <http://trac.webkit.org/changeset/118900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.