New issue
Advanced search Search tips

Issue 699137 link

Starred by 5 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

position:sticky - bad behavior for two composited sticky <th> in table

Project Member Reported by smcgruer@chromium.org, Mar 7 2017

Issue description

Forked from comment #1 on  issue 698358 . Reproduction is https://output.jsbin.com/puyutid on a high DPI device; the right-hand <th> is positioned strangely and doesn't stick.
 

Comment 1 by m.go...@gmail.com, Mar 7 2017

Following from https://bugs.chromium.org/p/chromium/issues/detail?id=698358#c2:

> On what version of Chrome, OS, and is that a high or low DPI screen?

I have a mid-2015 15" Retina MacBook Pro with AMD Radeon R9 M370X 2048 MB
. I'm on macOS 10.12.3 (16D32). I have an external screen - the Apple Thunderbolt display (which is not high-DPI).

This is quite funny. There are 4 cases:
1. The test case as-is (non-composited table) on the internal, high-DPI screen - broken.
2. The test case w/ a composited table on the internal, high-DPI screen - works.
3. The test case as-is (non-composited table) on the external, low-DPI screen - works.
4. The test case w/ a composited table on the external, low-DPI screen - broken.

I've recorded screencasts for each of these scenarios, see attachments.

It seems, then, that on a high-DPI screen only the composited version works and on the low-DPI one only the non-composited does.

Note, also, that the headers are for some reason very wide which is not the case in Safari (but that's probably another issue?)

sticky-high-dpi-non-composited.mov
342 KB Download
sticky-high-dpi-composited.mov
307 KB Download
sticky-low-dpi-non-composited.mov
406 KB Download
sticky-low-dpi-composited.mov
214 KB Download
Status: Assigned (was: Untriaged)
Labels: -Pri-3 Pri-2
Status: Started (was: Assigned)
A few interesting discoveries:

1. Inside of each <td> here there is a <div> that is position:relative. This makes the correct behavior quite strange; I believe the sticky elements should stick but the <div> should actually overlap them as they scroll. (Because both are positioned and the <div> come later in the HTML than the <th>).

2. The existence of the position:relative <div> cause us to create a layer for squashing contents to deal with the overlap, but very strangely it is associated with the *second* <th> rather than the <tbody> which is what I would expect. Something weird there.

3. If you remove the position:relative on the inner divs, then this example works properly in stable but is broken in ToT :(.

I'm going to tackle #3 first, then come back to the question of 'what should we do if those <div> are position:relative?'.
Ooo, this is fun. For #3, what is happening is that the <th> layers are not inside the scroll container layer. The layer tree is:

#document
|
|--> .table-container
|  |
|   --> .table-container
|
|--> th
|--> th

Where it *should* look like:

#document
|
|--> .table-container
|  |
|   --> .table-container
      |
      |--> th
       --> th

This can be seen in https://output.jsbin.com/gipibon/quiet on low or high DPI
Retraction; #5 is incorrect, the table-container is not a stacking context so the <th> end up using a scrollParent - https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/PaintLayer.h?l=684&rcl=832a2a - and that Layer tree is correct.

The issue is in CompositedLayerMapping::updateStickyConstraints, where we compare our compositingContainer and ancestorOverflowLayer for finding our enclosingLayerOffset. There is at least one and possible two flawed uses of this:

1. We call 'compositingContainer->convertToLayerCoords(ancestorOverflowLayer, enclosingLayerOffset)'
2. We compare 'compositingContainer != ancestorOverflowLayer' to determine if we should add a scroll offset back in.

#2 is definitely wrong, we need to only do this if we don't have a scroll parent.

For #1, I'm less sure what the correct behavior is here, will think about it for a bit.
Strange that the commit bot didn't tag this bug, but http://crrev.com/d5514f0 fixes the case mentioned in comment 5. The non-position relative cell case now works properly again as of 59.0.3051.0 .

So back to trying to figure out the relative cell case.
Repro for the relative case is https://output.jsbin.com/lofewaj on low or high DPI btw.
[1:1:0327/104121.522713:257488166321:INFO:property_tree.cc(462)] 7: returning [2.000000 106.000000] - [9.000000 7.000000] - [-9.000000 97.000000] - [2.000000 2.000000] = [0.000000 0.000000]
[1:1:0327/104121.522953:257488166483:INFO:property_tree.cc(462)] 8: returning [69.000000 106.000000] - [9.000000 7.000000] - [0.000000 0.000000] - [69.000000 2.000000] = [-9.000000 97.000000]

At first glance, the second <th> appears to be missing a node->source_to_parent offset, which is what gives the first <th> the correct offset.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 6 2017

Labels: merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13a8712c4430293f11cca14eb50eaca5fd640dec

commit 13a8712c4430293f11cca14eb50eaca5fd640dec
Author: Robert Flack <flackr@chromium.org>
Date: Thu Apr 06 18:43:43 2017

Fix position:sticky location bug when scroller is not a stacking context

This was broken in http://crrev.com/e481938 when I added code to correct
for scroll offset in intermediate layers - that logic is invalid if we
arent a LayerTree descendent of the scroller.

The reason that we didn't spot that this was broken is that we keep
using will-change:transform to force-enable compositing of elements, but
this also creates a stacking context. For now I have switched this to
backface-visibility:hidden, which isn't perfect but at least doesn't
create a stacking context.

BUG=699137,706455
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2765503003
Cr-Commit-Position: refs/heads/master@{#459546}
(cherry picked from commit d5514f08a923485a8ef2a58cab355893d0e55fcb)

Review-Url: https://codereview.chromium.org/2803593007 .
Cr-Commit-Position: refs/branch-heads/3029@{#609}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers-expected.html
[add] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers-stacking-context-expected.html
[add] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers-stacking-context.html
[modify] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html
[modify] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-expected.html
[add] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-stacking-context-expected.html
[add] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-stacking-context.html
[modify] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element.html
[modify] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/13a8712c4430293f11cca14eb50eaca5fd640dec/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp

Diving back in after a few weeks chasing other bugs, unfortunately the repro https://output.jsbin.com/lofewaj/quiet is no longer broken on canary. After a bisect, it turns out that https://chromium.googlesource.com/chromium/src/+/b445984eba859a4e3febed02d619776e4afd3dff changed the layer promotion setup so that both <th> are promoted into their own layers as siblings, as opposed to the strange setup we had before where one was being placed inside the layer for squashing contents.

So we still have a bug I believe (we shouldn't be using node->source_to_parent at all), but I no longer have a reproduction for it at this time. :/
Labels: -Pri-2 Pri-3
Due to #11, we're going to drop this to P3. I can no longer think of how to reproduce the underlying bug, and we fixed the majority of the issues here.
Cc: smcgruer@chromium.org mmanchala@chromium.org ligim...@chromium.org
 Issue 712774  has been merged into this issue.
Owner: ----
Status: Available (was: Started)

Sign in to add a comment