[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