Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833233002:
[blink] Perspective layer should be considered fixed-pos container layer (Closed)
|
Created:
3 years, 8 months ago by trchen Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[blink] Perspective layer should be considered fixed-pos container layer
This CL fixes a bug that fixed-position descendants skip its containing
block's perspective transform. Perspective layer (CompositedLayerMapping::
child_transform_layer_) should be considered fixed-pos container if
present.
BUG=714001
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2833233002
Cr-Commit-Position: refs/heads/master@{#467175}
Committed: https://chromium.googlesource.com/chromium/src/+/25eaaeb522126f998e9bd83056f96a5ce3cc78c2
Patch Set 1 #
Total comments: 1
Patch Set 2 : apply to more layers && add explainations #
Messages
Total messages: 23 (14 generated)
Description was changed from ========== [blink] Perspective layer should be considered fixed-pos container layer This CL fixes a bug that fixed-position descendants skip its containing block's perspective transform. Perspective layer (CompositedLayerMapping:: child_transform_layer_) should be considered fixed-pos container if present. BUG=714001 ========== to ========== [blink] Perspective layer should be considered fixed-pos container layer This CL fixes a bug that fixed-position descendants skip its containing block's perspective transform. Perspective layer (CompositedLayerMapping:: child_transform_layer_) should be considered fixed-pos container if present. BUG=714001 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
trchen@chromium.org changed reviewers: + chrishtr@chromium.org
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1616: child_transform_layer_ ? child_transform_layer_.get() Add a comment explaining.
trchen@chromium.org changed reviewers: + ajuma@chromium.org
On 2017/04/24 15:44:22, chrishtr wrote: > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > File > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > (right): > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1616: > child_transform_layer_ ? child_transform_layer_.get() > Add a comment explaining. Thanks for asking! I realized this isn't right when I tried to explain it. :) For element that is both fixed-pos container and scrollable, its fixed-pos descendants are positioned by the SCROLLED box. i.e. scroll along in-flow contents. Therefore we should use CLM::ChildForSuperlayers() as fixed-pos container layer instead. Patch set 1 restores fixed-pos container layer behavior to match what we had prior to ajuma's CL r449448. However the existing code before r449448 was wrong too. Fixed-pos elements would incorrectly escape scrolling in cc. It wasn't obvious because it is only transient until main thread commits. i.e. Fixed-pos child scrolled by a transformed parent looks lag behind on scroll. https://output.jsbin.com/qeqomet/ demonstrates the bug. Scrolling the scroller vertically should never reveal a white box that is occluded by a fixed-pos sibling. I'll upload a new patch set that uses CLM::ChildForSuperlayers() instead.
On 2017/04/25 03:18:22, trchen wrote: > On 2017/04/24 15:44:22, chrishtr wrote: > > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > > File > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > > (right): > > > > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1616: > > child_transform_layer_ ? child_transform_layer_.get() > > Add a comment explaining. > > Thanks for asking! I realized this isn't right when I tried to explain it. :) > > For element that is both fixed-pos container and scrollable, its fixed-pos > descendants are positioned by the SCROLLED box. i.e. scroll along in-flow > contents. Therefore we should use CLM::ChildForSuperlayers() as fixed-pos > container layer instead. > > Patch set 1 restores fixed-pos container layer behavior to match what we had > prior to ajuma's CL r449448. However the existing code before r449448 was wrong > too. Fixed-pos elements would incorrectly escape scrolling in cc. It wasn't > obvious because it is only transient until main thread commits. i.e. Fixed-pos > child scrolled by a transformed parent looks lag behind on scroll. > > https://output.jsbin.com/qeqomet/ demonstrates the bug. Scrolling the scroller > vertically should never reveal a white box that is occluded by a fixed-pos > sibling. > > I'll upload a new patch set that uses CLM::ChildForSuperlayers() instead. Using CLM::ChildForSuperlayers() as fixed-pos container will probably not fix the bug demostrated by https://output.jsbin.com/qeqomet/ though, due to this hack: https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?rcl=d7... I don't know why cc skips a scrollable layer as fixed-pos container. ajuma@ do you remember why was it added? Feels like a workaround to old blink bugs.
On 2017/04/25 03:22:43, trchen wrote: > On 2017/04/25 03:18:22, trchen wrote: > > On 2017/04/24 15:44:22, chrishtr wrote: > > > > > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > > > File > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > > > > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1616: > > > child_transform_layer_ ? child_transform_layer_.get() > > > Add a comment explaining. > > > > Thanks for asking! I realized this isn't right when I tried to explain it. :) > > > > For element that is both fixed-pos container and scrollable, its fixed-pos > > descendants are positioned by the SCROLLED box. i.e. scroll along in-flow > > contents. Therefore we should use CLM::ChildForSuperlayers() as fixed-pos > > container layer instead. > > > > Patch set 1 restores fixed-pos container layer behavior to match what we had > > prior to ajuma's CL r449448. However the existing code before r449448 was > wrong > > too. Fixed-pos elements would incorrectly escape scrolling in cc. It wasn't > > obvious because it is only transient until main thread commits. i.e. Fixed-pos > > child scrolled by a transformed parent looks lag behind on scroll. > > > > https://output.jsbin.com/qeqomet/ demonstrates the bug. Scrolling the scroller > > vertically should never reveal a white box that is occluded by a fixed-pos > > sibling. > > > > I'll upload a new patch set that uses CLM::ChildForSuperlayers() instead. > > Using CLM::ChildForSuperlayers() as fixed-pos container will probably not fix > the bug demostrated by https://output.jsbin.com/qeqomet/ though, due to this > hack: > https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?rcl=d7... > I don't know why cc skips a scrollable layer as fixed-pos container. ajuma@ do > you remember why was it added? Feels like a workaround to old blink bugs. I added it here: https://codereview.chromium.org/1144993004 Reading that CL's description, it sounds like I just misunderstood the way a scrollable fixed-pos container is supposed to work (possibly as a result of trying to match whatever the old CalcDrawProps was doing).
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/25 13:25:33, ajuma wrote: > On 2017/04/25 03:22:43, trchen wrote: > > On 2017/04/25 03:18:22, trchen wrote: > > > On 2017/04/24 15:44:22, chrishtr wrote: > > > > > > > > > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > > > > File > > > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2833233002/diff/1/third_party/WebKit/Source/c... > > > > > > > > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1616: > > > > child_transform_layer_ ? child_transform_layer_.get() > > > > Add a comment explaining. > > > > > > Thanks for asking! I realized this isn't right when I tried to explain it. > :) > > > > > > For element that is both fixed-pos container and scrollable, its fixed-pos > > > descendants are positioned by the SCROLLED box. i.e. scroll along in-flow > > > contents. Therefore we should use CLM::ChildForSuperlayers() as fixed-pos > > > container layer instead. > > > > > > Patch set 1 restores fixed-pos container layer behavior to match what we had > > > prior to ajuma's CL r449448. However the existing code before r449448 was > > wrong > > > too. Fixed-pos elements would incorrectly escape scrolling in cc. It wasn't > > > obvious because it is only transient until main thread commits. i.e. > Fixed-pos > > > child scrolled by a transformed parent looks lag behind on scroll. > > > > > > https://output.jsbin.com/qeqomet/ demonstrates the bug. Scrolling the > scroller > > > vertically should never reveal a white box that is occluded by a fixed-pos > > > sibling. > > > > > > I'll upload a new patch set that uses CLM::ChildForSuperlayers() instead. > > > > Using CLM::ChildForSuperlayers() as fixed-pos container will probably not fix > > the bug demostrated by https://output.jsbin.com/qeqomet/ though, due to this > > hack: > > > https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?rcl=d7... > > I don't know why cc skips a scrollable layer as fixed-pos container. ajuma@ do > > you remember why was it added? Feels like a workaround to old blink bugs. > > I added it here: https://codereview.chromium.org/1144993004 > > Reading that CL's description, it sounds like I just misunderstood the way a > scrollable fixed-pos container is supposed to work (possibly as a result of > trying to match whatever the old CalcDrawProps was doing). I updated this CL to make fixed-pos container applies to all graphics layers that can potentially own children. As expected, it doesn't fix https://output.jsbin.com/qeqomet/ due to the hack. I worry it may have something to do with Frame/LayoutView scrolling +- RLS. Say, fixed-pos element contained by viewport may start moving with the hack removed. As we don't have much integration test coverage in this area, I'll try to remove the hack in a separate CL and manually test it.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493165359637620,
"parent_rev": "b103c2ee064e10d11adcd883855fcd5a72de1d31", "commit_rev":
"25eaaeb522126f998e9bd83056f96a5ce3cc78c2"}
Message was sent while issue was closed.
Description was changed from ========== [blink] Perspective layer should be considered fixed-pos container layer This CL fixes a bug that fixed-position descendants skip its containing block's perspective transform. Perspective layer (CompositedLayerMapping:: child_transform_layer_) should be considered fixed-pos container if present. BUG=714001 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [blink] Perspective layer should be considered fixed-pos container layer This CL fixes a bug that fixed-position descendants skip its containing block's perspective transform. Perspective layer (CompositedLayerMapping:: child_transform_layer_) should be considered fixed-pos container if present. BUG=714001 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2833233002 Cr-Commit-Position: refs/heads/master@{#467175} Committed: https://chromium.googlesource.com/chromium/src/+/25eaaeb522126f998e9bd83056f9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/25eaaeb522126f998e9bd83056f9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
