Respect break-inside:avoid on table rows

Respect break-inside:avoid on table rows

When break-inside:avoid is set on a table row then push the row down to the
pagination boundary if it would otherwise straddle it.

BUG=99124
Committed: https://crrev.com/6827228e3e093e665e2460503c258730f570e477
Cr-Commit-Position: refs/heads/master@{#392404}

[rhogan, 2016-02-02 21:22:25]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-02-03 14:38:48]

What's the deal with "inline content" in particular (your description)?

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/multicol/cell-shrinkback-expected.html
(right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/multicol/cell-shrinkback-expected.html:3:
body { font: 16px Ahem;} /* Disable auto-scrollbars, to hide crbug.com/534751 */
I'm pretty sure you didn't mean to copy-paste that comment. :)

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/multicol/table-row-height-increase.html
(left):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/multicol/table-row-height-increase.html:12:
<table cellpadding=0 cellspacing=0 style="background-color: red;">
Looks like the table cells with text are pushed to the next column now. But why?
That doesn't seem right.

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/printing/cells-dont-fit-on-page-paginated.html
(right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/cells-dont-fit-on-page-paginated.html:8:
font-size: 165px;
This is fine. There's no good way of specifying the page height for printing
tests that I know of, so just making assumptions about the page height, like you
do here, is probably the best we can do.

But, instead of adding printing tests, you could write multicol tests. Then you
have full control over fragmentainer heights and what not. :)

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/printing/single-line-cells-paginated.html
(right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/single-line-cells-paginated.html:43:
</table>
Doesn't terminate anything.

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:740: if
(block.isTableCell() && totalLogicalHeight <= pageLogicalHeight)
It should be better to have allowsPaginationStrut() allow table cells than
performing the check here. That's going to cause inconsistencies.

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:132: void
setPaginationStrutPropagatedFromCell(LayoutUnit strut) {
m_paginationStrutPropagatedFromCell = strut; };
What if the cells don't all have the same strut?

Also: no need for the trailing semi-colon. And I think we generally like to have
a blank line before "private:".

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:170: LayoutUnit
m_paginationStrutPropagatedFromCell;
Not sure if LayoutUnit should be used in table code. It's mostly about ints
there.

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1064: if
(rowLayoutObject && rowLayoutObject->paginationStrutPropagatedFromCell()) {
I haven't reviewed this yet.

[mstensho (USE GERRIT), 2016-02-11 12:08:07]

Description was changed from

==========
Don't let table rows with inline content straddle page boundaries

BUG=99124
==========

to

==========
Don't let table rows with inline content straddle page boundaries

BUG=99124
==========

[rhogan, 2016-02-24 18:58:03]

On 2016/02/03 at 14:38:48, mstensho wrote:
> What's the deal with "inline content" in particular (your description)?

I'm clumsily trying to say that instead of letting a sentence or some inline
content in a cell  (and hence the cell and row iteslf) straddle a page, always
push the row down to the next page border. 

>
https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/multicol/table-row-height-increase.html:12:
<table cellpadding=0 cellspacing=0 style="background-color: red;">
> Looks like the table cells with text are pushed to the next column now. But
why? That doesn't seem right.

Yes, this is my intention.

> 
> But, instead of adding printing tests, you could write multicol tests. Then
you have full control over fragmentainer heights and what not. :)

We can raincheck this until I know you're happy with the rest of the CL.

[rhogan, 2016-02-24 18:58:17]

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right):

https://codereview.chromium.org/1602773005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:132: void
setPaginationStrutPropagatedFromCell(LayoutUnit strut) {
m_paginationStrutPropagatedFromCell = strut; };
On 2016/02/03 at 14:38:48, mstensho wrote:
> What if the cells don't all have the same strut?

I can't come up with a test in which they don't - which is how I would expect a
row to work - always expanding cells to the height of the tallest one. I've
added an ASSERT.

[mstensho (USE GERRIT), 2016-02-26 13:29:59]

It seems hard to break nicely inside table cells at all now, and the first piece
of content is over-eagerly pushed to the next column, even if it would fit in
the first one.

<!DOCTYPE html>
<div style="-webkit-columns:3; column-fill:auto; line-height:20px; height:90px;
background:yellow;">
    <br>
    <table>
        <tr>
            <td>
                First column<br>
                First column<br>
                First column<br>
                Second column<br>
                Second column<br>
                Second column<br>
                Second column<br>
                Third column<br>
            </td>
        </tr>
    </table>
</div>

https://codereview.chromium.org/1602773005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1602773005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:44: #include
"core/layout/LayoutTableRow.h"
No need for this, I hope.

https://codereview.chromium.org/1602773005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:726: LayoutUnit
lineHeight = lineBox.lineBottomWithLeading() - lineBox.lineTopWithLeading();
Maybe you meant to revert these changes, now that we return true in
allowsPaginationStrut() for table cells.

[rhogan, 2016-02-27 11:20:08]

On 2016/02/26 at 13:29:59, mstensho wrote:
> It seems hard to break nicely inside table cells at all now, and the first
piece of content is over-eagerly pushed to the next column, even if it would fit
in the first one.
> 
> <!DOCTYPE html>
> <div style="-webkit-columns:3; column-fill:auto; line-height:20px;
height:90px; background:yellow;">
>     <br>
>     <table>
>         <tr>
>             <td>
>                 First column<br>
>                 First column<br>
>                 First column<br>
>                 Second column<br>
>                 Second column<br>
>                 Second column<br>
>                 Second column<br>
>                 Third column<br>
>             </td>
>         </tr>
>     </table>
> </div>
> 

Maybe I should confine it to document.isPrinting()? That's what it's for after
all. I'm not interested in creating the effect in multi-col if it's not a good
fit for the multi-col use cases.

[rhogan, 2016-02-27 11:23:41]

On 2016/02/27 at 11:20:08, rhogan wrote:
> On 2016/02/26 at 13:29:59, mstensho wrote:
> > It seems hard to break nicely inside table cells at all now, and the first
piece of content is over-eagerly pushed to the next column, even if it would fit
in the first one.
> > 
> > <!DOCTYPE html>
> > <div style="-webkit-columns:3; column-fill:auto; line-height:20px;
height:90px; background:yellow;">
> >     <br>
> >     <table>
> >         <tr>
> >             <td>
> >                 First column<br>
> >                 First column<br>
> >                 First column<br>
> >                 Second column<br>
> >                 Second column<br>
> >                 Second column<br>
> >                 Second column<br>
> >                 Third column<br>
> >             </td>
> >         </tr>
> >     </table>
> > </div>
> > 
> 
> Maybe I should confine it to document.isPrinting()? That's what it's for after
all. I'm not interested in creating the effect in multi-col if it's not a good
fit for the multi-col use cases.

Actually if that happens when printing too then I need to fix it.

[mstensho (USE GERRIT), 2016-02-29 09:46:47]

https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:728: if
(block.isTableCell() && toLayoutTableCell(block).table()->vBorderSpacing() +
block.logicalHeight() + remainingLogicalHeight < pageLogicalHeight) {
I'm sorry, but this just looks like a hack. While it fixes the test case I
showed you, how about this:

<!DOCTYPE html>
<div style="-webkit-columns:3; column-fill:auto; line-height:20px;
height:90px;">
    <br>
    <br>
    <br>
    <table>
        <tr>
            <td>
                First column<br>
                Second column<br>
            </td>
        </tr>
    </table>
</div>

Also note that I don't think we want to behave differently in multicol and
printing here. Fragmentation is fragmentation. :)

https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1647:
layoutState.markForFullLayout();
I really hope that we can avoid this.

[rhogan, 2016-02-29 19:28:07]

On 2016/02/29 at 09:46:47, mstensho wrote:
> I'm sorry, but this just looks like a hack. While it fixes the test case I
showed you, how about this:
> 
> <!DOCTYPE html>
> <div style="-webkit-columns:3; column-fill:auto; line-height:20px;
height:90px;">
>     <br>
>     <br>
>     <br>
>     <table>
>         <tr>
>             <td>
>                 First column<br>
>                 Second column<br>
>             </td>
>         </tr>
>     </table>
> </div>
> 

That's the behaviour I'm introducing - since the cell can't all fit in the first
column, we move it to the top of second one. This is different from your other
case because there the entire cell was taller than any available page.


> Also note that I don't think we want to behave differently in multicol and
printing here. Fragmentation is fragmentation. :)
Agreed.

>
https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1647:
layoutState.markForFullLayout();
> I really hope that we can avoid this.
I don't think we can - I need it in order to ensure that I consider the cell as
a whole when deciding whether I need to push it to the next page.

[mstensho (USE GERRIT), 2016-02-29 21:01:31]

On 2016/02/29 19:28:07, rhogan wrote:
> On 2016/02/29 at 09:46:47, mstensho wrote:
> > I'm sorry, but this just looks like a hack. While it fixes the test case I
> showed you, how about this:
> > 
> > <!DOCTYPE html>
> > <div style="-webkit-columns:3; column-fill:auto; line-height:20px;
> height:90px;">
> >     <br>
> >     <br>
> >     <br>
> >     <table>
> >         <tr>
> >             <td>
> >                 First column<br>
> >                 Second column<br>
> >             </td>
> >         </tr>
> >     </table>
> > </div>
> > 
> 
> That's the behaviour I'm introducing - since the cell can't all fit in the
first
> column, we move it to the top of second one. This is different from your other
> case because there the entire cell was taller than any available page.

It should only behave like that if break-inside is avoid, avoid-page or
avoid-column. It seems that you want table-rows to behave like that by default?

> > Also note that I don't think we want to behave differently in multicol and
> printing here. Fragmentation is fragmentation. :)
> Agreed.
> 
> >
>
https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1647:
> layoutState.markForFullLayout();
> > I really hope that we can avoid this.
> I don't think we can - I need it in order to ensure that I consider the cell
as
> a whole when deciding whether I need to push it to the next page.

Maybe we need to get back to this later. Normally, it's the
markForPaginationRelayoutIfNeeded() that takes care of deep layout passes for
pagination.

[rhogan, 2016-03-03 21:33:40]

On 2016/02/29 at 21:01:31, mstensho wrote:
> On 2016/02/29 19:28:07, rhogan wrote:
> > On 2016/02/29 at 09:46:47, mstensho wrote:
> > > I'm sorry, but this just looks like a hack. While it fixes the test case I
> > showed you, how about this:
> > > 
> > > <!DOCTYPE html>
> > > <div style="-webkit-columns:3; column-fill:auto; line-height:20px;
> > height:90px;">
> > >     <br>
> > >     <br>
> > >     <br>
> > >     <table>
> > >         <tr>
> > >             <td>
> > >                 First column<br>
> > >                 Second column<br>
> > >             </td>
> > >         </tr>
> > >     </table>
> > > </div>
> > > 
> > 
> > That's the behaviour I'm introducing - since the cell can't all fit in the
first
> > column, we move it to the top of second one. This is different from your
other
> > case because there the entire cell was taller than any available page.
> 
> It should only behave like that if break-inside is avoid, avoid-page or
avoid-column. It seems that you want table-rows to behave like that by default?

For the printing case - yes. For multicol more generally - sounds like I should
respect break-inside.

> 
> > > Also note that I don't think we want to behave differently in multicol and
> > printing here. Fragmentation is fragmentation. :)
> > Agreed.
> > 
> > >
> >
https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1647:
> > layoutState.markForFullLayout();
> > > I really hope that we can avoid this.
> > I don't think we can - I need it in order to ensure that I consider the cell
as
> > a whole when deciding whether I need to push it to the next page.
> 
> Maybe we need to get back to this later. Normally, it's the
markForPaginationRelayoutIfNeeded() that takes care of deep layout passes for
pagination.

OK.

Thoughts on next steps for this CL?

[mstensho (USE GERRIT), 2016-03-03 21:42:33]

On 2016/03/03 21:33:40, rhogan wrote:
> On 2016/02/29 at 21:01:31, mstensho wrote:
> > It should only behave like that if break-inside is avoid, avoid-page or
> avoid-column. It seems that you want table-rows to behave like that by
default?
> 
> For the printing case - yes. For multicol more generally - sounds like I
should
> respect break-inside.

It's wrong to behave like that by default, regardless of fragmentation type.
It's fine to break inside table cells and rows.

> > > > Also note that I don't think we want to behave differently in multicol
and
> > > printing here. Fragmentation is fragmentation. :)
> > > Agreed.
> > > 
> > > >
> > >
>
https://codereview.chromium.org/1602773005/diff/140001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1647:
> > > layoutState.markForFullLayout();
> > > > I really hope that we can avoid this.
> > > I don't think we can - I need it in order to ensure that I consider the
cell
> as
> > > a whole when deciding whether I need to push it to the next page.
> > 
> > Maybe we need to get back to this later. Normally, it's the
> markForPaginationRelayoutIfNeeded() that takes care of deep layout passes for
> pagination.
> 
> OK.
> 
> Thoughts on next steps for this CL?

First of all, I think it needs to be conditioned on "break-inside" being
different from "auto" (initial value). Non-default break-inside (i.e. something
with "avoid" in it) is currently only supported for regular block children in
Blink; see LayoutBlockFlow::adjustForUnsplittableChild(). I suppose we should
add something similar in LayoutTableSection or LayoutTableRow.

[rhogan, 2016-03-14 21:04:57]

On 2016/03/03 at 21:42:33, mstensho wrote:
> 
> First of all, I think it needs to be conditioned on "break-inside" being
different from "auto" (initial value). Non-default break-inside (i.e. something
with "avoid" in it) is currently only supported for regular block children in
Blink; see LayoutBlockFlow::adjustForUnsplittableChild(). I suppose we should
add something similar in LayoutTableSection or LayoutTableRow.

OK - applied your comments from this and previous rounds. Ready for another
look!

[mstensho (USE GERRIT), 2016-03-17 14:04:04]

A bunch of comments, but I like the direction this CL is taking! :)

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/cell-taller-than-col-straddles-columns.html
(right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/cell-taller-than-col-straddles-columns.html:7:
<td data-total-y=2>
It's going to be hard to get this one to fail, since we don't propagate the
strut set on the line up to the table cell or row. We SHOULD probably do that,
but that's definitely not for this CL.

How about you wrap everything in the cell inside a DIV and put your data-total-y
expectation on that one instead? Then you'll be able to catch unwanted struts.

Oh, and did you forget break-inside:avoid here?

Fun fact: The only valid class A break point [1] before the first line in this
cell, is after the anonymous block with the BR and before the table. We should
never break before a first child, unless there's a class C break point there.
But let's not worry about that for now. I'm actually working on this at the
moment, but it will definitely take some time before it gets implemented inside
tables.

[1] https://drafts.csswg.org/css-break/#possible-breaks

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html (right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html:9: font-size:
20px;
Better use forced line breaks inside the cells, rather than making assumptions
about glyph widths.

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html:12:
page-break-inside: avoid;
Can you say break-inside:avoid (break-inside:avoid-column works too, but no need
to be that specific) instead? We're not paged, only columnized. It's correct
according to the spec that page-break-inside:avoid work inside multicol [1], but
it's pretty weird, right? :)

[1] https://drafts.csswg.org/css-break/#page-break-properties

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/single-cell-too-large-for-page.html
(right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/single-cell-too-large-for-page.html:12:
page-break-inside: avoid;
break-inside:avoid

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/single-cell-too-large-for-page.html:16:
<div style="-webkit-columns:3; line-height: 60px; height:210px;
background-color: yellow;">
You probably don't want the columns balanced, so you should add column-fill:auto

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/single-cell-too-large-for-page.html:18:
<tr><td class="td" data-total-y=1>Text Text Text Text Text Text Text
Text</td><td>Text Text Text Text Text Text Text Text</td></tr>
Will "never" fail, no matter how broken we are (as mentioned in an earlier
comment). Wrap everything inside a DIV and put the expectation on the DIV? Also,
please use explicit line breaks, rather than depending on the width of the text
with whatever font we end up with.

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:914: bool
insideBreakableObject = !isTableCell() ||
toLayoutBox(parent())->getPaginationBreakability() == AllowAnyBreaks;
This is wrong. break-inside:avoid means that you should *attempt* to avoid
breaks inside, but if you have no other options, then you have to break. We
already break normally here for regular blocks with break-inside:avoid, and
table cells should be no different. This now fails:

<!DOCTYPE html>
<div style="width:600px;">
    <div style="columns:3; column-fill:auto; height:90px; line-height:20px;">
        <div style="display:table-row; break-inside:avoid;">
            first column<br>
            first column<br>
            first column<br>
            first column<br>
            second column<br>
            second column<br>
        </div>
    </div>
</div>

(the outer DIV is to prevent Blink from laying out twice, which would hide the
bug (Blink lays out twice because it initially assumes that there'll be a
vertical scrollbar))

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:589: // Specify which page or
column to associate with an offset, if said offset is exactly at a page
YES! This belongs in LayoutBox, and finally we have a use case for it. :)

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:978: for (unsigned
rowIndex = r; paginationStrut && rowIndex <= totalRows; rowIndex++)
Kind of sad that we loop through the remainder of the table section here, since
we're eventually going to do that anyway with the outer loop. Could we somehow
accumulate the total strut instead and apply them to each row as we go through
them?

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1060: if
(!paginationStrut && view()->layoutState()->isPaginated() &&
cell->logicalHeight() != rHeight) {
Why isPaginated()? I think pageLogicalHeight() was the right check. You want to
skip all this if page height is 0 (which typically means that we're in the
initial balancing pass of multicol, and we don't want to insert struts or breaks
in that layout pass, since we have absolutely no idea how tall the columns are
going to be).

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1104: int
LayoutTableSection::adjustForUnsplittableRow(LayoutTableRow* row, LayoutUnit
logicalOffset) const
This looks like a LayoutBlockFlow::adjustForUnsplittableChild() for table
sections. But it returns the strut, not the (potentially new) offset. I find
this confusing. I suggest that you either change it to return the offset, or
rename the method (and keep returning the strut). Judging from the usage,
renaming it seems like the best choice.

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1113: if
(!pageLogicalHeight)
Might want to move this check before the rowLogicalHeight > pageLogicalHeight
one. Not that it matters with the current code, but comparing something with a
zero page height is meaningless and confusing.

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1602773005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: int
adjustForUnsplittableRow(LayoutTableRow*, LayoutUnit logicalOffset) const;
Please add a blank line after this.

[rhogan, 2016-03-23 21:32:26]

On 2016/03/17 at 14:04:04, mstensho wrote:
> A bunch of comments, but I like the direction this CL is taking! :)

Addressed all your comments except the one suggesting to accumulate the strut
when laying out rows - couldn't get it to work because you do need to have
m_rowPos in a consistent state when doing the rest of the work in the loop.

I'd like to re-add the 'printing' tests I had in the original CL and make them
png tests if possible. It's very easy to miss something otherwise - pngs are
dirty but super-effective at catching stuff you wouldn't always think to look
for in a check-layout test or similar.

[mstensho (USE GERRIT), 2016-03-29 09:57:46]

I don't see the need for any printing/ tests, since your changes are purely
about fragmentation, and it doesn't really matter whether you use printing or
multicol to test it. I agree that check-layout tests may miss regressions, but
then we could just add a reftest or seven, which should be just as good as a PNG
test in this sense.

But maybe you want to move the tests from fast/multicol/ to fragmentation/ ,
since you're not changing anything multicol specific here?

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:44: #include
"core/layout/LayoutTableCell.h"
No need for this.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1648: if
(isTableCell() && toLayoutBox(parent())->getPaginationBreakability() !=
AllowAnyBreaks) {
This piece of code isn't covered by any test, it seems.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1006: } // namespace
blink
Please re-add the blank line before this.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1008: // If
break-inside:avoid/forbid is set on a cell we should avoid setting a strut on
lines unless the cell is taller than the page,
"forbid" is not a valid value.
Also, you're checking if its set on a row, not on a cell.
But I really don't understand why we want this at all. This method is called
when we have already decided that we need to break (either before a block or
before a line), and someone might be upset by the fact that we don't respect it.
Furthermore, no tests fail if I remove this piece of code.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1014: return
LayoutBox::calculatePaginationStrutToFitContent(offset, strutToNextPage,
contentLogicalHeight);
Please use LayoutBlockFlow:: (super class)

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:976: if
(view()->layoutState()->isPaginated()) {
Might want to cache this on the outside of the loop.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: int
getPaginationStrutForRow(LayoutTableRow*, LayoutUnit logicalOffset) const;
s/getP/p/ ?

[rhogan, 2016-03-29 19:04:37]

Thanks mstensho - can you take a look at my comments before I do a new CL? It
would be great if I could just add the PNG tests, this is a tough CL and doing
ref-test results for tables is a finicky dispiriting business.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1648: if
(isTableCell() && toLayoutBox(parent())->getPaginationBreakability() !=
AllowAnyBreaks) {
On 2016/03/29 at 09:57:46, mstensho wrote:
> This piece of code isn't covered by any test, it seems.

No - but it is covered by one of my printing tests, which is why I want to add
it. I think I have the energy now to rewrite or come up with new ones now - it
was too dispiriting a prospect last time around!

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1008: // If
break-inside:avoid/forbid is set on a cell we should avoid setting a strut on
lines unless the cell is taller than the page,
On 2016/03/29 at 09:57:46, mstensho wrote:
> "forbid" is not a valid value.
Reflecting my loose-grasp of the break-inside value. :/

> Also, you're checking if its set on a row, not on a cell.
Yes.

> But I really don't understand why we want this at all. This method is called
when we have already decided that we need to break (either before a block or
before a line), and someone might be upset by the fact that we don't respect it.

We're still going to break before the block or line - if break-inside:avoid is
set we'll break before the cell/row, if not then we'll break before the line.
The one exception is where the row/cell is taller than the page, in that
situation we can only break before the line because there's nowhere sensible to
move the row/cell to.

So I don't think this function is overturning anything - it's just deciding how
best to implement the required break on the cell.



> Furthermore, no tests fail if I remove this piece of code.
My printing/ tests catch this.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: int
getPaginationStrutForRow(LayoutTableRow*, LayoutUnit logicalOffset) const;
On 2016/03/29 at 09:57:46, mstensho wrote:
> s/getP/p/ ?

Is that the new style? I still see getPaginationBreakability everywhere else.

[rhogan, 2016-03-29 19:15:13]

On 2016/03/29 at 19:04:37, rhogan wrote:
> Thanks mstensho - can you take a look at my comments before I do a new CL? It
would be great if I could just add the PNG tests, this is a tough CL and doing
ref-test results for tables is a finicky dispiriting business.
> 
> No - but it is covered by one of my printing tests, which is why I want to add
it. I think I have the energy now to rewrite or come up with new ones now - it
was too dispiriting a prospect last time around!

Way to contradict myself! :) If you have any inspiration/example that can
unblock me on doing some up that would be great, really struggling to attack
them again.

[mstensho (USE GERRIT), 2016-03-31 09:55:04]

I still insist that reftests are better than PNG tests, and that they way are
easier to write with multicol rather than printing.

Basically, you just have to establish a multicol container with a fixed height,
and disable column balancing (column-fill:auto), because all you care about is
generic fragmentation, and column balancing will just be noise.

<div style="columns:3; column-fill:auto; height:20em;">

Additionally, to make it easier to write a ref, you may want to set column-gap
(to 0, for instance).

LayoutTests/printing/cells-dont-fit-on-page-paginated.html could be written like
this:

<!DOCTYPE html>
<style>
table { border-collapse: collapse; }
tr { break-inside: avoid; }
td { border: 1px solid black; background-color: #ddd; }
</style>
<p>crbug.com/99124: Table rows shouldn't straddle page boundaries.</p>
<div style="columns:2; column-fill:auto; column-gap:0; width:40em; height:4em;">
    <table>
        <tr><td>Text Text Text<br> Text Text</td><td>Text Text Text<br> Text
Text</td></tr>
        <tr><td>Text Text Text<br> Text Text</td><td>Text Text Text<br> Text
Text</td></tr>
    </table>
</div>

And the ref:

<!DOCTYPE html>
<style>
table { border-collapse: collapse; }
td { border: 1px solid black; background-color: #ddd; }
</style>
<p>crbug.com/99124: Table rows shouldn't straddle page boundaries.</p>
<div style="width:40em; height:4em;">
    <div style="float:left; width:20em;">
        <table>
            <tr><td>Text Text Text<br> Text Text</td><td>Text Text Text<br> Text
Text</td></tr>
        </table>
    </div>
    <div style="float:left; width:20em;">
        <table>
            <tr><td>Text Text Text<br> Text Text</td><td>Text Text Text<br> Text
Text</td></tr>
        </table>
    </div>
</div>

Making it easier for a human to decide if it's rendered correctly or not,
though, I think I prefer something like this:

<!DOCTYPE html>
<style>
table { border-spacing:0; line-height:2em; }
tr { break-inside:avoid; }
td { padding:0; background:red; }
td > div { width:1em; background:green; }
</style>
<p>There should be a green square and no red below.</p>
<div style="columns:2; column-fill:auto; column-gap:0; width:4em;
height:5.9em;">
    <table>
        <tr>
            <td><div><br><br></div></td>
            <td><div><br><br></div></td>
        </tr>
        <tr>
            <td><div><br><br></div></td>
            <td><div><br><br></div></td>
        </tr>
    </table>
</div>

With this super-simple ref:

<!DOCTYPE html>
<p>There should be a green square and no red below.</p>
<div style="width:4em; height:4em; background:green;"></div>

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1008: // If
break-inside:avoid/forbid is set on a cell we should avoid setting a strut on
lines unless the cell is taller than the page,
On 2016/03/29 19:04:36, rhogan wrote:
> On 2016/03/29 at 09:57:46, mstensho wrote:
> > "forbid" is not a valid value.
> Reflecting my loose-grasp of the break-inside value. :/
> 
> > Also, you're checking if its set on a row, not on a cell.
> Yes.
> 
> > But I really don't understand why we want this at all. This method is called
> when we have already decided that we need to break (either before a block or
> before a line), and someone might be upset by the fact that we don't respect
it.
> 
> We're still going to break before the block or line - if break-inside:avoid is
> set we'll break before the cell/row, if not then we'll break before the line.
> The one exception is where the row/cell is taller than the page, in that
> situation we can only break before the line because there's nowhere sensible
to
> move the row/cell to.
> 
> So I don't think this function is overturning anything - it's just deciding
how
> best to implement the required break on the cell.
>
> > Furthermore, no tests fail if I remove this piece of code.
> My printing/ tests catch this.
> 

OK, which test? Should be fairly easy to convert it to multicol.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: int
getPaginationStrutForRow(LayoutTableRow*, LayoutUnit logicalOffset) const;
On 2016/03/29 19:04:37, rhogan wrote:
> On 2016/03/29 at 09:57:46, mstensho wrote:
> > s/getP/p/ ?
> 
> Is that the new style? I still see getPaginationBreakability everywhere else.

I don't think the intention is to prefix all getters with "get". There's just
some rule in the Google / Chromium style guide that says that names that only
differ in uppercase vs. lowercase or with or without underscore is verboten. So
it cannot be PaginationBreakability paginationBreakability(), because
strcasecmp("PaginationBreakability", "paginationBreakability") == 0.

[rhogan, 2016-04-03 14:56:41]

Addressed all your comments I think, and added new reftests.

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1648: if
(isTableCell() && toLayoutBox(parent())->getPaginationBreakability() !=
AllowAnyBreaks) {
On 2016/03/29 at 19:04:36, rhogan wrote:
> On 2016/03/29 at 09:57:46, mstensho wrote:
> > This piece of code isn't covered by any test, it seems.
> 
> No - but it is covered by one of my printing tests, which is why I want to add
it. I think I have the energy now to rewrite or come up with new ones now - it
was too dispiriting a prospect last time around!

Now covered by fast/multicol/cell-doesnt-fit-on-page-has-text.html

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/1602773005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1008: // If
break-inside:avoid/forbid is set on a cell we should avoid setting a strut on
lines unless the cell is taller than the page,
On 2016/03/29 at 09:57:46, mstensho wrote:
> "forbid" is not a valid value.
> Also, you're checking if its set on a row, not on a cell.
> But I really don't understand why we want this at all. This method is called
when we have already decided that we need to break (either before a block or
before a line), and someone might be upset by the fact that we don't respect it.
Furthermore, no tests fail if I remove this piece of code.

Now covered by:
LayoutTests/fast/multicol/single-line-cells-paginated-with-text.html

[mstensho (USE GERRIT), 2016-04-04 21:05:39]

Thanks for the new tests! Pretty much full coverage now. :)

How about that move from fast/multicol/ to fragmentation/ , which I suggested in
a previous comment? This is pure fragmentation work. It just so happens that
multicol is a good way of testing it.

You should also update the description. Mention something about support for
break-inside:avoid on table rows, perhaps.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/cell-doesnt-fit-on-page-has-text.html
(right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/cell-doesnt-fit-on-page-has-text.html:12:
page-break-inside: avoid;
break-inside

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/cell-doesnt-fit-on-page-has-text.html:15:
<p>crbug.com/99124: A break-inside:avoid row doesn't fit on page, so it breaks
and the text inside it moves to avoid straddling the page-break.</p>
A pass condition would be nice.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/cell-doesnt-fit-on-page-has-text.html:16:
<div style="-webkit-columns:3; column-fill:auto; height:210px;
background:yellow;">
You can use unprefixed column properties now.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/cell-doesnt-fit-on-page-has-text.html:17:
<table>
Indentation.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/cell-taller-than-col-straddles-columns.html
(right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/cell-taller-than-col-straddles-columns.html:7:
<td>
Double indentation.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html (right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html:14:
<p>crbug.com/99124: Table rows shouldn't straddle page boundaries.</p>
A pass condition would be nice.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html:16: <table>
Missing indentation.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/multi-line-cells.html:17:
<tr><td>Text Text Text <br> Text Text</td><td>Text Text Text <br> Text
Text</td></tr>
Might consider using one single word per line, to make the test more window
width tolerant.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/single-line-cells-paginated-with-text.html
(right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/single-line-cells-paginated-with-text.html:11:
<div style="columns:3; column-gap: 0; width:30em; height:12em;">
You probably want to specify line-height here (20px?), so that it passes on all
platforms.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/multicol/single-line-cells.html
(right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/single-line-cells.html:14:
<p>crbug.com/99124: Table rows shouldn't straddle page boundaries.</p>
A pass condition would be nice.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1646: // If a
cell's row wants to avoid straddling page breaks then we always need to layout
all
Pagination may indeed call for deeper layout passes, but this doesn't look like
the right place to do it. This code is only run when children are inline. What
about block children?

I think this is something that markForPaginationRelayoutIfNeeded() should be in
charge of. I was kind of hoping that it already handled this correctly, but then
there's this change right here, which kind of suggests otherwise. :)

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1008: if
(toLayoutBox(parent())->getPaginationBreakability() != AllowAnyBreaks &&
table()->vBorderSpacing() + logicalHeight() + contentLogicalHeight <
pageLogicalHeight)
I still think this looks strange. We have decided that we need to break here
(e.g. between two lines inside a table cell), but we just nuke the strut, and
brutally slice content across column boundaries. This looks like a hack to me. I
wonder if the problem is in LayoutTableSection::layoutRows(). I prefer if we
just leave the strut as it is here, and instead adjust the row heights correctly
in LayoutTableSection::layoutRows(). When paginationStrutForRow() gives us
something there, we should lay out all affected cells over again, and pay
attention if their heights change. Cell heights will typically shrink, since
there'll typically be fewer pagination struts inside. This height decrease needs
to be propagated to all consecutive rows.

This code here also assumes that logicalHeight() is flush with the bottom for
the piece of content that we're supposed to break after, and it doesn't seem
like that's always the case. Which seems to be the reason why this fails:

<div style="columns:2; column-fill:auto; height:5em; line-height:2em;">
    first column<br>
    <table cellpadding="1" cellspacing="0">
        <tr style="break-inside: avoid;">
            <td>second column<br>second column</td>
        </tr>
    </table>
</div>

I'd also like to point out that fragmentation inside tables is quite broken, in
fact, because we don't always lay out the objects at their correct location; see
crbug.com/534751 - a source of weird and broken layout.

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:979: for (unsigned
rowIndex = r; paginationStrutOnRow && rowIndex <= totalRows; rowIndex++)
Better check paginationStrutOnRow before entering the loop at all.

[rhogan, 2016-05-04 18:45:12]

Description was changed from

==========
Don't let table rows with inline content straddle page boundaries

BUG=99124
==========

to

==========
Respect break-inside:avoid on table rows

When break-inside:avoid is set on a table row then push the row down to the
pagination boundary if it would otherwise straddle it.

BUG=99124
==========

[rhogan, 2016-05-04 18:46:10]

On 2016/04/04 at 21:05:39, mstensho wrote:
> Thanks for the new tests! Pretty much full coverage now. :)
> 
> How about that move from fast/multicol/ to fragmentation/ , which I suggested
in a previous comment? This is pure fragmentation work. It just so happens that
multicol is a good way of testing it.

OK - I've addressed all your comments I think! (Sorry for the delay in
responding, I kept putting this one on the long finger.)

[mstensho (USE GERRIT), 2016-05-04 20:23:44]

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fragmentation/second-cell-doesnt-fit-in-column.html
(right):

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fragmentation/second-cell-doesnt-fit-in-column.html:4:
border-collapse: collapse;
Does this actually make any difference? Border collapsing is oh so confusing.

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fragmentation/second-cell-doesnt-fit-in-column.html:19:
<td>second column<br>second column</td>
I only see one cell. The name of this file suggests otherwise.

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1063:
rowHeightIncreaseForPagination = std::max<int>(rowHeightIncreaseForPagination,
oldLogicalHeight - rHeight);
This is new. What is it for? Looks like you want to allow the row height to
shrink?

[rhogan, 2016-05-04 20:31:45]

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1063:
rowHeightIncreaseForPagination = std::max<int>(rowHeightIncreaseForPagination,
oldLogicalHeight - rHeight);
On 2016/05/04 at 20:23:44, mstensho wrote:
> This is new. What is it for? Looks like you want to allow the row height to
shrink?

This is per your comment: " When paginationStrutForRow() gives us something
there, we should lay out all affected cells over again, and pay attention if
their heights change. Cell heights will typically shrink, since there'll
typically be fewer pagination struts inside. This height decrease needs to be
propagated to all consecutive rows."

That's what I'm doing here.

[mstensho (USE GERRIT), 2016-05-04 21:02:24]

Can you follow up on my comments regarding the new test as well, please?

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1602773005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1063:
rowHeightIncreaseForPagination = std::max<int>(rowHeightIncreaseForPagination,
oldLogicalHeight - rHeight);
On 2016/05/04 20:31:45, rhogan wrote:
> On 2016/05/04 at 20:23:44, mstensho wrote:
> > This is new. What is it for? Looks like you want to allow the row height to
> shrink?
> 
> This is per your comment: " When paginationStrutForRow() gives us something
> there, we should lay out all affected cells over again, and pay attention if
> their heights change. Cell heights will typically shrink, since there'll
> typically be fewer pagination struts inside. This height decrease needs to be
> propagated to all consecutive rows."
> 
> That's what I'm doing here.

Oh, I said that? Indeed I did. Well, can't argue with myself, can I? :)

[rhogan, 2016-05-09 17:38:19]

On 2016/05/04 at 21:02:24, mstensho wrote:
> 
> Oh, I said that? Indeed I did. Well, can't argue with myself, can I? :)

Got time to take another look?

[mstensho (USE GERRIT), 2016-05-09 18:50:42]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-05-09 18:50:43]

lgtm

[commit-bot: I haz the power, 2016-05-09 18:51:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1602773005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1602773005/300001

[commit-bot: I haz the power, 2016-05-09 20:00:36]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-05-09 20:01:58]

Description was changed from

==========
Respect break-inside:avoid on table rows

When break-inside:avoid is set on a table row then push the row down to the
pagination boundary if it would otherwise straddle it.

BUG=99124
==========

to

==========
Respect break-inside:avoid on table rows

When break-inside:avoid is set on a table row then push the row down to the
pagination boundary if it would otherwise straddle it.

BUG=99124

Committed: https://crrev.com/6827228e3e093e665e2460503c258730f570e477
Cr-Commit-Position: refs/heads/master@{#392404}
==========

[commit-bot: I haz the power, 2016-05-09 20:02:02]

Patchset 16 (id:??) landed as
https://crrev.com/6827228e3e093e665e2460503c258730f570e477
Cr-Commit-Position: refs/heads/master@{#392404}