Don't repeat thead if the first row exceeds the height of the page

Don't repeat thead if the first row exceeds the height of the page

BUG=669690
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/6287f21e89cc890abab98237d7b1a0d51f649221
Cr-Commit-Position: refs/heads/master@{#438084}

[rhogan, 2016-12-03 13:18:10]

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

[commit-bot: I haz the power, 2016-12-03 13:18:20]

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-12-03 14:20:00]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2016-12-03 18:06:40]

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

[rhogan, 2016-12-03 18:06:41]

HI mstensho,

https://drafts.csswg.org/css-tables-3/#repeated-headers says not to repeat
headers if the first content row exceeds the height of the page. This conflicts
with what we do when we handle repeating headers on a table nested inside a
content row.

What is the correct behaviour here in your opinion? 

Rendering the nested headers like we do in the now-failing tests seems
reasonable to me.

[mstensho (USE GERRIT), 2016-12-05 11:41:00]

On 2016/12/03 18:06:41, rhogan wrote:
> HI mstensho,
> 
> https://drafts.csswg.org/css-tables-3/#repeated-headers says not to repeat
> headers if the first content row exceeds the height of the page. This
conflicts
> with what we do when we handle repeating headers on a table nested inside a
> content row.
> 
> What is the correct behaviour here in your opinion? 
> 
> Rendering the nested headers like we do in the now-failing tests seems
> reasonable to me.

With the code changes, those tests still leave room in each column for the
header of the outer table, which seems wrong (since it's not painted). Other
than that, I find it reasonable to disallow repeating headers if the TBODY rows
are too tall.

[rhogan, 2016-12-05 18:50:19]

On 2016/12/05 at 11:41:00, mstensho wrote:
> With the code changes, those tests still leave room in each column for the
header of the outer table, which seems wrong (since it's not painted). Other
than that, I find it reasonable to disallow repeating headers if the TBODY rows
are too tall.

So should I just remove the code that deals with repeating headers in nested
tables? By definition if a table is inside a row and extends over more than one
page we're not allowed to repeat its header. If we are still to paint the nested
header it doesn't seem right to paint it in the same position as the outer
table's header, does it?

[mstensho (USE GERRIT), 2016-12-06 17:42:37]

On 2016/12/05 18:50:19, rhogan wrote:
> On 2016/12/05 at 11:41:00, mstensho wrote:
> > With the code changes, those tests still leave room in each column for the
> header of the outer table, which seems wrong (since it's not painted). Other
> than that, I find it reasonable to disallow repeating headers if the TBODY
rows
> are too tall.
> 
> So should I just remove the code that deals with repeating headers in nested
> tables? By definition if a table is inside a row and extends over more than
one
> page we're not allowed to repeat its header. If we are still to paint the
nested
> header it doesn't seem right to paint it in the same position as the outer
> table's header, does it?

That sounds good. Just remove support for nesting and only repeat the inner
header? That should simplify the code a bit as well, I suppose.

[rhogan, 2016-12-06 17:46:47]

Should I repeat it at the same y pos as the outer header though? That seems
potentially confusing.

On Tue, 6 Dec 2016 17:42 , <mstensho@opera.com> wrote:

> On 2016/12/05 18:50:19, rhogan wrote:
> > On 2016/12/05 at 11:41:00, mstensho wrote:
> > > With the code changes, those tests still leave room in each column for
> the
> > header of the outer table, which seems wrong (since it's not painted).
> Other
> > than that, I find it reasonable to disallow repeating headers if the
> TBODY
> rows
> > are too tall.
> >
> > So should I just remove the code that deals with repeating headers in
> nested
> > tables? By definition if a table is inside a row and extends over more
> than
> one
> > page we're not allowed to repeat its header. If we are still to paint the
> nested
> > header it doesn't seem right to paint it in the same position as the
> outer
> > table's header, does it?
>
> That sounds good. Just remove support for nesting and only repeat the inner
> header? That should simplify the code a bit as well, I suppose.
>
> https://codereview.chromium.org/2545243002/
>

-- 
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.

[rhogan, 2016-12-06 17:46:47]

Should I repeat it at the same y pos as the outer header though? That seems
potentially confusing.

On Tue, 6 Dec 2016 17:42 , <mstensho@opera.com> wrote:

> On 2016/12/05 18:50:19, rhogan wrote:
> > On 2016/12/05 at 11:41:00, mstensho wrote:
> > > With the code changes, those tests still leave room in each column for
> the
> > header of the outer table, which seems wrong (since it's not painted).
> Other
> > than that, I find it reasonable to disallow repeating headers if the
> TBODY
> rows
> > are too tall.
> >
> > So should I just remove the code that deals with repeating headers in
> nested
> > tables? By definition if a table is inside a row and extends over more
> than
> one
> > page we're not allowed to repeat its header. If we are still to paint the
> nested
> > header it doesn't seem right to paint it in the same position as the
> outer
> > table's header, does it?
>
> That sounds good. Just remove support for nesting and only repeat the inner
> header? That should simplify the code a bit as well, I suppose.
>
> https://codereview.chromium.org/2545243002/
>

-- 
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.

[mstensho (USE GERRIT), 2016-12-06 17:49:08]

On 2016/12/06 17:46:47, rhogan wrote:
> Should I repeat it at the same y pos as the outer header though? That seems
> potentially confusing.

At the top of every fragmentainer involved. Is there any other option?

[rhogan, 2016-12-06 18:10:29]

Yes I guess you're right!

On Tue, 6 Dec 2016 17:49 , <mstensho@opera.com> wrote:

> On 2016/12/06 17:46:47, rhogan wrote:
> > Should I repeat it at the same y pos as the outer header though? That
> seems
> > potentially confusing.
>
> At the top of every fragmentainer involved. Is there any other option?
>
> https://codereview.chromium.org/2545243002/
>

-- 
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.

[rhogan, 2016-12-06 18:10:30]

Yes I guess you're right!

On Tue, 6 Dec 2016 17:49 , <mstensho@opera.com> wrote:

> On 2016/12/06 17:46:47, rhogan wrote:
> > Should I repeat it at the same y pos as the outer header though? That
> seems
> > potentially confusing.
>
> At the top of every fragmentainer involved. Is there any other option?
>
> https://codereview.chromium.org/2545243002/
>

-- 
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.

[rhogan, 2016-12-10 11:37:37]

Description was changed from

==========
Don't repeat thead if the first row exceeds the height of the page

BUG=669690
==========

to

==========
Don't repeat thead if the first row exceeds the height of the page

BUG=669690
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rhogan, 2016-12-10 11:37:56]

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

[commit-bot: I haz the power, 2016-12-10 11:38:02]

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-12-10 13:22:27]

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

[commit-bot: I haz the power, 2016-12-10 13:22:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rhogan, 2016-12-11 17:27:47]

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

[commit-bot: I haz the power, 2016-12-11 17:28:05]

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-12-11 18:58:33]

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

[commit-bot: I haz the power, 2016-12-11 18:58:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[rhogan, 2016-12-12 14:10:44]

This is ready for another look!

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

https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html
(right):

https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html:7:
tr {
Couldn't you just remove this selector?

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

https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:110:
m_rowOffsetFromRepeatingHeader(LayoutUnit()) {
No need to initialize this explicitly.

https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:474: LayoutUnit
m_rowOffsetFromRepeatingHeader;
This should be stored in LayoutTable instead.

[rhogan, 2016-12-12 20:11:34]

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

[rhogan, 2016-12-12 20:12:17]

On 2016/12/12 at 19:22:41, mstensho wrote:
>
https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html
(right):
> 
>
https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html:7:
tr {
> Couldn't you just remove this selector?
> 
>
https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):
> 
>
https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:110:
m_rowOffsetFromRepeatingHeader(LayoutUnit()) {
> No need to initialize this explicitly.
> 
>
https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):
> 
>
https://codereview.chromium.org/2545243002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.h:474: LayoutUnit
m_rowOffsetFromRepeatingHeader;
> This should be stored in LayoutTable instead.

Thanks for that. Applied your comments!

[commit-bot: I haz the power, 2016-12-12 20:12:47]

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-12-12 20:15:45]

lgtm

[rhogan, 2016-12-12 21:34:39]

The CQ bit was unchecked by robhogan@gmail.com

[rhogan, 2016-12-12 21:34:41]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-12-12 21:36:13]

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-12-12 22:17:10]

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

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

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[rhogan, 2016-12-13 06:30:34]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-12-13 06:30:50]

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-12-13 06:35:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481610634043070,
"parent_rev": "8007204efc34e4896e5d0fc38dfc5f4e0723ddb3", "commit_rev":
"5154772cfe14a0a6cb3b1e6fc3fb9c9916f200d8"}

[commit-bot: I haz the power, 2016-12-13 06:35:35]

Description was changed from

==========
Don't repeat thead if the first row exceeds the height of the page

BUG=669690
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't repeat thead if the first row exceeds the height of the page

BUG=669690
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2545243002
==========

[commit-bot: I haz the power, 2016-12-13 06:35:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-13 06:37:41]

Description was changed from

==========
Don't repeat thead if the first row exceeds the height of the page

BUG=669690
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2545243002
==========

to

==========
Don't repeat thead if the first row exceeds the height of the page

BUG=669690
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/6287f21e89cc890abab98237d7b1a0d51f649221
Cr-Commit-Position: refs/heads/master@{#438084}
==========

[commit-bot: I haz the power, 2016-12-13 06:37:42]

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