[CSS Regions] avoid value is not obeyed in region-break-before and region-break-after CSS properties

• Component: CSS
• Status: RESOLVED WONTFIX

[Mihai Balan]

Created attachment 175202 [details]
HTML file highlighting the problem

When specifying 'avoid' for -webkit-region-break-before or -webkit-region-break-after the breaking does not seem to take it into account.
However, 'avoid' seems implemented for -webkit-region-break-inside.

In attached file, top and bottom row don't obey the region break avoid property, but the middle one (for break inside) works as expected.

[Mihai Maerean]

Created attachment 192229 [details]
patch. wip.

How stuff works:

1. when we encounter the 1st block in the series of blocks with break-after:avoid, we store it.
2. when we encounter another block (in the normal flow) that is a sibling of the said block, if there's a break between the two blocks we move the break:avoid block to the next page by faking break-before:always on it.
3. we reset the layout loop of the parent's list of children to the break:avoid block.

Things still to be done:

1. test for chains of break:avoid blocks.
2. test for possible infinite loops when the chain of break:avoid blocks doesn't fit in the page they're moved to.

Please review.

[Mihnea Ovidenie]

Comment on attachment 192229 [details]
patch. wip.

View in context: https://bugs.webkit.org/attachment.cgi?id=192229&action=review

> LayoutTests/fast/regions/break-before-avoid.html:42
> +</html>

In general, when i create tests for which text is important, i find that specifying the font family, font size and line height make the tests more robust for all the platforms.
Like: body { font: 16px/1.25 monospace; }
I also put the paragraph describing the bug under the body, not at the end of test.

I would also like to see tests that include margins.

> Source/WebCore/rendering/RenderBlock.cpp:7410
> +bool RenderBlock::hasBreakAfterAvoid() const

This function has a similar patter for PBALWAYS in applyBeforeBreak etc. Can we change it to receive the type of break as a parameter?

> Source/WebCore/rendering/RenderBlock.h:1082
> +    void setrendererWithBreakAfterAvoid(RenderBox* r)

Change to setRenderer..., capital R instead of r.

> Source/WebCore/rendering/RenderBlock.h:1089
> +    RenderBox* rendererWhereToResetLayout() const { return m_rareData ? m_rareData->m_rendererWhereToResetLayout : 0; }

I would try to find a different name instead of reset layout.

> Source/WebCore/rendering/RenderBox.h:173
> +    // All checks are done.

What do you mean by that?

> Source/WebCore/rendering/RenderFlowThread.cpp:788
> +    return ++m_regionList.find(firstEnd) == m_regionList.find(secondStart);

Why do you need the second find here? You can probably do:
*(++m_regionList.find(firstEnd)) == secondStart.

> Source/WebCore/rendering/RenderFlowThread.h:140
> +    bool isABreakBetweenBoxes(const RenderBox*, const RenderBox*) const;

I would name this function differently, inSuccessiveRegions or smth.

[Mihai Maerean]

Comment on attachment 192229 [details]
patch. wip.

View in context: https://bugs.webkit.org/attachment.cgi?id=192229&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:7410
>> +bool RenderBlock::hasBreakAfterAvoid() const
> 
> This function has a similar patter for PBALWAYS in applyBeforeBreak etc. Can we change it to receive the type of break as a parameter?

I will think out it.

>> Source/WebCore/rendering/RenderBlock.h:1082
>> +    void setrendererWithBreakAfterAvoid(RenderBox* r)
> 
> Change to setRenderer..., capital R instead of r.

true.

>> Source/WebCore/rendering/RenderBlock.h:1089
>> +    RenderBox* rendererWhereToResetLayout() const { return m_rareData ? m_rareData->m_rendererWhereToResetLayout : 0; }
> 
> I would try to find a different name instead of reset layout.

I'm open to suggestions.

>> Source/WebCore/rendering/RenderBox.h:173
>> +    // All checks are done.
> 
> What do you mean by that?

The above functions have this comment attached:
// Use this with caution! No type checking is done!

>> Source/WebCore/rendering/RenderFlowThread.cpp:788
>> +    return ++m_regionList.find(firstEnd) == m_regionList.find(secondStart);
> 
> Why do you need the second find here? You can probably do:
> *(++m_regionList.find(firstEnd)) == secondStart.

me like it :)

>> Source/WebCore/rendering/RenderFlowThread.h:140
>> +    bool isABreakBetweenBoxes(const RenderBox*, const RenderBox*) const;
> 
> I would name this function differently, inSuccessiveRegions or smth.

The current name is exactly what I need to determine (if there is a break that conflicts with break:avoid) which makes the code easier to read.

[Dave Hyatt]

You can't just rewind block layout like that. That approach does not work. The problem of avoidance is also a lot more complicated than this, since you may be a nested block that happens to be the last child, and so a a break can occur after you even when you have no following siblings.

In general, I think knowledge that you are at a breakpoint that needs to be avoided is probably going to need to be in the layout state rather than trying to hack it into the rare data.

I'm also not convinced that rewinding layout is actually necessary to make this work, but I could be wrong about that.

[Dave Hyatt]

Actually I take it back. You probably will need bits in the rare data. This problem is pretty similar to margin propagation and pagination strut propagation. You'll probably just need to cache and propagate bits indicating the avoidance state before and after the block.

[Mihai Maerean]

Having in mind that break-[after|before]:avoid properties are propagated to the parents when necessary as David Hyatt said in the previous comment,
this is the proposed algorithm that results in the avoid condition being fulfilled when a break is encountered between 2 consecutive blocks in the normal flow where a break should be avoided:

Starting with the block that needs to avoid a break at the after side where a break did occur,
1. If it's unsplittable (has break-inside:avoid etc)
     1.1 then if there is a break-avoid rule at the before side
         1.1.1 then we move the cursor of the algorithm to its previous sibling block in normal flow
         1.1.2 else we set a fake break-before:always on it and stop the search.
     1.2 else (meaning it can be split)
         1.2.1 If it doesn't have any children
                 1.2.1.1 then we set a fake break-before:always on it and stop the search.
         1.2.2 If it only has inline content
                 1.2.2.1 then we break this block based on the widows and orphans algorithm and stop
                         the search.
         1.2.3 starting from the last child block in normal flow, we recursively look for another
               block that meets the same conditions.
2. If we have found a break location
     2.1 then we mark all the objects that have been touched as NeedsLayout as all of them will be
         affected by the break that was just inserted.
         Relayout the parent that is closest to all the touched nodes.

[Dave Hyatt]

Note that the pagination strut is the mechanism we are using right now to effectively propagate page-break-before:always out to parents, since we also do this for unforced breaks.

When considering a breakpoint that is between two block-level boxes, you have to look at the margins of all boxes that collapsed at that point. If any one of them is set to "always" on the correct side (either before/after depending on the box) then "always" wins. If any one of them is set to "avoid", then "avoid" wins, and you can't break here. If nothing is set explicitly on any before/after, then the inherited value of "inside" from an ancestor container wins.

So with those rules in mind, we need to:
(a) Be able to easily know what the breaking state is where the margins meet. This is very similar to propagation of margins themselves (and a little similar to pagination struts).

(b) Breaks should only apply between siblings, so for the first and last children, we should just propagate the values out to the parent block. This involves changing how the "always" value works.

(c) We need to easily know what the avoidance state is from an ancestor (whether or not we're inside an ancestor that set page-break-inside:avoid).

(d) Track the last viable breakpoint that is still on the same page. Essentially while laying out the block-level children, you want to know where the last viable encountered breakpoint is. This may be inside a descendant.

(e) When you determine that a break has to be avoided but that you also don't fit on the page, you need to then see if you have a viable breakpoint that also exists on the page. If it does, then you have to lay the block out again with a note to apply a break at the viable breakpoint. Again, this could be inside a descendant and is not necessarily in between your own immediate childen.

So at a high level, the idea of "rewinding" and then applying a "forced break" is a good one. However rewinding needs to just lay out the whole block again. You can't just rewind. Also, you can't just go back to a child's previous sibling because the place to apply the break may be inside a descendant box.

You also need to know if that breakpoint is on the same page. If it's on a previous page, then no viable breakpoint exists on the current page, and in that case you have to just overflow the current page (either that or we do a bad slice).

[Brent Fulgham]

CSS Regions were removed in Bug 174978.