Bug 84551

Summary: CSS3 calc: Implement CSSOM support
Product: WebKit Reporter: Lea Verou <lea>
Component: CSSAssignee: 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 Flags
Patch
none
Patch
none
Patch for landing none

Lea Verou
Reported 2012-04-22 14:41:58 PDT
Check the few lines of JavaScript in the testcase, it should be self-explanatory. IE and Firefox show the function, WebKit drops everything.
Attachments
Patch (6.30 KB, patch)
2012-05-22 22:37 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
Patch (6.29 KB, patch)
2012-05-29 23:22 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
Patch for landing (6.37 KB, patch)
2012-05-29 23:42 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Nathaniel Higgins
Comment 1 2012-04-22 15:03:51 PDT
Yup, WebKit drops it in Chrome Version 20.0.1105.0 dev
Mike Lawther
Comment 2 2012-04-22 22:14:39 PDT
Thanks for the textcase.
Mike Lawther
Comment 3 2012-05-22 22:37:53 PDT
Created attachment 143464 [details] Patch
Ryosuke Niwa
Comment 4 2012-05-29 22:08:38 PDT
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
Comment 5 2012-05-29 23:22:44 PDT
Created attachment 144714 [details] Patch
Mike Lawther
Comment 6 2012-05-29 23:25:18 PDT
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
Comment 7 2012-05-29 23:30:04 PDT
>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 8 2012-05-29 23:33:49 PDT
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
Comment 9 2012-05-29 23:42:47 PDT
Created attachment 144726 [details] Patch for landing
WebKit Review Bot
Comment 10 2012-05-30 00:01:28 PDT
Comment on attachment 144726 [details] Patch for landing Clearing flags on attachment: 144726 Committed r118900: <http://trac.webkit.org/changeset/118900>
WebKit Review Bot
Comment 11 2012-05-30 00:01:34 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.