Use ceil() when integerizing pagination struts before table rows. (issue 2382043003 by mstensho@opera.com)

0 views
Skip to first unread message

mste...@opera.com

unread,
Sep 30, 2016, 4:22:29 PM9/30/16
to e...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
Reviewers: eae
CL: https://codereview.chromium.org/2382043003/

Description:
Use ceil() when integerizing pagination struts before table rows.

Subpixel rendering is not supported in table parts, so everything needs to be
integers. However, instead of rounding the pagination strut down to the nearest
integer, round it up. This way we at least make sure that we manage to push all
the content over to the designated fragmentainer, rather than leaving one tiny
strip behind in the previous fragmentainer. There'll still be off-by-one
errors, but at least all the content is in the right fragmentainer.

Updated some tests to not use subpixel multicol heights, since what they
required cannot really be satisfied without adding full subpixel support to
tables.

Also added a new test that *does* use subpixel multicol height. This test
merely makes sure that nothing is left behind in the previous fragmentainer at
breaks, without worrying about the exact top position of the objects.

This problem was discovered while working on bug 487026, which is about
reducing the amount of forced re-layouts that we do for fragmentation, and it
turns out that table layout in general, and perhaps strut calculation there in
particular, tends to need more layout passes it explicitly asks for (so it
depends on other parts of the system dealing out layout passes for free). Added
body { overflow:hidden; } declarations to some tests, to reduce the number of
layout passes you get for free, i.e. make the tests more evil.

BUG=487026

Affected files (+48, -4 lines):
M third_party/WebKit/LayoutTests/fragmentation/cells-dont-fit-on-page-paginated.html
M third_party/WebKit/LayoutTests/fragmentation/multi-line-cells-paginated.html
M third_party/WebKit/LayoutTests/fragmentation/single-line-cells-paginated.html
A third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer.html
A third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer-expected.txt
M third_party/WebKit/Source/core/layout/LayoutTableSection.cpp


Index: third_party/WebKit/LayoutTests/fragmentation/cells-dont-fit-on-page-paginated.html
diff --git a/third_party/WebKit/LayoutTests/fragmentation/cells-dont-fit-on-page-paginated.html b/third_party/WebKit/LayoutTests/fragmentation/cells-dont-fit-on-page-paginated.html
index 84034832e6891ca07617070178cfad1afc8337a6..578d5f3a1eb98c527c53232415a0002772770e75 100644
--- a/third_party/WebKit/LayoutTests/fragmentation/cells-dont-fit-on-page-paginated.html
+++ b/third_party/WebKit/LayoutTests/fragmentation/cells-dont-fit-on-page-paginated.html
@@ -1,12 +1,13 @@
<!DOCTYPE html>
<style>
+body { overflow: hidden; } /* Avoid triggering a second layout pass, as that might hide bugs. */
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;">
+<div style="columns:2; column-fill:auto; column-gap:0; width:4em; height:5.5em;">
<table>
<tr>
<td><div><br><br></div></td>
Index: third_party/WebKit/LayoutTests/fragmentation/multi-line-cells-paginated.html
diff --git a/third_party/WebKit/LayoutTests/fragmentation/multi-line-cells-paginated.html b/third_party/WebKit/LayoutTests/fragmentation/multi-line-cells-paginated.html
index 29f1b5cff47939c354c8532ea46778d9d85e096c..4273404dcc4ff6c3b90485e18333b43589a24a28 100644
--- a/third_party/WebKit/LayoutTests/fragmentation/multi-line-cells-paginated.html
+++ b/third_party/WebKit/LayoutTests/fragmentation/multi-line-cells-paginated.html
@@ -1,12 +1,13 @@
<!DOCTYPE html>
<style>
+body { overflow: hidden; } /* Avoid triggering a second layout pass, as that might hide bugs. */
table { border-spacing:0; line-height:1.5em; }
tr { break-inside:avoid; }
td { padding:0; background:red; }
td > div { width:2em; background:green; }
</style>
<p>crbug.com/99124: A multi-line cell doesn't straddle a page-break when break-inside:avoid is specified on the row. There should be a green square and no red below.</p>
-<div style="columns:2; column-fill:auto; column-gap:0; width:4em; height:7.9em;">
+<div style="columns:2; column-fill:auto; column-gap:0; width:4em; height:8.5em;">
<table>
<tr>
<td><div><br><br></div></td>
Index: third_party/WebKit/LayoutTests/fragmentation/single-line-cells-paginated.html
diff --git a/third_party/WebKit/LayoutTests/fragmentation/single-line-cells-paginated.html b/third_party/WebKit/LayoutTests/fragmentation/single-line-cells-paginated.html
index 18f2b173a7c9850da4e180b6c418585151a05066..a7c67b221c12ef6b1ae7c5e479227ef3b12ac325 100644
--- a/third_party/WebKit/LayoutTests/fragmentation/single-line-cells-paginated.html
+++ b/third_party/WebKit/LayoutTests/fragmentation/single-line-cells-paginated.html
@@ -1,12 +1,13 @@
<!DOCTYPE html>
<style>
+body { overflow: hidden; } /* Avoid triggering a second layout pass, as that might hide bugs. */
table { border-spacing:0; line-height:1em; }
tr { break-inside:avoid; }
td { padding:0; background:red; }
td > div { width:1em; background:green; }
</style>
<p>crbug.com/99124: A single-line cell doesn't straddle a page-break when break-inside:avoid is specified on the row. There should be a green square and no red below.</p>
-<div style="columns:2; column-fill:auto; column-gap:0; width:2em; height:3.9em;">
+<div style="columns:2; column-fill:auto; column-gap:0; width:2em; height:3.8em;">
<table>
<tr>
<td><div><br></div></td>
Index: third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer-expected.txt
diff --git a/third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer-expected.txt b/third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer-expected.txt
new file mode 100644
index 0000000000000000000000000000000000000000..a1e1361164e6fbdaaf0840ad7669f8931e92f2da
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer-expected.txt
@@ -0,0 +1,3 @@
+There should be six blue squares below. They should ideally be perfectly vertically aligned, but since Blink doesn't support subpixel rendering inside tables, off-by-one is acceptable. But there should be no blue lines below the squares.
+
+PASS
Index: third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer.html
diff --git a/third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer.html b/third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer.html
new file mode 100644
index 0000000000000000000000000000000000000000..f3aacc21c0fb1696bf569fda61d18c713af65310
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fragmentation/table-in-subpixel-fragmentainer.html
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<style>
+body { overflow:hidden; } /* Avoid triggering a second layout pass, as that might hide bugs. */
+.table { display:table; width:100%; }
+.row { display:table-row; break-inside:avoid; }
+.row > div { width:100%; height:60px; background:blue; }
+</style>
+<p>There should be six blue squares below. They should ideally be
+ perfectly vertically aligned, but since Blink doesn't support
+ subpixel rendering inside tables, off-by-one is acceptable. But
+ there should be no blue lines below the squares.</p>
+<div id="multicol" style="position:relative; width:410px; columns:6; column-fill:auto; column-gap:10px; height:88.7px;">
+ <div class="table">
+ <div class="row">
+ <div data-offset-x="0"></div>
+ </div>
+ <div class="row">
+ <div data-offset-x="70"></div>
+ </div>
+ <div class="row">
+ <div data-offset-x="140"></div>
+ </div>
+ <div class="row">
+ <div data-offset-x="210"></div>
+ </div>
+ <div class="row">
+ <div data-offset-x="280"></div>
+ </div>
+ <div class="row">
+ <div data-offset-x="350"></div>
+ </div>
+ </div>
+</div>
+
+<script src="../resources/check-layout.js"></script>
+<script>
+ checkLayout("#multicol");
+</script>
Index: third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
index bbe80054f0e4c599e4eb96399e5e697c07f28487..e4d2481ddf1c5af5d145f34d604ab9fadb179fd5 100644
--- a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
@@ -1158,7 +1158,7 @@ int LayoutTableSection::paginationStrutForRow(LayoutTableRow* row, LayoutUnit lo
// completely. No point in leaving a page completely blank.
return 0;
}
- return paginationStrut.toInt();
+ return paginationStrut.ceil();
}

void LayoutTableSection::computeOverflowFromCells()


e...@chromium.org

unread,
Sep 30, 2016, 4:26:44 PM9/30/16
to mste...@opera.com, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
LGTM

Please add a comment explaining why to the code.

https://codereview.chromium.org/2382043003/

mste...@opera.com

unread,
Sep 30, 2016, 5:00:17 PM9/30/16
to e...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
On 2016/09/30 20:26:22, eae wrote:
> LGTM
>
> Please add a comment explaining why to the code.

Ohh, that was a good idea! :)

Done.

https://codereview.chromium.org/2382043003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 5:01:04 PM9/30/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 9:12:07 PM9/30/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
Exceeded global retry quota

https://codereview.chromium.org/2382043003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 4:13:53 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 4:17:12 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
Failed to apply patch for
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:
While running git apply --index -3 -p1;
error: patch failed:
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1158
Falling back to three-way merge...
Applied patch to
'third_party/WebKit/Source/core/layout/LayoutTableSection.cpp' with conflicts.
U third_party/WebKit/Source/core/layout/LayoutTableSection.cpp

Patch: third_party/WebKit/Source/core/layout/LayoutTableSection.cpp

Index: third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
index
bbe80054f0e4c599e4eb96399e5e697c07f28487..c15d383be268faf9361b96fc71341a9840ca9c85

100644
--- a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
@@ -1158,7 +1158,9 @@ int

LayoutTableSection::paginationStrutForRow(LayoutTableRow* row, LayoutUnit lo
// completely. No point in leaving a page completely blank.
return 0;
}
- return paginationStrut.toInt();
+ // Table layout parts only work on integers, so we have to round. Round up,
to make sure that no
+ // fraction ever gets left behind in the previous fragmentainer.

+ return paginationStrut.ceil();
}

void LayoutTableSection::computeOverflowFromCells()


commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 6:44:25 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 8:50:05 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 9:27:03 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 9:32:31 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean


https://codereview.chromium.org/2382043003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 9:33:57 AM10/1/16
to mste...@opera.com, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org
Patchset 3 (id:??) landed as
https://crrev.com/435e6b536c9dd3115b7ebe9ad915487b10f0f537
Cr-Commit-Position: refs/heads/master@{#422312}

https://codereview.chromium.org/2382043003/
Reply all
Reply to author
Forward
0 new messages