Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(753)

Issue 2433473002: Move table row pagination strut insertion to the first layout pass. (Closed)

Created:
4 years, 2 months ago by mstensho (USE GERRIT)
Modified:
4 years, 2 months ago
Reviewers:
rhogan, dgrogan, eae
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, mstensho (USE GERRIT)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move table row pagination strut insertion to the first layout pass. Pagination struts are inserted before a table row, when we should avoid breaking inside it, and it doesn't fit as a whole in its current fragmentainer. We should avoid breaking inside rows when their break-inside is "avoid", or when there are repeating table headers (which turns on break-inside:avoid for all rows in the table). This CL also includes the code that deals with repeating headers, since it proved hard to separate it from the rest. We need to make sure to subtract the struts from previous rows' height now; just like we don't include border spacing in the rows, we should also omit the pagination strut of the next row. In order to be consistent about this, layoutRows() in LayoutTableSection now uses the rows' logical heighs more extensively than before (rather than using the m_rowPos array to calculate heights). This has an implication for rowspanned cells. We now need to wait until we are at their last row before processing them, since we calculate row heights on the fly. There's a small fix here. Previously, the strut wasn't baked into the logical top of a table row, unlike all other layout objects. This resulted in wrong offsets for table rows after fragmentainer breaks, but the cells in there still had correct offsets, so it wasn't possible to observe this bug in any visual test. It does affect a couple of dump-render-tree printing tests, though. Added a couple of tests for this on my own, which use offsetTop and offsetHeight. table-disable-fragmentation.html is just a regression test. We need to be careful to ignore struts when not fragmented. It passed before and it passes now, but I nearly broke it while working on this. BUG=534751 Committed: https://crrev.com/7360974b322ce25f50726e1722f5bcca977577da Cr-Commit-Position: refs/heads/master@{#426265}

Patch Set 1 #

Patch Set 2 : Need to size all rows before applying vertical alignment and flexing, to handle overlapping cells c… #

Patch Set 3 : Rebaseline semi-manually thank you very much. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+245 lines, -74 lines) Patch
A third_party/WebKit/LayoutTests/fragmentation/table-disable-fragmentation.html View 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/table-disable-fragmentation-expected.txt View 1 chunk +9 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/table-row-dimensions.html View 1 chunk +29 lines, -0 lines 0 comments Download
A + third_party/WebKit/LayoutTests/fragmentation/table-row-dimensions-expected.txt View 1 chunk +8 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/table-row-dimensions-with-thead.html View 1 chunk +35 lines, -0 lines 0 comments Download
A + third_party/WebKit/LayoutTests/fragmentation/table-row-dimensions-with-thead-expected.txt View 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/printing/thead-repeats-at-top-of-each-page-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/virtual/threaded/printing/thead-repeats-at-top-of-each-page-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/printing/thead-repeats-at-top-of-each-page-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/virtual/threaded/printing/thead-repeats-at-top-of-each-page-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/win7/printing/thead-repeats-at-top-of-each-page-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/win7/virtual/threaded/printing/thead-repeats-at-top-of-each-page-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/printing/thead-repeats-at-top-of-each-page-multiple-tables-expected.txt View 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTable.cpp View 3 chunks +22 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.h View 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.cpp View 1 6 chunks +81 lines, -48 lines 0 comments Download

Messages

Total messages: 23 (18 generated)
mstensho (USE GERRIT)
I'm struggling with the rebaseline machinery, but this is ready for review.
4 years, 2 months ago (2016-10-19 15:27:50 UTC) #11
eae
LGTM
4 years, 2 months ago (2016-10-19 15:46:18 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2433473002/40001
4 years, 2 months ago (2016-10-19 19:42:19 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 2 months ago (2016-10-19 20:05:40 UTC) #21
commit-bot: I haz the power
4 years, 2 months ago (2016-10-21 13:10:54 UTC) #23
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/7360974b322ce25f50726e1722f5bcca977577da
Cr-Commit-Position: refs/heads/master@{#426265}

Powered by Google App Engine
This is Rietveld 408576698