Bug 82365 – Update shape-inside/shape-outside CSS Exclusion properties

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

[Bear Travis]

The draft specification renames -webkit-wrap-shape-inside and -webkit-wrap-shape-outside to -webkit-shape-inside and -webkit-shape-outside
http://dev.w3.org/csswg/css3-exclusions/

[Bear Travis]

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/

[Alexis Menard (darktears)]

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

[Alexis Menard (darktears)]

(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)?

[Bear Travis]

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.

[Alexis Menard (darktears)]

Comment on attachment 134402 [details]
updated patch

Ok you need to seek a reviewer now :)

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bear Travis]

Created attachment 134618 [details]
updated patch

updating the expected test results for the failing test case
svg computed styles should include -webkit-shape properties

[Ryosuke Niwa]

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.

[Bear Travis]

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.

[Ryosuke Niwa]

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

[Bear Travis]

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

[Bear Travis]

Created attachment 134682 [details]
simplified patch, split with 82667

simplified change after separating out the code to add the computed properties (bug 82667)

[WebKit Review Bot]

Comment on attachment 134682 [details]
simplified patch, split with 82667

Clearing flags on attachment: 134682

Committed r112629: <http://trac.webkit.org/changeset/112629>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.