Bug 100299 - [CSS Exclusions] shape-outside on floats for rounded rectangle shapes
Summary: [CSS Exclusions] shape-outside on floats for rounded rectangle shapes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
URL: http://dev.w3.org/csswg/css3-exclusio...
Keywords:
Depends on:
Blocks: 98664
  Show dependency treegraph
 
Reported: 2012-10-24 15:50 PDT by Bem Jones-Bey
Modified: 2013-03-15 13:08 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.09 KB, patch)
2013-03-13 17:26 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (13.33 KB, patch)
2013-03-14 09:26 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (12.86 KB, patch)
2013-03-14 11:09 PDT, Bem Jones-Bey
jchaffraix: review+
jchaffraix: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bem Jones-Bey 2012-10-24 15:50:22 PDT
This bug tracks the implementation of shape-outside on floats for roundedrectangular shape outsides.
Comment 1 Bem Jones-Bey 2013-03-13 17:26:06 PDT
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.
Comment 2 WebKit Review Bot 2013-03-13 17:29:06 PDT
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.
Comment 3 Bem Jones-Bey 2013-03-14 09:26:45 PDT
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.
Comment 4 Bem Jones-Bey 2013-03-14 11:09:55 PDT
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.
Comment 5 Dirk Schulze 2013-03-14 13:51:15 PDT
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.
Comment 6 WebKit Review Bot 2013-03-14 14:13:01 PDT
Comment on attachment 193155 [details]
Patch

Clearing flags on attachment: 193155

Committed r145847: <http://trac.webkit.org/changeset/145847>
Comment 7 WebKit Review Bot 2013-03-14 14:13:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Julien Chaffraix 2013-03-15 11:45:02 PDT
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.
Comment 9 Bem Jones-Bey 2013-03-15 13:08:54 PDT
(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.