Summary: | CSS3 calc: Implement CSSOM support | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lea Verou <lea> | ||||||||
Component: | CSS | Assignee: | Mike Lawther <mikelawther> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarcelo, kling, koivisto, macpherson, menard, mikelawther, nat, rniwa, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://result.dabblet.com/gist/2467073 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 16662 | ||||||||||
Attachments: |
|
Description
Lea Verou
2012-04-22 14:41:58 PDT
Yup, WebKit drops it in Chrome Version 20.0.1105.0 dev Thanks for the textcase. Created attachment 143464 [details]
Patch
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? Created attachment 144714 [details]
Patch
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. >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).
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) ... Created attachment 144726 [details]
Patch for landing
Comment on attachment 144726 [details] Patch for landing Clearing flags on attachment: 144726 Committed r118900: <http://trac.webkit.org/changeset/118900> All reviewed patches have been landed. Closing bug. |