Bug 100299 – [CSS Exclusions] shape-outside on floats for rounded rectangle shapes

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

[Bem Jones-Bey]

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

[Bem Jones-Bey]

Created attachment 193027 [details]
Patch

Checking to see if the bots think that this needs special tweaks for subpixel layout, since the current tweaks don't work for me locally.

[WebKit Review Bot]

Attachment 193027 [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-rounded-rectangle-expected.html', u'LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rounded-rectangle.html']" exit_code: 1
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 4 files


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

[Bem Jones-Bey]

Created attachment 193137 [details]
Patch

After looking at the code (and no complaints from EWS), I think that floats aren't laid out in the same subpixel fashion as the inline text content, so right now, I don't need the subpixel utils to have the test work properly.

[Bem Jones-Bey]

Created attachment 193155 [details]
Patch

Removing comment because it's wrong, but still not using subpixel utils because they shouldn't be needed since I'm using floats to stand in for floats, where in the shape inside case, it has to compensate for the fact that floats aren't the same as shape-inside for positioning purposes.

[Dirk Schulze]

Comment on attachment 193155 [details]
Patch

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

r=me

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:172
> +    if (floatValue == "right") {
> +        dimensions.shapeX = -dimensions.shapeWidth;
> +    }

The braces are not necessary here.

[WebKit Review Bot]

Comment on attachment 193155 [details]
Patch

Clearing flags on attachment: 193155

Committed r145847: <http://trac.webkit.org/changeset/145847>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

Comment on attachment 193155 [details]
Patch

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

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:23
> +    if (lineTop < dimensions.shapeHeight && lineBottom > dimensions.shapeHeight)

Part of this code is confusing and unneeded: you can't have lineTop >= dimensions.shapeHeight the way you are calling xOutset.

> LayoutTests/fast/exclusions/resources/rounded-rectangle.js:141
> +        + dimensions.shapeRadiusX + "px, "
> +        + dimensions.shapeRadiusY + "px); "

The specification has a 'curve' keyword before these 2. Couldn't find a bug about it though.

[Bem Jones-Bey]

(In reply to comment #8)
> (From update of attachment 193155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193155&action=review

My apologies, I didn't realize this had made it onto your queue, so I got another review for this yesterday. Thanks for the feedback, I'll incorporate it into my patch for Bug 98673.

> 
> > LayoutTests/fast/exclusions/resources/rounded-rectangle.js:23
> > +    if (lineTop < dimensions.shapeHeight && lineBottom > dimensions.shapeHeight)
> 
> Part of this code is confusing and unneeded: you can't have lineTop >= dimensions.shapeHeight the way you are calling xOutset.

You're right, and I've updated it in my patch for Bug 98673.

> 
> > LayoutTests/fast/exclusions/resources/rounded-rectangle.js:141
> > +        + dimensions.shapeRadiusX + "px, "
> > +        + dimensions.shapeRadiusY + "px); "
> 
> The specification has a 'curve' keyword before these 2. Couldn't find a bug about it though.

IIRC, I had the same issue with the spec in the past, and it was explained to me that it isn't actually a keyword, it's just a way of describing the 5th and 6th arguments. I'll bug Alan about that again, though. I'm pretty sure that the syntax we have implemented currently is correct.