Bug 66198

Created attachment 105625 [details] Patch I need to add more tests, so I didn't add a commit log and didn't send for review, yet. Some tests that I'm thinking at are: - floats splitting - different inline objects heights (different font-sizes) - avoid-breaks - force breaks for before and after - check that first line properties are correctly applied when backtracking a line that doesn't fit in the next region - nested blocks with different margins & margin collapsing - flex-box integration

You didn't list padding, but there's a bug in multicol I just found today (https://bugs.webkit.org/show_bug.cgi?id=67232). So that functionality for Regions may need to wait. I thought we'd decided not to split floats across region boundaries. Do you mean float positioning after being pushed across a region boundary?

Created attachment 105778 [details] Patch V2 Yes, floats should not split across regions, added a check for that. Still not finished the patch yet, but I've added some helper testing methods. To test that a specific element fits inside a region just add a class="check regionId" to that element. The className will also be used to print the name of the test when failing, so I added some details like "check region5 start" or "check region5 end" to differentiate between tests. Also for debugging reasons, "#debug" can be added at the end of the URL.

Created attachment 105859 [details] Testcases Here are three testcases - an updated version of the initial test, one testing paragraph splitting, and one testing floats. The float case does not yet work correctly. The float does get pushed to the second region, but the text following the float is fitting into the first region instead of following the float position. My expectation is that text that comes after a float should not normally display above the float. I also noted that with the v2 patch, floats in regions are being pushed instead of sliced, but floats in multicol are still being sliced. I don't know whether there's an opportunity to share the float-pushing logic, but where it's possible we should have all of the content breaking scenarios (regions, multicol, paged media) go through a single code path.

(In reply to comment #4) > Created an attachment (id=105859) [details] > Testcases > > Here are three testcases - an updated version of the initial test, one testing paragraph splitting, and one testing floats. The float case does not yet work correctly. The float does get pushed to the second region, but the text following the float is fitting into the first region instead of following the float position. My expectation is that text that comes after a float should not normally display above the float. According to http://www.w3.org/TR/CSS2/visuren.html#float-position the single rule about the top-edge of the float is about the elements that come before the float. There's nothing about anything that comes after the float. I think the behavior that you noticed is correct. > I also noted that with the v2 patch, floats in regions are being pushed instead of sliced, but floats in multicol are still being sliced. I don't know whether there's an opportunity to share the float-pushing logic, but where it's possible we should have all of the content breaking scenarios (regions, multicol, paged media) go through a single code path. The issue about splitting the floats across regions is not yet solved: http://wiki.csswg.org/spec/css3-regions#issue-17behavior-of-broken-floats . They actually use the same path in code, but I've manually added a check for regions, that's because I'm not sure there's any specification for floats splitting in multi-column elements. Also, there are some LayoutTests in WebKit that expect floats to be split across columns.

Created attachment 105950 [details] Patch V3 Reverted the unsplittable floats check from the previous patch. There's an issue that needs more code change and I would like to split that into a different patch. The issue is reproduced when there are two left floats and the one on the right has bigger height. If the float on the right is not small enough to fit in the current region, it will be moved to the following one, but it will not be realigned to the left margin. I think this one can be reproduced with columns and break-inside: avoid. Also blocks with region-break-inside: avoid should just find the first region to fit their needs. Currently, only the following region is checked. Will fix that in a different patch.

Comment on attachment 105950 [details] Patch V3 Attachment 105950 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9578397 New failing tests: printing/no-content-empty-pages.html fast/multicol/vertical-lr/column-break-with-balancing.html fast/multicol/column-break-with-balancing.html printing/page-break-always.html fast/multicol/vertical-rl/column-break-with-balancing.html

Created attachment 105971 [details] Patch V4 Fixed the layout tests.

You should split floats across regions. I don't know why you wouldn't do this. Columns and printing are capable of splitting floats. I see no reason to restrict that functionality.

Comment on attachment 105971 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=105971&action=review > Source/WebCore/rendering/RenderBlock.cpp:2085 > + if (!child->selfNeedsLayout() && view()->hasRenderFlowThread()) > + child->markForFlowThreadRelayoutIfNeeded(); Why selfNeedsLayout and not needsLayout? > Source/WebCore/rendering/RenderBlock.cpp:2258 > + if (!r->selfNeedsLayout() && view()->hasRenderFlowThread()) > + r->markForFlowThreadRelayoutIfNeeded(); Why selfNeedsLayout and not needsLayout? > Source/WebCore/rendering/RenderBlock.cpp:2301 > +void RenderBlock::markForFlowThreadRelayoutIfNeeded() > +{ > + ASSERT(!selfNeedsLayout() && view()->hasRenderFlowThread()); > + // FIXME: We can do better here, we can detect the cases when the change of the logical top > + // is not actually doing any harm on the lines. Until that time, we will force a full relayout here. > + // Flow threads are different from the columns pagination because currently there's no way to detect a line that > + // moves from one region to the other in RenderBlock::determineStartPosition. The width might change in that cases > + // and we wouldn't catch it. For columns, the only reason the width may change are new floats, but floats are cached > + // on RootInlineBox which make the structure a litle bigger. > + if (view()->layoutState()->pageLogicalOffset(logicalTop()) != pageLogicalOffset()) > + setNeedsLayout(true, false); > +} Don't get why this is needed at all. Why isn't markForPaginationRelayoutIfNeeded sufficient? > Source/WebCore/rendering/RenderBlock.cpp:3409 > + if (!childBox->selfNeedsLayout() && view()->hasRenderFlowThread()) > + childBox->markForFlowThreadRelayoutIfNeeded(); Same question here. > Source/WebCore/rendering/RenderBlock.cpp:6115 > + LayoutUnit pageLogicalHeight = pageLogicalHeightForLine(logicalOffset); "line" isn't the right term to use here. Again it's just a position or offset. pageLogicalHeightForOffset would be better. > Source/WebCore/rendering/RenderBlock.cpp:6120 > + LayoutUnit remainingLogicalHeight = pageRemainingLogicalHeightForLine(logicalOffset); Same quibble here. Just using an offset. No lines even have to be involved. > Source/WebCore/rendering/RenderBlock.cpp:6187 > +bool RenderBlock::isPaginated() const > +{ > + const LayoutState* layoutState = view()->layoutState(); > + return view()->hasRenderFlowThread() || (layoutState && layoutState->isPaginated()); > +} Why not just set isPaginated to true in the layout state when laying out the children of the RenderFlowThread? That will be faster than doing two checks every time. > Source/WebCore/rendering/RenderBlock.cpp:6193 > +bool RenderBlock::hasFixedPageHeight() const > +{ > + const LayoutState* layoutState = view()->layoutState(); > + return view()->hasRenderFlowThread() || (layoutState && layoutState->pageLogicalHeight()); > +} I kind of see why you can't reuse pageLogicalHeight() since it's variable, so I guess this is ok. I still dislike having two checks every time. > Source/WebCore/rendering/RenderBlock.cpp:6289 > + // FIXME: For regions we cannot use pagination strut, because the region width might change and the line > + // would need to be recomputed anyway. If the delta is 0 the line would not be recomputed. > + if (lineBox == firstRootBox() && totalLogicalHeight < pageLogicalHeight && !isPositioned() && !isTableCell() && !renderView->hasRenderFlowThread()) This is a problem with all line pagination and not just with regions. I'd prefer it if you take the hasRenderFlowThread() check out here and we can file a bug on improving line layout when you paginate to a position with a different available line width. You can reproduce the same problem with floats. > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:375 > + if (!child->selfNeedsLayout() && view()->hasRenderFlowThread()) > + child->markForFlowThreadRelayoutIfNeeded(); Again, confused over all these additional checks. I would think markForPaginationRelayoutIfNeeded would be enough. > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:439 > + if (!child->selfNeedsLayout() && view()->hasRenderFlowThread()) > + child->markForFlowThreadRelayoutIfNeeded(); Ditto. > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:671 > + if (!child->selfNeedsLayout() && view()->hasRenderFlowThread()) > + child->markForFlowThreadRelayoutIfNeeded(); Ditto.

I know it's not relevant to this patch, but I'd be curious to hear the logic behind not allowing floats to break across regions. We successfully split floats across columns and pages now, so I'm not sure why you would prevent them from splitting across regions.

(In reply to comment #11) > I know it's not relevant to this patch, but I'd be curious to hear the logic behind not allowing floats to break across regions. We successfully split floats across columns and pages now, so I'm not sure why you would prevent them from splitting across regions. I remember that the main point agains breakable floats is "float:right" in regions with different widths. The question was: What should happen when there are for example two regions, one with 100px and one with 200px width? If a float breaks, should it realign in the second region? However, at the time we were deciding that, we didn't thought at all at block splitting, so it might be totally different now.

Perhaps we could take the float split discussion to https://bugs.webkit.org/show_bug.cgi?id=67232, if you both agree with me that the padding and float split behavior are related. Is there a use case for splitting floats? The only time I can think of where I'd want a float split would be in a tiled print situation where the print media is too small to render the entire layout. Even in that scenario if there's a print reflow that avoids splitting I think I'd prefer that. In the use cases I have in mind it doesn't make sense to me to split boxes. Consider a float consisting of an image - a portrait. Why would you want someone's eyes displayed at the bottom of a column and their mouth displayed at the top of the next column? I'd rather have the whole image displayed in the next column.

(In reply to comment #10) > (From update of attachment 105971 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105971&action=review Thanks for the review! > > > Source/WebCore/rendering/RenderBlock.cpp:2085 > > + if (!child->selfNeedsLayout() && view()->hasRenderFlowThread()) > > + child->markForFlowThreadRelayoutIfNeeded(); > > Why selfNeedsLayout and not needsLayout? selfNeedsLayout is actually needed to force a full relayout in RenderBlock::layoutInlineChildren. I need a full relayout in cases when the logicalTop of a block changes, but nothing else is dirty. The issue that I fixed with that reproduces in regions with different widths. Take the following example: <div id="region1" style="width: 100px"></div> <div id="region2" style="width: 200px"></div> <div id="content1">First paragraph</div> <div id="content2">Second paragraph</div> Let's say that both content1 and content2 fit inside region1. When content1 changes height, it will push down the content2. Moving will force some lines to readapt, which can be caught in RenderBlock::determineStartPosition and trigger there a full relayout. However, there is the case when the whole content2 is moved to the second region. In that case nothing could be used to determine the region width change. The other way to fix it is to save the line width for every RootInlineBox, but that will make every RootInlineBox bigger. Floats should have the same issue, just that those are easily fixed because there's a vector of floats cached for each line. There's a check that triggers a full relayout if there are new/changed floats on a line. > Don't get why this is needed at all. Why isn't markForPaginationRelayoutIfNeeded sufficient? markForPaginationRelayoutIfNeeded is not sufficient because sometimes selfNeedsLayout != needsLayout. Forcing a full line relayout will need selfNeedsLayout to be true. Do you see any solution for the specific case I'm describing? It tends to reproduce a lot when blocks have margins. That's because the layout is done twice, once for without margin and once with margin applied (collapsing estimation). > > Source/WebCore/rendering/RenderBlock.cpp:6115 > > + LayoutUnit pageLogicalHeight = pageLogicalHeightForLine(logicalOffset); > > "line" isn't the right term to use here. Again it's just a position or offset. pageLogicalHeightForOffset would be better. Ok. > > Source/WebCore/rendering/RenderBlock.cpp:6187 > > +bool RenderBlock::isPaginated() const > > +{ > > + const LayoutState* layoutState = view()->layoutState(); > > + return view()->hasRenderFlowThread() || (layoutState && layoutState->isPaginated()); > > +} > > Why not just set isPaginated to true in the layout state when laying out the children of the RenderFlowThread? That will be faster than doing two checks every time. Ok. Can do that. I was initially afraid that there are cases when columns need different paths, but that doesn't seem the case now. > > Source/WebCore/rendering/RenderBlock.cpp:6193 > > +bool RenderBlock::hasFixedPageHeight() const > > +{ > > + const LayoutState* layoutState = view()->layoutState(); > > + return view()->hasRenderFlowThread() || (layoutState && layoutState->pageLogicalHeight()); > > +} > > I kind of see why you can't reuse pageLogicalHeight() since it's variable, so I guess this is ok. I still dislike having two checks every time. This is just used to save the logical offset of a block, so that a layout could be triggered if the block moves. I'm not really sure I understand the cases where pageLogicalHeight() is actually set to 0 and isPaginated returns true. Can you give a little bit of info about that? I'm not sure if I could just replace the checks with "isPaginated" and avoid using the layoutState->pageLogicalHeight() != 0 checks. > > Source/WebCore/rendering/RenderBlock.cpp:6289 > > + // FIXME: For regions we cannot use pagination strut, because the region width might change and the line > > + // would need to be recomputed anyway. If the delta is 0 the line would not be recomputed. > > + if (lineBox == firstRootBox() && totalLogicalHeight < pageLogicalHeight && !isPositioned() && !isTableCell() && !renderView->hasRenderFlowThread()) > > This is a problem with all line pagination and not just with regions. I'd prefer it if you take the hasRenderFlowThread() check out here and we can file a bug on improving line layout when you paginate to a position with a different available line width. This is similar to the first issue that made me create markForFlowThreadRelayoutIfNeeded. Should I take that out too?

(In reply to comment #13) > Perhaps we could take the float split discussion to https://bugs.webkit.org/show_bug.cgi?id=67232, if you both agree with me that the padding and float split behavior are related. I think those are two different things and should have different bugs. > In the use cases I have in mind it doesn't make sense to me to split boxes. Consider a float consisting of an image - a portrait. Why would you want someone's eyes displayed at the bottom of a column and their mouth displayed at the top of the next column? I'd rather have the whole image displayed in the next column. Images are replaced RenderObjects and should not split. If that's not the case, you can fill a bug.

My suggestion would be that you not try to solve variable region width issues in the same patch that's landing the initial slicing, since I think we have more to talk about with variable region width stuff first. I personally still like the idea of implementing variable region widths using synthetic floats. Then you wouldn't need all this regionWidthForOffset and region fitting stuff, since you could just make clearFloats virtual and then subclass it in RenderFlowThread and put in floating objects on the sides of the narrower regions. The advantage of using synthetic floats for variable region widths is that all the float layout code that is smart about vertical position movement in layoutInlineChildren would start kicking in for you, and it also turns the variable width region problem into the same problem as floats, e.g., something we already have to solve if we move a line to a new location and the line width changes. So my suggestion would be to just yank all the code in this patch related to variable region widths and retool variable region widths to use synthetic floats instead (so that line layout does the right thing without needing a full relayout. "I'm not really sure I understand the cases where pageLogicalHeight() is actually set to 0 and isPaginated returns true. Can you give a little bit of info about that?" This situation occurs when you are balancing columns, i.e., you don't yet know what the height of the columns is going to be. You have to lay them out without making pagination slices, but you are tracking certain information about the columns in order to know how to slice them better when you lay them out again. So basically it's only used by columns (and not printing or regions).

Created attachment 106328 [details] Patch V5

Attachment 106328 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:16: Line contains tab character. [whitespace/tab] [5] Total errors found: 6 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 106330 [details] Patch V5 fixed style

Comment on attachment 106330 [details] Patch V5 fixed style Attachment 106330 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9594722

Comment on attachment 106330 [details] Patch V5 fixed style r=me

Comment on attachment 106330 [details] Patch V5 fixed style View in context: https://bugs.webkit.org/attachment.cgi?id=106330&action=review This patch fails a bunch of printing tests. See LayoutTests/printing. > Source/WebCore/rendering/RenderBlock.cpp:6102 > return logicalOffset + (remainingLogicalHeight ? remainingLogicalHeight : pageLogicalHeight); if (!renderView->hasRenderFlowThread()) { 6185 LayoutUnit pageLogicalHeight = renderView->layoutState()->m_pageLogicalHeight; 6186 LayoutUnit remainingHeight = pageLogicalHeight - layoutMod(offset, pageLogicalHeight); 6187 if (includeBoundaryPoint) { 6188 // If includeBoundaryPoint is true the line exactly on the top edge of a 6189 // column will act as being part of the previous column. 6190 remainingHeight = layoutMod(remainingHeight, pageLogicalHeight); 6191 } 6192 return remainingHeight; 6193 } > Source/WebCore/rendering/RenderBlock.cpp:6197 > + LayoutUnit pageLogicalHeight = layoutState->m_pageLogicalHeight; 6119 LayoutSize delta = layoutState->m_layoutOffset - layoutState->m_pageOffset; 6120 LayoutUnit offset = isHorizontalWritingMode() ? delta.height() : delta.width(); 6121 LayoutUnit remainingLogicalHeight = layoutMod(pageLogicalHeight - layoutMod(offset + logicalOffset, pageLogicalHeight), pageLogicalHeight);

Ignore the accidental cut and paste of code. Something is causing printing tests to fail.

Created attachment 107577 [details] Patch that fixes the printing regressions.

Comment on attachment 107577 [details] Patch that fixes the printing regressions. Clearing flags on attachment: 107577 Committed r95264: <http://trac.webkit.org/changeset/95264>

All reviewed patches have been landed. Closing bug.