Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890583002:
Fix incorrect use of coords conversion for sticky elements (Closed)
|
Created:
3 years, 7 months ago by yigu Modified:
3 years, 6 months ago 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, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix incorrect use of coords conversion for sticky elements
Normally, a compositing_container of a sticky element is a descendant of
its ancestor_overflow_layer. We therefore convert the coordinates and
apply the offset to the sticky element. However, the assumption above
may not be true and accordingly applying the computed offset makes no
sense.
BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments #Patch Set 3 : Add comments #
Total comments: 5
Patch Set 4 : Bug fix #
Total comments: 2
Patch Set 5 : Apply source_offset #Patch Set 6 : nit #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html ========== to ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + flackr@chromium.org
PTAL. Thanks!
https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:25: <div class="composited container"> No need to separately composite anything for the expectation right? https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:28: <div class="composited fixed"> Does this being fixed position affect the test? What about fixed position is causing a problem? https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:33: Second line. It's a little unclear what this is testing. I assume the idea is that these should all line up perfectly? Perhaps make the text explain what it is doing and what is expected, i.e. (please change if my understanding is wrong) A sticky element not contained by its scroller should get the offset from its composited container to the scroll ancestor. The result is these lines should all line up. Out of curiosity, does the second line affect the test? https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:318: bool is_descendant = false; Is this not equivalent to !ScrollParent()? https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322: enclosing_layer_offset = LayoutPoint(); How does the sticky element know where it is with respect to the scroller if we don't know where the scroller is with respect to the compositing container?
Thanks Rob. PTAL. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:25: <div class="composited container"> On 2017/05/17 19:44:03, flackr wrote: > No need to separately composite anything for the expectation right? Right. We only need to composite the outer div for LCDText purpose. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:28: <div class="composited fixed"> On 2017/05/17 19:44:03, flackr wrote: > Does this being fixed position affect the test? What about fixed position is > causing a problem? With "position:fixed", the sticky element's compositing container is "composited fixed" and the enclosing_layer_offset we computed from ConvertToLayerCoords (non-zero) would make no sense. Without "fix", its compositing container becomes to "document" and the enclosing_layer_offset is zero therefore we cannot see the bug due to that offset. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:33: Second line. On 2017/05/17 19:44:03, flackr wrote: > It's a little unclear what this is testing. I assume the idea is that these > should all line up perfectly? > > Perhaps make the text explain what it is doing and what is expected, i.e. > (please change if my understanding is wrong) > A sticky element not contained by its scroller should get the offset from its > composited container to the scroll ancestor. > The result is these lines > should all line up. > > Out of curiosity, does the second line affect the test? Actually anything in the sticky div has an incorrect offset without patch. Have replaced the text. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:318: bool is_descendant = false; On 2017/05/17 19:44:03, flackr wrote: > Is this not equivalent to !ScrollParent()? No. is_descendant will be false if compositing_container is not a descendant of ancestor_overflow_layer while !ScrollParent may be true in this case if we composite the ancestor using will-change:transform. That way the sticky element doesn't have a ScrollParent. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322: enclosing_layer_offset = LayoutPoint(); On 2017/05/17 19:44:03, flackr wrote: > How does the sticky element know where it is with respect to the scroller if we > don't know where the scroller is with respect to the compositing container? When the compositing_container is a descendant of the ancestor_overflow_layer as it should be based on the logic of ConvertToLayerCoords, everything is the same as before. But if that's not the case, the enclosing_layer_offset computed from ConvertToLayerCoords doesn't make sense anymore therefore we should reset the value to avoid misuse below.
https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:318: bool is_descendant = false; On 2017/05/17 20:28:15, yigu wrote: > On 2017/05/17 19:44:03, flackr wrote: > > Is this not equivalent to !ScrollParent()? > > No. is_descendant will be false if compositing_container is not a descendant of > ancestor_overflow_layer while !ScrollParent may be true in this case if we > composite the ancestor using will-change:transform. That way the sticky element > doesn't have a ScrollParent. Composite which ancestor? If you apply will-change: transform to the scroller, then the compositing container is the scroller so it will get no offset from ConvertToLayerCoords called on itself. If will-change: transform is above the scroller then ScrollParent will be true. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322: enclosing_layer_offset = LayoutPoint(); On 2017/05/17 20:28:15, yigu wrote: > On 2017/05/17 19:44:03, flackr wrote: > > How does the sticky element know where it is with respect to the scroller if > we > > don't know where the scroller is with respect to the compositing container? > > When the compositing_container is a descendant of the ancestor_overflow_layer as > it should be based on the logic of ConvertToLayerCoords, everything is the same > as before. But if that's not the case, the enclosing_layer_offset computed from > ConvertToLayerCoords doesn't make sense anymore therefore we should reset the > value to avoid misuse below. My concern is when the scroller is not at the top-left corner of the compositing container. Don't we need to add in the position of the scroller in order to find out where the sticky_box_rect maps to in the ancestor composited layer? e.g. scrollContainerRelativeStickyBoxRect = (100, 100) but the scroll container is not composited, and is at (50, 50) within compositing_container. This means that the parent_relative_sticky_box_rect should be (150, 150) which would seem to require adding that (50, 50) offset.
Thanks Rob for the investigation! PTAL.
Description was changed from ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + chrishtr@chromium.org
chrishtr@chromium.org changed reviewers: + trchen@chromium.org
+trchen for review while I am out.
On 2017/05/18 22:03:38, chrishtr wrote: > +trchen for review while I am out. Okie dokie. I'm not very familiar with sticky position though. Will need to take a while to study related code.
Thanks! I've updated the patch. PTAL.
https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:5: } Remove unused class. https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:4: position: fixed; I think will-change: transform will work just as well, and has fewer side effects. https://codereview.chromium.org/2890583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:326: !compositing_container->GetLayoutObject().IsFixedPositioned()) { I thought we wanted to only compute this offset if the compositing_container was a descendant but this will compute it even if the compositing container is an ancestor but is not fixed position (i.e. the scroll parent = true case).
Description was changed from ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Thanks Rob. I've updated the patch which doesn't break Stephen's layout test (https://output.jsbin.com/pukoso). Let's see how it goes with the rest of the layout tests. https://codereview.chromium.org/2890583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:326: !compositing_container->GetLayoutObject().IsFixedPositioned()) { On 2017/05/19 18:28:31, flackr wrote: > I thought we wanted to only compute this offset if the compositing_container was > a descendant but this will compute it even if the compositing container is an > ancestor but is not fixed position (i.e. the scroll parent = true case). Done.
This looks good, I think you missed some comments in patch set 3 though?
Thanks Rob. I've removed the unused class. But the compositing container should be fix positioned to reproduce the bug. Please see below. https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:5: } On 2017/05/19 18:28:31, flackr wrote: > Remove unused class. Done. https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:4: position: fixed; On 2017/05/19 18:28:31, flackr wrote: > I think will-change: transform will work just as well, and has fewer side > effects. I think position:fixed is necessary to reproduce this bug (although "will-change:transform" creates the same layer structure). For sticky elements, the offset from compositing_container to ancestor_overflow_layer is computed by source_to_parent iff it has ScrollParent and fix positioned container.
https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:4: position: fixed; On 2017/05/23 19:16:40, yigu wrote: > On 2017/05/19 18:28:31, flackr wrote: > > I think will-change: transform will work just as well, and has fewer side > > effects. > > I think position:fixed is necessary to reproduce this bug (although > "will-change:transform" creates the same layer structure). For sticky elements, > the offset from compositing_container to ancestor_overflow_layer is computed by > source_to_parent iff it has ScrollParent and fix positioned container. Doesn't this imply that the test would still be broken for will-change: transform if the source_to_parent calculation isn't done?
Are we abandoning this in favor of https://codereview.chromium.org/2911463002/ ? If so, can you close this patch? Thanks.
Description was changed from ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
