[webkit-reviews] review canceled: [Bug 89261] [CSS Exclusions] Floats should respect shape-inside on exclusions : [Attachment 175288] Initial Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 21 09:36:53 PST 2012


Bem Jones-Bey <bjonesbe at adobe.com> has canceled Bem Jones-Bey
<bjonesbe at adobe.com>'s request for review:
Bug 89261: [CSS Exclusions] Floats should respect shape-inside on exclusions
https://bugs.webkit.org/show_bug.cgi?id=89261

Attachment 175288: Initial Patch
https://bugs.webkit.org/attachment.cgi?id=175288&action=review

------- Additional Comments from Bem Jones-Bey <bjonesbe at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=175288&action=review


Removing review flag, since I'm definitely going to need to push a new patch.

>> Source/WebCore/rendering/RenderBlock.cpp:3868
>> +	ExclusionShapeInsideInfo* shapeInsideInfo = exclusionShapeInsideInfo();

> 
> This will only work when the shape-inside is directly set on this block. For
example:
> <div style='shape-inside:rectangle(0, 0, 100px, 100px)'>
>  <div>
>   <div style='float:left'></div>
>   Some inline content...
>  </div>
> </div>
> Should still have the float respect the shape inside.
> You may wish to file a separate bug for this.
> Check out RenderBlockLineLayout.cpp::layoutExclusionShapeInsideInfo.
> You may also need to confirm that you are at the correct logical height
offset.

Filed Bug 102948 for this.

You have a point about the logical height offset,  this convinces me more that
it may be best to move the calculation of the shape-inside offsets to
logicalLeftOffsetForLine and logicalRightOffsetForLine (as I mention below).

>> Source/WebCore/rendering/RenderBlock.cpp:3870
>> +	// equal to the maximum segment length:
https://bugs.webkit.org/show_bug.cgi?id=102846
> 
> As a nit, I've generally seen these as
> // FIXME Bug 102846: Comments
> Really tiny thing, but it'll avoid having to include the whole link

Ok.

>> Source/WebCore/rendering/RenderBlock.cpp:3873
>> +	    logicalLeftOffset += shapeInsideInfo->segments()[0].logicalLeft;
> 
> You'll need to add a fixme & file a bug for dealing with multiple segments.
> computeInlineDirectionPositionsForLine just replaces logicalLeft &
logicalRight offsets, rather than adding them to the existing offsets. Which
method is correct?
> And, given the differences, is it worth it to add tests with
margin/padding/content offsets and shape-inside?

I've filed Bug 102949 for multiple segments.

I believe that what computeInlineDirectionPositionsForLine is doing is
incorrect. That's probably the root of Bug 102715, since the offset has the
padding and border included already. I wonder if we should remove this code
from both computeInlineDirectionPositionsForLine and
computeLogicalLocationForFloat and move it into logicalLeftOffsetForLine
instead. Do you see any issues with that? I believe that would fix both this
bug and Bug 102715.

And once that would be fixed, then I would be happy to add tests with
margins/padding/etc, since the only reason I've omitted them so far is because
it the layout for the text doesn't work with it right now.

>> LayoutTests/fast/exclusions/resources/shape-inside-floats-simple.js:1
>> +if (window.internals)
> 
> Is this script more generic than just for floats?
> Also, if it works for you, you may consider using simple-rectangle.js, which
I was going to eventually convert these tests to.
> The shape-inside-multiple-blocks and shape-inside-subsequent-blocks use the
script.

It's specific to floats for now. But I'll rewrite the test to use
simple-rectangle.js.


More information about the webkit-reviews mailing list