[CSSRegions] Regions should not slice line box rendering

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alan Stearns]

Created attachment 103869 [details]
Test file

If a line box does not entirely fit in a region's display area, the part that does not fit gets rendered in the next region. Regions should only display a line box if it fits entirely - partial line boxes should be pushed to the next region.

[Alexandru Chiculita]

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

[Alan Stearns]

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?

[Alexandru Chiculita]

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.

[Alan Stearns]

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.

[Alexandru Chiculita]

(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.

[Alexandru Chiculita]

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.

[WebKit Review Bot]

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

[Alexandru Chiculita]

Created attachment 105971 [details]
Patch V4

Fixed the layout tests.

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Alexandru Chiculita]

(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.

[Alan Stearns]

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.

[Alexandru Chiculita]

(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?

[Alexandru Chiculita]

(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.

[Dave Hyatt]

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).

[Alexandru Chiculita]

Created attachment 106328 [details]
Patch V5

[WebKit Review Bot]

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.

[Alexandru Chiculita]

Created attachment 106330 [details]
Patch V5 fixed style

[WebKit Review Bot]

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

[Dave Hyatt]

Comment on attachment 106330 [details]
Patch V5 fixed style

r=me

[Dave Hyatt]

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);

[Dave Hyatt]

Ignore the accidental cut and paste of code.

Something is causing printing tests to fail.

[Dave Hyatt]

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

[WebKit Review Bot]

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

Clearing flags on attachment: 107577

Committed r95264: <http://trac.webkit.org/changeset/95264>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.