Bug 98673 – [CSS Exclusions] shape-outside on floats for circle and ellipse shapes

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

[Bem Jones-Bey]

This bug tracks the implementation of shape-outside on floats for circle shape outsides.

[Bem Jones-Bey]

*** Bug 98674 has been marked as a duplicate of this bug. ***

[Bem Jones-Bey]

Also for ellipses.

[Bem Jones-Bey]

Created attachment 193348 [details]
Patch

Add support for circles and ellipses for shape-outside.

[WebKit Review Bot]

Attachment 193348 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/resources/rounded-rectangle.js', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-circle-expected.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-circle.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-ellipse-expected.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-ellipse.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp']" exit_code: 1
LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 10 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bem Jones-Bey]

Created attachment 193351 [details]
Patch

Add support for circles and ellipses for shape-outside. I fix my vim config on one machine, but must have forgotten this one. *sigh*

[Bem Jones-Bey]

Created attachment 193359 [details]
Patch

Add a small fix to make xOutset more readable from feedback in Bug 100299.

[WebKit Review Bot]

Attachment 193359 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/resources/rounded-rectangle.js', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-circle-expected.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-circle.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-ellipse-expected.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-ellipse.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp']" exit_code: 1
LayoutTests/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bem Jones-Bey]

Created attachment 193367 [details]
Patch

And I just got caught by the tab issue again.

[Dirk Schulze]

Comment on attachment 193367 [details]
Patch

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

Some comments from me.

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:6
> +function computeRoundedRectDimensions(dimensions) {

Is the function name not bogus if you test for ellipses and circles?

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:8
> +    if (defined(dimensions.shapeCenterX) && defined(dimensions.shapeCenterY)) { // circle or ellipse
> +        if (defined(dimensions.shapeRadius)) { // circle

Comments at the beginning of a line are easier to see IMO.

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:16
> +        }
> +        else { // ellipse

no new line for the else

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:22
> +    }

what happens if it is neither circle nor ellipse here? Something should indicate that this should not happen or a fixme should be added.

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:155
> +    if (defined(dimensions.shapeCenterX) && defined(dimensions.shapeCenterY)) { // circle or ellipse

Comment in it's own line

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:164
> +        }
> +        else { // ellipse

Ditto, no break and new line for comment.

[Bem Jones-Bey]

Created attachment 193392 [details]
Patch

Rename computeRoundedRectDimensions to convertToRoundedRect make its purpose clearer; reformat and add comments per review feedback; and remove some comments that are useless because the next line of code says what type of shape is being created.

[WebKit Review Bot]

Comment on attachment 193392 [details]
Patch

Clearing flags on attachment: 193392

Committed r145982: <http://trac.webkit.org/changeset/145982>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.