Enable position:sticky for <thead> and <tr> elements

Enable position:sticky for <thead> and <tr> elements

With the recent changes in tip-of-tree for nested sticky and correctly
walking the ancestor sticky chain, it appears we are able to support
sticky correctly for <thead> and <tr> elements. Supporting this enables
the use-case of (much) easier multi-row sticky table headers.

BUG=702927

[smcgruer, 2017-03-29 18:02:56]

smcgruer@chromium.org changed reviewers:
+ flackr@chromium.org

[smcgruer, 2017-03-29 18:02:57]

I was surprised, but it seems like we might be able to correctly support sticky
on <thead> and <tr> elements. If you can come up with any counter-examples that
would be great.

My list of test cases for this CL is http://output.jsbin.com/toxiliz .

https://codereview.chromium.org/2786743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2786743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:992:
LayoutObject* locationContainerForSticky =
1. I'm not sure if we also want to do this for its use in
skippedContainersOffset above. Thoughts?

2. I thought we would also want to do this for findFirstStickyBetween, but was
unable to construct a test case. I was thinking of something like the following,
thoughts?

<tr class=".sticky">
  <th><div class=".sticky"></div></th>
</tr>

I think in this case the <div> would fail to notice the <tr> sticky ancestor,
but in my tests this doesn't seem to actually cause any problems which is
strange.

[smcgruer, 2017-03-29 18:03:05]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 18:03:33]

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), 2017-03-29 18:27:54]

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

[mstensho (USE GERRIT), 2017-03-29 18:27:55]

I find it a bit unsettling to let position:sticky on internal table boxes allow
them to become containers of absolutely positioned descendants, while setting
position:relative still has no such effect.

Also, what about things like z-index on sticky elements? I suppose that
position:sticky makes an object an entry in a stacking context. At least that's
what position:relative/absolute/fix does.

I really think we need to implement position:relative support for
display:table-* (the relevant types here would be table-row-group,
table-header-group, table-footer-group, table-row, table-cell, table-caption)
before we attempt to support position:sticky.

https://codereview.chromium.org/2786743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2786743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:350:
style.position() == EPosition::kRelative)
This change will let e.g. a table-row act as a containing block for absolutely
positioned descendants. I cannot imagine that it will work out of the box.

<div style="display:table-row; position:sticky;">
    <div style="position:absolute;"></div>
</div>

[commit-bot: I haz the power, 2017-03-29 20:07:03]

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

[commit-bot: I haz the power, 2017-03-29 20:07:04]

Dry run: This issue passed the CQ dry run.

[flackr, 2017-04-11 21:08:51]

On 2017/03/29 18:27:55, mstensho wrote:
> I find it a bit unsettling to let position:sticky on internal table boxes
allow
> them to become containers of absolutely positioned descendants, while setting
> position:relative still has no such effect.
> 
> Also, what about things like z-index on sticky elements? I suppose that
> position:sticky makes an object an entry in a stacking context. At least
that's
> what position:relative/absolute/fix does.

Correct.
> 
> I really think we need to implement position:relative support for
> display:table-* (the relevant types here would be table-row-group,
> table-header-group, table-footer-group, table-row, table-cell, table-caption)
> before we attempt to support position:sticky.

I'm inclined to agree. sticky with no constraints set should behave identically
to position relative, and I'd be worried about exposing bugs with position:
sticky that are due to bugs in position: relative. I'm not sure how much work
there is involved in supporting relative on table types, but I think we should
tackle this first or at the same time.

> 
>
https://codereview.chromium.org/2786743002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/2786743002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:350:
> style.position() == EPosition::kRelative)
> This change will let e.g. a table-row act as a containing block for absolutely
> positioned descendants. I cannot imagine that it will work out of the box.
> 
> <div style="display:table-row; position:sticky;">
>     <div style="position:absolute;"></div>
> </div>

[smcgruer, 2017-04-12 14:17:01]

> > I really think we need to implement position:relative support for
> > display:table-* (the relevant types here would be table-row-group,
> > table-header-group, table-footer-group, table-row, table-cell,
table-caption)
> > before we attempt to support position:sticky.
> 
> I'm inclined to agree. sticky with no constraints set should behave
identically
> to position relative, and I'd be worried about exposing bugs with position:
> sticky that are due to bugs in position: relative. I'm not sure how much work
> there is involved in supporting relative on table types, but I think we should
> tackle this first or at the same time.

Ok, I will close this issue for now. Is there a bug tracking position:relative
support for display:table-* ? Users are quite upset that we broke sticky on
<thead> (since M56 had this working and M57 disabled it). The common use-case is
sticky multi-row headers, where applying sticky to individual <th> elements is
hacky at best and usually doesn't display nicely (you have to guess the height
of the cell).

[smcgruer, 2017-04-12 14:17:11]

Description was changed from

==========
Enable position:sticky for <thead> and <tr> elements

With the recent changes in tip-of-tree for nested sticky and correctly
walking the ancestor sticky chain, it appears we are able to support
sticky correctly for <thead> and <tr> elements. Supporting this enables
the use-case of (much) easier multi-row sticky table headers.

BUG=702927
==========

to

==========
Enable position:sticky for <thead> and <tr> elements

With the recent changes in tip-of-tree for nested sticky and correctly
walking the ancestor sticky chain, it appears we are able to support
sticky correctly for <thead> and <tr> elements. Supporting this enables
the use-case of (much) easier multi-row sticky table headers.

BUG=702927
==========

[Yiorsi, 2017-04-13 02:40:26]

On 2017/04/12 14:17:01, smcgruer wrote:
> > > I really think we need to implement position:relative support for
> > > display:table-* (the relevant types here would be table-row-group,
> > > table-header-group, table-footer-group, table-row, table-cell,
> table-caption)
> > > before we attempt to support position:sticky.
> > 
> > I'm inclined to agree. sticky with no constraints set should behave
> identically
> > to position relative, and I'd be worried about exposing bugs with position:
> > sticky that are due to bugs in position: relative. I'm not sure how much
work
> > there is involved in supporting relative on table types, but I think we
should
> > tackle this first or at the same time.
> 
> Ok, I will close this issue for now. Is there a bug tracking position:relative
> support for display:table-* ? Users are quite upset that we broke sticky on
> <thead> (since M56 had this working and M57 disabled it). The common use-case
is
> sticky multi-row headers, where applying sticky to individual <th> elements is
> hacky at best and usually doesn't display nicely (you have to guess the height
> of the cell).

See: https://bugs.chromium.org/p/chromium/issues/detail?id=417223