New issue
Advanced search Search tips

Issue 693741 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 667946



Sign in to add a comment

Implement CAP compositing reasons for root scroller

Project Member Reported by pdr@chromium.org, Feb 17 2017

Issue description

We need to implement the compositing reasons for scroll offset transforms in SPV2, in PaintPropertyTreeBuilder. This will largely mirror the SPV1 PaintLayerScrollableArea::computeNeedsCompositedScrolling code.
 

Comment 1 by pdr@chromium.org, Mar 16 2018

Status: Started (was: Assigned)
When we enabled root layer scrolling, the layout view is no longer composited scrolled because of this bug.
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d

commit fb079780798a45f84b3fb9c0f1ba02e8fb800f3d
Author: Philip Rogers <pdr@chromium.org>
Date: Wed Mar 21 02:41:51 2018

[SPV2] Set compositing reasons for the root scroller

This patch extracts out the existing root scroll compositing logic into
CompositingReasonFinder::RequiresCompositingForRootScroller and uses it
for SPV2. This fixes a bug where the document was not scrollable with
the mousewheel when --enable-slimming-paint-v2 was used.

This layout test now passes:
background-paint-into-scrolling-contents-layer-with-root-layer-scrolls-offset.html

Although there are spv2 differences, these look to be newly passing:
paint/invalidation/scroll/outline-change-scrollable.html
paint/invalidation/outline/outline-change-vertical-rl.html

This layout test now shows a failure:
paint/invalidation/reflection/scroll-absolute-layer-with-reflection.html

These tests had minor pixel differences:
fast/css/sticky/sticky-both-sides-top-left-constrained.html
fast/dom/scroll-reveal-top-overflow.html
fast/css/sticky/sticky-overflowing.html
fast/overflow/overflow_hidden.html
fast/css/sticky/sticky-vertically-overconstrained.html
fast/dom/scroll-reveal-left-overflow.html
fast/writing-mode/border-radius-clipping-vertical-lr.html

Bug: 693741
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I467008ac569ce1dd998caba7689c50b91d3687e4
Reviewed-on: https://chromium-review.googlesource.com/968983
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544624}
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[add] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/fast/dom/scroll-reveal-left-overflow-expected.png
[add] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/fast/dom/scroll-reveal-top-overflow-expected.png
[add] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/fast/overflow/overflow_hidden-expected.png
[add] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/fast/overflow/overflow_hidden-expected.txt
[add] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/fast/writing-mode/border-radius-clipping-vertical-lr-expected.png
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/outline/outline-change-vertical-rl-expected.txt
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/scroll/outline-change-scrollable-expected.txt
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/Source/core/paint/compositing/CompositingReasonFinder.cpp
[modify] https://crrev.com/fb079780798a45f84b3fb9c0f1ba02e8fb800f3d/third_party/WebKit/Source/core/paint/compositing/CompositingReasonFinder.h

In https://chromium-review.googlesource.com/c/chromium/src/+/1156002 we discovered two SPV2 scroll-related deficiencies:
1) The scroll hit test layer will prevent the scrolling contents from squashing into the background.
2) There is no logic for drawing solid-color backgrounds into the scrolling contents (aka with the scrolling contents properties). Because this does not exist, the background and foreground are in different layers which results in subpixel differences.

These need to be addressed before we can add compositing reasons for the LayoutView.
Cc: wangxianzhu@chromium.org
I believe 2) in #c3 has been fixed. What's the plan for scroll hit test layer?
Since that comment was written, we have launched PaintTouchActionRects which paints hit test rects with the correct property tree states. I recommend we move the implementation of BlockPainter::PaintScrollHitTestDisplayItem into BoxPainter::RecordHitTestData, and make CompositeAfterPaint depend on PaintTouchActionRects.
Cc: atotic@chromium.org
 Issue 922419  has been merged into this issue.
Can LayoutNG launch before this bug is fixed?

We have a failing webkit_unit_test:
All/FrameThrottlingTest.ThrottleSubtreeAtomically/2

According to @pdr, root cause of failure is this bug. We'd like to disable the test for NG if it is not a launch blocker.
@atotic, I wonder if this was actually fixed. I just landed a fix to the same crash you were hitting. Can you see if the crash still occurs?

Originally I thought the NG failure was due to this TODO:
static bool NeedsScrollNode(const LayoutObject& object) {
  ...
  // TODO(pdr): CAP has invalidation issues ( crbug.com/732611 ) as well as
  // subpixel issues (crbug.com/693741) which prevent us from compositing the
  // root scroller.
  if (RuntimeEnabledFeatures::CompositeAfterPaintEnabled())
    return HasScrollsOverflow(box);
  return HasScrollsOverflow(box) || IsRootScroller(box);
}

But then I found a fundamental issue with the scroll tree which has been fixed as of https://crrev.com/622886.
All/FrameThrottlingTest.ThrottleSubtreeAtomically/2 is still failing.
Ah ok :/ To answer your question: LayoutNG can launch without fixing this bug. CompositeAfterPaint is quite a ways out.

Comment 11 by chrishtr@chromium.org, Yesterday (29 hours ago)

Summary: Implement CAP compositing reasons for root scroller (was: Implement SPV2 compositing reasons for scrolling)

Comment 12 by chrishtr@chromium.org, Yesterday (29 hours ago)

Owner: ----
Status: Available (was: Started)

Sign in to add a comment