Bug 89261

Created attachment 175288 [details] Initial Patch Make floats respect shape-inside

The ChangeLog says "No new tests" . You might explain that shape-inside-floats-simple has been revised to cover the new functionality.

I think the conventional WebKit FIXME syntax is FIXME: A complete sentence.

Comment on attachment 175288 [details] Initial Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175288&action=review > Source/WebCore/ChangeLog:12 > + No new tests (OOPS!). Make sure to document the tests you've changed here. > 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. > Source/WebCore/rendering/RenderBlock.cpp:3870 > + // FIXME Take into account the height of the content. The offset should be > + // 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 > Source/WebCore/rendering/RenderBlock.cpp:3873 > + logicalRightOffset += shapeInsideInfo->segments()[0].logicalRight - availableLogicalWidth(); > + 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? > 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.

Comment on attachment 175288 [details] Initial Patch 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.

Comment on attachment 175288 [details] Initial Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175288&action=review >>> 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. Moving this into the offset code sounds like a good idea. My only concern is that we're still going to have to deal with individual segment offsets on multi-segment lines, and I don't know if it will make as much sense in that case. So I would propose waiting until after bug 91878 has landed before making the refactor.

Created attachment 176040 [details] Updated Patch Make floats respect shape-inside

Comment on attachment 176040 [details] Updated Patch It feels weird to me to patch this spot. We already have functions that tell you the left and right offsets for content at a given block offset.... why would you not just patch them? Basically I'm wondering why you didn't patch at a lower level, e.g., logicalLeftOffsetForContent.

(In reply to comment #8) > (From update of attachment 176040 [details]) > It feels weird to me to patch this spot. We already have functions that tell you the left and right offsets for content at a given block offset.... why would you not just patch them? > > Basically I'm wondering why you didn't patch at a lower level, e.g., logicalLeftOffsetForContent. Only because I was waiting for bug 91878 to land before refactoring at a lower level, since it would probably require touching some things that are affected by bug 91878. However at this point, that argument is moot since bug 91878 has landed. I'll update the patch to do it's thing at a lower level. Thanks.

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 176040 [details] [details]) > > It feels weird to me to patch this spot. We already have functions that tell you the left and right offsets for content at a given block offset.... why would you not just patch them? > > > > Basically I'm wondering why you didn't patch at a lower level, e.g., logicalLeftOffsetForContent. > > Only because I was waiting for bug 91878 to land before refactoring at a lower level, since it would probably require touching some things that are affected by bug 91878. However at this point, that argument is moot since bug 91878 has landed. I'll update the patch to do it's thing at a lower level. Thanks. After spending some time going the workings of multiple segments, I don't see a way that this could be moved into a lower level like logicalLeftOffsetForContent and still be able to properly implement positioning floats in the case layout out floats in multi-segment polygons (Bug 102949). I could add something like logicalLeftOffesetForContentInSegment, and put the code there, if you like that better.

Comment on attachment 176040 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176040&action=review Ok we can go with the original patch. Here is my feedback on it. > Source/WebCore/rendering/RenderBlock.cpp:3866 > LayoutUnit logicalLeftOffset = logicalLeftOffsetForContent(logicalTopOffset); // Constant part of left offset. Add the following line: LayoutUnit floatLogicalWidth = logicalRightOffset - logicalLeftOffset; > Source/WebCore/rendering/RenderBlock.cpp:3874 > + logicalRightOffset += shapeInsideInfo->segments()[0].logicalRight - availableLogicalWidth(); This is incorrect with variable width regions. You need to be using the available logical width in the specific region. You have this available by subtracting the logicalLeft and logicalRight offsets. I haven't looked at whether or not you do segment calculation correctly across variable width regions (probably not), but we should at least have this code be prepared for it. Basically change the above line to: logicalRightOffset += shapeInsideInfo->segments()[0].logicalRight - floatLogicalWidth; > Source/WebCore/rendering/RenderBlock.cpp:3878 > LayoutUnit floatLogicalWidth = min(logicalWidthForFloat(floatingObject), logicalRightOffset - logicalLeftOffset); // The width we look for. Now patch this line of code to read: floatLogicalWidth = min(logicalWidthForFloat(floatingObject), floatLogicalWidth);

Thinking about this further I think this can be simplified: logicalRightOffset += shapeInsideInfo->segments()[0].logicalRight - floatLogicalWidth; That is equivalent to: logicalRightOffset = logicalRightOffset + shapeInsideInfo->segments()[0].logicalRight - (logicalRightOffset - logicalLeftOffset); which turns into: logicalRightOffset = logicalLeftOffset + shapeInsideInfo->segments()[0].logicalRight; I find this kind of confusing. Do segments not have positions that include border and padding of the enclosing box? Is this really correct?

(In reply to comment #12) > Thinking about this further I think this can be simplified: > > logicalRightOffset += shapeInsideInfo->segments()[0].logicalRight - floatLogicalWidth; > > That is equivalent to: > > logicalRightOffset = logicalRightOffset + shapeInsideInfo->segments()[0].logicalRight - (logicalRightOffset - logicalLeftOffset); > > which turns into: > > logicalRightOffset = logicalLeftOffset + shapeInsideInfo->segments()[0].logicalRight; > > I find this kind of confusing. Do segments not have positions that include border and padding of the enclosing box? Is this really correct? (In reply to comment #12) > Thinking about this further I think this can be simplified: > > logicalRightOffset += shapeInsideInfo->segments()[0].logicalRight - floatLogicalWidth; > > That is equivalent to: > > logicalRightOffset = logicalRightOffset + shapeInsideInfo->segments()[0].logicalRight - (logicalRightOffset - logicalLeftOffset); > > which turns into: > > logicalRightOffset = logicalLeftOffset + shapeInsideInfo->segments()[0].logicalRight; > > I find this kind of confusing. Do segments not have positions that include border and padding of the enclosing box? Is this really correct? Segment positions are relative to the content box, so they don't include border and padding. So I believe that is correct.

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

Created attachment 179355 [details] Updated Patch Update for review comments.

Comment on attachment 179355 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179355&action=review r=me > Source/WebCore/rendering/RenderBlock.cpp:3824 > + // The segment offsets are relative to the content box. It's good this comment is here, but I do question why segments are using content box coordinates. This is not something we do anywhere else in the code. All other constructs, from positioned elements, to line boxes, to floats, etc. are in border box coordinates, so why have we made segments different? I strongly believe you should express segment positions using border box coordinates instead (and shapes too if they are what led segments to be in content box coordinates). I'm not going to r- over this issue, but I think it's something you should consider. Having exclusions be different than every other layout construct doesn't seem ideal as far as other people coming into the code and trying to understand it. You'll be putting clarifying comments like the one above all over the place as you patch more spots in the code and find you have to do border<->content conversion math in those spots also.

(In reply to comment #16) > (From update of attachment 179355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179355&action=review > > r=me Thanks! > > > Source/WebCore/rendering/RenderBlock.cpp:3824 > > + // The segment offsets are relative to the content box. > > It's good this comment is here, but I do question why segments are using content box coordinates. This is not something we do anywhere else in the code. All other constructs, from positioned elements, to line boxes, to floats, etc. are in border box coordinates, so why have we made segments different? I strongly believe you should express segment positions using border box coordinates instead (and shapes too if they are what led segments to be in content box coordinates). > > I'm not going to r- over this issue, but I think it's something you should consider. Having exclusions be different than every other layout construct doesn't seem ideal as far as other people coming into the code and trying to understand it. You'll be putting clarifying comments like the one above all over the place as you patch more spots in the code and find you have to do border<->content conversion math in those spots also. The coordinate problems will be addressed as part of fixing the generat border/padding issues with shape-inside in bug 102715.

Comment on attachment 179355 [details] Updated Patch Clearing flags on attachment: 179355 Committed r137920: <http://trac.webkit.org/changeset/137920>

All reviewed patches have been landed. Closing bug.