Offset repeating theads correctly when two tables adjoin at a page border

Offset repeating theads correctly when two tables adjoin at a page border

When a header group is near a page border and doesn't fit on the page
it may get a pagination strut to move it to the top of the next page.

We need to subtract out that strut when storing the header's height for use
later (when we paint the header across pages and have to move content rows
below it).

Also, when figuring out where the header starts on a series of pages we need
to account for any strut it has so that we identify the place where the header
group starts rather than just the table.

BUG=634404
Committed: https://crrev.com/581269dfb3c302163b973edcbbbf508291fe6671
Cr-Commit-Position: refs/heads/master@{#411747}

[rhogan, 2016-08-05 21:03:04]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 21:03:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 22:34:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 22:34:01]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-08-09 20:34:38]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com, wangxianzhu@chromium.org

[rhogan, 2016-08-09 20:34:39]

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:551: offsetForTableHeaders
+= section->logicalHeight() -
section->paginationStrutForRow(section->firstRow(), section->logicalTop());
We're only interested in the actual content height of the header group, not any
pagination strut included in the section's height to make it appear at the top
of a subsequent page.

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:47: LayoutUnit
headerGroupOffset = table->pageLogicalOffset() +
m_layoutTableSection.paginationStrutForRow(m_layoutTableSection.firstRow(),
table->pageLogicalOffset());
We need to account for any pagination strut on the header group because it, plus
the table's offset within the page, represents the position at which the header
group actually starts.

[rhogan, 2016-08-09 20:36:47]

Description was changed from

==========
Offset repeating theads correctly when two tables adjoin at a page border

BUG=634404
==========

to

==========
Offset repeating theads correctly when two tables adjoin at a page border

When a header group is near a page border and doesn't fit on the page
it may get a pagination strut to move it to the top of the next page.

We need to account for that strut when storing the height of the group for later
use.

BUG=634404
==========

[rhogan, 2016-08-09 20:39:46]

Description was changed from

==========
Offset repeating theads correctly when two tables adjoin at a page border

When a header group is near a page border and doesn't fit on the page
it may get a pagination strut to move it to the top of the next page.

We need to account for that strut when storing the height of the group for later
use.

BUG=634404
==========

to

==========
Offset repeating theads correctly when two tables adjoin at a page border

When a header group is near a page border and doesn't fit on the page
it may get a pagination strut to move it to the top of the next page.

We need to subtract out that strut when storing the header's height for use
later (when we paint the header across pages and have to move content rows
below it).

Also, when figuring out where the header starts on a series of pages we need
to account for any strut it has so that we identify the place where the header
group starts rather than just the table.

BUG=634404
==========

[rhogan, 2016-08-09 20:47:12]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 20:48:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 22:23:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 22:23:25]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-08-10 09:42:48]

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:45: LayoutUnit
pageHeight = table->pageLogicalHeightForOffset(LayoutUnit());
(not for this CL) Page height isn't necessarily uniform throughout the entire
fragmentation context, so this is potentially wrong.

https://codereview.chromium.org/2219153002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:47: LayoutUnit
headerGroupOffset = table->pageLogicalOffset() +
m_layoutTableSection.paginationStrutForRow(m_layoutTableSection.firstRow(),
table->pageLogicalOffset());
On 2016/08/09 20:34:39, rhogan wrote:
> We need to account for any pagination strut on the header group because it,
plus
> the table's offset within the page, represents the position at which the
header
> group actually starts.

Just an observation (probably not something to do as part of this CL): I wonder
if everything would be easier if we propagated the strut to the LayoutTable, so
that we wouldn't have to deal with it here. That might require some work, but it
would be the correct thing to do. Spec-wise, there are no break opportunities
between the top border edge of the table and the bottom edge of the first piece
of actual content (inside a cell in the first row in the first section,
typically).

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/repeating-thead-multiple-tables-page-border.html
(right):

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/repeating-thead-multiple-tables-page-border.html:27:
<thead id="top-thead">
Duplicate and unused #top-thead.

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-cells-multiple-tables-no-repeating-thead.html
(right):

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-cells-multiple-tables-no-repeating-thead.html:11:
<div style="-webkit-columns:3; line-height: 20px; column-fill: auto;
height:190px;">
Note that the -webkit- prefix is no longer required for multicol.

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-cells-multiple-tables-no-repeating-thead.html:27:
<thead id="top-thead">
#top-thead already used above. You don't seem to need the ID at all, though.

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:552: offsetForTableHeaders
+= section->logicalHeight() -
section->paginationStrutForRow(section->firstRow(), section->logicalTop());
section->firstRow() may be nullptr.

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:49: LayoutUnit
headerGroupOffset = table->pageLogicalOffset() +
m_layoutTableSection.paginationStrutForRow(m_layoutTableSection.firstRow(),
table->pageLogicalOffset());
m_layoutTableSection.firstRow() may be nullptr.

What's the difference between calculating the pagination strut for the first
row, and simply using its logicalTop(), BTW?

[rhogan, 2016-08-10 18:05:14]

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:49: LayoutUnit
headerGroupOffset = table->pageLogicalOffset() +
m_layoutTableSection.paginationStrutForRow(m_layoutTableSection.firstRow(),
table->pageLogicalOffset());
On 2016/08/10 at 09:42:48, mstensho wrote:
> m_layoutTableSection.firstRow() may be nullptr.
> 
> What's the difference between calculating the pagination strut for the first
row, and simply using its logicalTop(), BTW?

We don't propagate the strut to the logical top of the row or the section, we
just add it to the logicalHeight() of the row. We don't even store it like we do
for LayoutBox. I'm thinking it might be cleaner to just store it on the
paginationStrut() on the LayoutTableRow rather than calculate it like this. We
only need to call on it when the section is a header group but I guess it would
be less confusing to store it in all cases.

[rhogan, 2016-08-11 17:40:50]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 17:41:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2016-08-11 17:46:04]

On 2016/08/10 at 18:05:14, rhogan wrote:
>
https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):
> 
>
https://codereview.chromium.org/2219153002/diff/10001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:49: LayoutUnit
headerGroupOffset = table->pageLogicalOffset() +
m_layoutTableSection.paginationStrutForRow(m_layoutTableSection.firstRow(),
table->pageLogicalOffset());
> On 2016/08/10 at 09:42:48, mstensho wrote:
> > m_layoutTableSection.firstRow() may be nullptr.
> > 
> > What's the difference between calculating the pagination strut for the first
row, and simply using its logicalTop(), BTW?
> 
> We don't propagate the strut to the logical top of the row or the section, we
just add it to the logicalHeight() of the row. We don't even store it like we do
for LayoutBox. I'm thinking it might be cleaner to just store it on the
paginationStrut() on the LayoutTableRow rather than calculate it like this. We
only need to call on it when the section is a header group but I guess it would
be less confusing to store it in all cases.

I decided against this - it doesn't have any use beyond these specialized cases.
Got time to take another look?

[commit-bot: I haz the power, 2016-08-11 20:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 20:23:45]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-08-12 08:36:59]

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

https://codereview.chromium.org/2219153002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1138: if (!row)
Why would you ever want to pass a null pointer? Better move this check to the
call sites, I think.

[rhogan, 2016-08-12 18:39:36]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[rhogan, 2016-08-12 18:39:48]

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

https://codereview.chromium.org/2219153002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1138: if (!row)
On 2016/08/12 at 08:36:59, mstensho wrote:
> Why would you ever want to pass a null pointer? Better move this check to the
call sites, I think.

Sure - updated.

[commit-bot: I haz the power, 2016-08-12 18:39:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mstensho (USE GERRIT), 2016-08-12 19:10:22]

lgtm

[commit-bot: I haz the power, 2016-08-12 19:51:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 19:51:55]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-08-12 19:57:01]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-08-12 19:57:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 20:03:01]

Description was changed from

==========
Offset repeating theads correctly when two tables adjoin at a page border

When a header group is near a page border and doesn't fit on the page
it may get a pagination strut to move it to the top of the next page.

We need to subtract out that strut when storing the header's height for use
later (when we paint the header across pages and have to move content rows
below it).

Also, when figuring out where the header starts on a series of pages we need
to account for any strut it has so that we identify the place where the header
group starts rather than just the table.

BUG=634404
==========

to

==========
Offset repeating theads correctly when two tables adjoin at a page border

When a header group is near a page border and doesn't fit on the page
it may get a pagination strut to move it to the top of the next page.

We need to subtract out that strut when storing the header's height for use
later (when we paint the header across pages and have to move content rows
below it).

Also, when figuring out where the header starts on a series of pages we need
to account for any strut it has so that we identify the place where the header
group starts rather than just the table.

BUG=634404

Committed: https://crrev.com/581269dfb3c302163b973edcbbbf508291fe6671
Cr-Commit-Position: refs/heads/master@{#411747}
==========

[commit-bot: I haz the power, 2016-08-12 20:03:02]

Patchset 4 (id:??) landed as
https://crrev.com/581269dfb3c302163b973edcbbbf508291fe6671
Cr-Commit-Position: refs/heads/master@{#411747}