Summary: | Update shape-inside/shape-outside CSS Exclusion properties | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bear Travis <betravis> | ||||||||||||||
Component: | CSS | Assignee: | Bear Travis <betravis> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dglazkov, macpherson, menard, rniwa, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 82363 | ||||||||||||||||
Attachments: |
|
Description
Bear Travis
2012-03-27 12:03:13 PDT
Created attachment 134122 [details] Proposed Patch Simple name-change patch updating the exclusions css properties "-webkit-wrap-shape-inside" and "-webkit-wrap-shape-outside" to "-webkit-shape-inside" and "-webkit-shape-outside". The current spec is located here: http://dev.w3.org/csswg/css3-exclusions/ Comment on attachment 134122 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134122&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2329 > + case CSSPropertyWebkitShapeOutside: static const int computedProperties[] = doesn't list it. That's odd and it's a bug to me. Maybe you could add a test case to cover where computedProperties is used in this file. > Source/WebCore/css/CSSParser.cpp:2400 > + return parseWrapShape((propId == CSSPropertyWebkitShapeInside), important); Why you don't remove this function to parseShape (or some better name)? (In reply to comment #2) > (From update of attachment 134122 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134122&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2329 > > + case CSSPropertyWebkitShapeOutside: > > static const int computedProperties[] = doesn't list it. That's odd and it's a bug to me. Maybe you could add a test case to cover where computedProperties is used in this file. And make sure CSSPropertyWebkitShapeOutside and CSSPropertyWebkitShapeInside are taken into account. > > > Source/WebCore/css/CSSParser.cpp:2400 > > + return parseWrapShape((propId == CSSPropertyWebkitShapeInside), important); > > Why you don't remove this function to parseShape (or some better name)? Created attachment 134402 [details]
updated patch
thanks for the quick feedback.
I have added shape-inside and shape-outside to the list of computed style properties, and updated the tests that check these properties.
I have also renamed the parsing methods to make them more clear. parseShape > parseClipShape, and parseWrapShape > parseExclusionShape.
Comment on attachment 134402 [details]
updated patch
Ok you need to seek a reviewer now :)
Comment on attachment 134402 [details] updated patch Attachment 134402 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12193013 New failing tests: svg/css/getComputedStyle-basic.xhtml Created attachment 134461 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 134618 [details]
updated patch
updating the expected test results for the failing test case
svg computed styles should include -webkit-shape properties
Comment on attachment 134618 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=134618&action=review > LayoutTests/ChangeLog:11 > + Reviewed by NOBODY (OOPS!). This line should appear before the description but after the bug title and the bug url. > LayoutTests/platform/chromium-mac-snowleopard/svg/css/getComputedStyle-basic-expected.txt:450 > +rect: style.getPropertyValue(-webkit-shape-inside) : auto > +rect: style.getPropertyCSSValue(-webkit-shape-inside) : [object CSSPrimitiveValue] > +rect: style.getPropertyValue(-webkit-shape-outside) : auto > +rect: style.getPropertyCSSValue(-webkit-shape-outside) : [object CSSPrimitiveValue] I don't understand. Why are we adding these lines now? And ditto for all lines that are added below. > Source/WebCore/ChangeLog:11 > + Reviewed by NOBODY (OOPS!). Ditto. Created attachment 134672 [details]
updated patch
updating changelogs
This patch adds -webkit-shape-inside and -webkit-shape-outside to the list of computed shape properties (CSSComputedStyleDeclaration::computedProperties)
Because they were not there before, they were not showing up in the result of a getComputedStyle js call.
With this addition, we need to update the tests that look at all the properties of the computed style.
(In reply to comment #10) > This patch adds -webkit-shape-inside and -webkit-shape-outside to the list of computed shape properties (CSSComputedStyleDeclaration::computedProperties) > Because they were not there before, they were not showing up in the result of a getComputedStyle js call. Can we separate this change? It's hard to review a patch when it does more than one thing at a time. (In reply to comment #11) > Can we separate this change? It's hard to review a patch when it does more than one thing at a time. Yes, I will separate this out into another bug and patch. Created attachment 134682 [details] simplified patch, split with 82667 simplified change after separating out the code to add the computed properties (bug 82667) Comment on attachment 134682 [details] simplified patch, split with 82667 Clearing flags on attachment: 134682 Committed r112629: <http://trac.webkit.org/changeset/112629> All reviewed patches have been landed. Closing bug. |