Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2326303002:
Don't repeat headers if at least one row of content doesn't fit (Closed)
|
Created:
4 years, 3 months ago by rhogan Modified:
4 years, 3 months ago Reviewers:
mstensho (USE GERRIT) CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't repeat headers if at least one row of content doesn't fit
The spec (https://drafts.csswg.org/css-tables-3/#repeated-headers) tells us that
we shouldn't bother repeating headers if a row of content doesn't fit. It's not
clear what to do in the situation where it's just the first page that the row
doesn't fit. I think we ultimately want to do the same as Edge/IE and still
repeat the headers on subsequent pages.
That would require a second layout pass to achieve so for now just drop the
repeating headers if we can't fit a row on the first page.
BUG=624814
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/8bf47bb4e1d0ea9599bc443906fc26c3ce86f152
Cr-Commit-Position: refs/heads/master@{#418838}
Patch Set 1 #
Total comments: 2
Patch Set 2 : bug 624814 #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 26 (13 generated)
Description was changed from ========== Don't repeat headers if at least one row of content doesn't fit BUG=624814 ========== to ========== Don't repeat headers if at least one row of content doesn't fit BUG=624814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robhogan@gmail.com changed reviewers: + mstensho@opera.com
Description was changed from ========== Don't repeat headers if at least one row of content doesn't fit BUG=624814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't repeat headers if at least one row of content doesn't fit The spec (https://drafts.csswg.org/css-tables-3/#repeated-headers) tells us that we shouldn't bother repeating headers if a row of content doesn't fit. It's not clear what to do in the situation where it's just the first page that the row doesn't fit. I think we ultimately want to do the same as Edge/IE and still repeat the headers on subsequent pages. That would require a second layout pass to achieve so for now just drop the repeating headers if we can't fit a row on the first page. BUG=624814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: if (sectionBelow && sectionBelow->firstRow() && sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), sectionBelow->logicalTop())) We don't get a valid result from paginationStrutForRow() when we're not in layout.
https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: if (sectionBelow && sectionBelow->firstRow() && sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), sectionBelow->logicalTop())) On 2016/09/12 19:07:35, rhogan wrote: > We don't get a valid result from paginationStrutForRow() when we're not in > layout. Why is that?
On 2016/09/13 at 06:51:36, mstensho wrote: > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: if (sectionBelow && sectionBelow->firstRow() && sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), sectionBelow->logicalTop())) > On 2016/09/12 19:07:35, rhogan wrote: > > We don't get a valid result from paginationStrutForRow() when we're not in > > layout. > > Why is that? This is because offsetFromLogicalTopOfFirstPage relies on LayoutState to give a correct answer.
On 2016/09/13 14:03:13, rhogan wrote: > On 2016/09/13 at 06:51:36, mstensho wrote: > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: if > (sectionBelow && sectionBelow->firstRow() && > sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), > sectionBelow->logicalTop())) > > On 2016/09/12 19:07:35, rhogan wrote: > > > We don't get a valid result from paginationStrutForRow() when we're not in > > > layout. > > > > Why is that? > > This is because offsetFromLogicalTopOfFirstPage relies on LayoutState to give a > correct answer. Oh, right! How about just storing struts directly on each row instead, though? There's already setPaginationStrut() in LayoutBox, which you can use, since LayoutTableRow is a LayoutBox.
On 2016/09/13 at 17:00:17, mstensho wrote: > On 2016/09/13 14:03:13, rhogan wrote: > > On 2016/09/13 at 06:51:36, mstensho wrote: > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: if > > (sectionBelow && sectionBelow->firstRow() && > > sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), > > sectionBelow->logicalTop())) > > > On 2016/09/12 19:07:35, rhogan wrote: > > > > We don't get a valid result from paginationStrutForRow() when we're not in > > > > layout. > > > > > > Why is that? > > > > This is because offsetFromLogicalTopOfFirstPage relies on LayoutState to give a > > correct answer. > > Oh, right! > > How about just storing struts directly on each row instead, though? There's already setPaginationStrut() in LayoutBox, which you can use, since LayoutTableRow is a LayoutBox. I decided not to do it this way because we definitely want to avoid LayoutBox invadvertently doing something with it when it's acting as the base class for a LayoutRow. That feels like something that could easily happen during a refactoring.
On 2016/09/13 18:06:39, rhogan wrote: > On 2016/09/13 at 17:00:17, mstensho wrote: > > On 2016/09/13 14:03:13, rhogan wrote: > > > On 2016/09/13 at 06:51:36, mstensho wrote: > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): > > > > > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: if > > > (sectionBelow && sectionBelow->firstRow() && > > > sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), > > > sectionBelow->logicalTop())) > > > > On 2016/09/12 19:07:35, rhogan wrote: > > > > > We don't get a valid result from paginationStrutForRow() when we're not > in > > > > > layout. > > > > > > > > Why is that? > > > > > > This is because offsetFromLogicalTopOfFirstPage relies on LayoutState to > give a > > > correct answer. > > > > Oh, right! > > > > How about just storing struts directly on each row instead, though? There's > already setPaginationStrut() in LayoutBox, which you can use, since > LayoutTableRow is a LayoutBox. > > I decided not to do it this way because we definitely want to avoid LayoutBox > invadvertently doing something with it when it's acting as the base class for a > LayoutRow. That feels like something that could easily happen during a > refactoring. We're eventually going to have to store struts on both sections and rows anyway, in order to get proper fragmentation. Storing a pagination strut on a row when it's laid out should be the right thing to do.
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Done. On Tue, 13 Sep 2016 19:27 , <mstensho@opera.com> wrote: > On 2016/09/13 18:06:39, rhogan wrote: > > On 2016/09/13 at 17:00:17, mstensho wrote: > > > On 2016/09/13 14:03:13, rhogan wrote: > > > > On 2016/09/13 at 06:51:36, mstensho wrote: > > > > > > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > > > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp > (left): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > > > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: > if > > > > (sectionBelow && sectionBelow->firstRow() && > > > > sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), > > > > sectionBelow->logicalTop())) > > > > > On 2016/09/12 19:07:35, rhogan wrote: > > > > > > We don't get a valid result from paginationStrutForRow() when > we're > not > > in > > > > > > layout. > > > > > > > > > > Why is that? > > > > > > > > This is because offsetFromLogicalTopOfFirstPage relies on > LayoutState to > > give a > > > > correct answer. > > > > > > Oh, right! > > > > > > How about just storing struts directly on each row instead, though? > There's > > already setPaginationStrut() in LayoutBox, which you can use, since > > LayoutTableRow is a LayoutBox. > > > > I decided not to do it this way because we definitely want to avoid > LayoutBox > > invadvertently doing something with it when it's acting as the base > class for > a > > LayoutRow. That feels like something that could easily happen during a > > refactoring. > > We're eventually going to have to store struts on both sections and rows > anyway, > in order to get proper fragmentation. Storing a pagination strut on a row > when > it's laid out should be the right thing to do. > > https://codereview.chromium.org/2326303002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Done. On Tue, 13 Sep 2016 19:27 , <mstensho@opera.com> wrote: > On 2016/09/13 18:06:39, rhogan wrote: > > On 2016/09/13 at 17:00:17, mstensho wrote: > > > On 2016/09/13 14:03:13, rhogan wrote: > > > > On 2016/09/13 at 06:51:36, mstensho wrote: > > > > > > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > > > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp > (left): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2326303002/diff/1/third_party/WebKit/Source/c... > > > > > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1736: > if > > > > (sectionBelow && sectionBelow->firstRow() && > > > > sectionBelow->paginationStrutForRow(sectionBelow->firstRow(), > > > > sectionBelow->logicalTop())) > > > > > On 2016/09/12 19:07:35, rhogan wrote: > > > > > > We don't get a valid result from paginationStrutForRow() when > we're > not > > in > > > > > > layout. > > > > > > > > > > Why is that? > > > > > > > > This is because offsetFromLogicalTopOfFirstPage relies on > LayoutState to > > give a > > > > correct answer. > > > > > > Oh, right! > > > > > > How about just storing struts directly on each row instead, though? > There's > > already setPaginationStrut() in LayoutBox, which you can use, since > > LayoutTableRow is a LayoutBox. > > > > I decided not to do it this way because we definitely want to avoid > LayoutBox > > invadvertently doing something with it when it's acting as the base > class for > a > > LayoutRow. That feels like something that could easily happen during a > > refactoring. > > We're eventually going to have to store struts on both sections and rows > anyway, > in order to get proper fragmentation. Storing a pagination strut on a row > when > it's laid out should be the right thing to do. > > https://codereview.chromium.org/2326303002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm, but please answer my question in the comments. https://codereview.chromium.org/2326303002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid.html (right): https://codereview.chromium.org/2326303002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid.html:14: break-after: avoid; Note that the engine doesn't support break-{before,after}:avoid yet. https://codereview.chromium.org/2326303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left): https://codereview.chromium.org/2326303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1004: paginationStrutOnRow = paginationStrutForRow(rowLayoutObject, LayoutUnit(m_rowPos[r])); I think this should be the one and only call to paginationStrutForRow() now, and all other call sites should just use row->paginationStrut(). Right? Will you clean that up in a follow-up CL?
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Don't repeat headers if at least one row of content doesn't fit The spec (https://drafts.csswg.org/css-tables-3/#repeated-headers) tells us that we shouldn't bother repeating headers if a row of content doesn't fit. It's not clear what to do in the situation where it's just the first page that the row doesn't fit. I think we ultimately want to do the same as Edge/IE and still repeat the headers on subsequent pages. That would require a second layout pass to achieve so for now just drop the repeating headers if we can't fit a row on the first page. BUG=624814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't repeat headers if at least one row of content doesn't fit The spec (https://drafts.csswg.org/css-tables-3/#repeated-headers) tells us that we shouldn't bother repeating headers if a row of content doesn't fit. It's not clear what to do in the situation where it's just the first page that the row doesn't fit. I think we ultimately want to do the same as Edge/IE and still repeat the headers on subsequent pages. That would require a second layout pass to achieve so for now just drop the repeating headers if we can't fit a row on the first page. BUG=624814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/8bf47bb4e1d0ea9599bc443906fc26c3ce86f152 Cr-Commit-Position: refs/heads/master@{#418838} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8bf47bb4e1d0ea9599bc443906fc26c3ce86f152 Cr-Commit-Position: refs/heads/master@{#418838} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
