position:sticky - bad behavior for two composited sticky <th> in table |
||||||||
Issue descriptionForked 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.
,
Mar 8 2017
,
Mar 20 2017
,
Mar 20 2017
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?'.
,
Mar 20 2017
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
,
Mar 20 2017
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.
,
Mar 27 2017
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.
,
Mar 27 2017
Repro for the relative case is https://output.jsbin.com/lofewaj on low or high DPI btw.
,
Mar 27 2017
[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.
,
Apr 6 2017
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
,
May 4 2017
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. :/
,
May 18 2017
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.
,
May 25 2017
Issue 712774 has been merged into this issue.
,
Feb 27 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by m.go...@gmail.com
, Mar 7 2017342 KB
342 KB Download
307 KB
307 KB Download
406 KB
406 KB Download
214 KB
214 KB Download