Skip to content

GitLab

T
tangled
Commit 19916d29 authored by David Bokan's avatar David Bokan Committed by Commit Bot
Browse files
Options

[Reland] Make scroll bounds include subpixel accumulation 

This relands r532919 which was reverted in r533667

The original patch worked only in the initial case but the corrected
layer bounds were clobbered in
ScrollingCoordinator::ScrollableAreaScrollLayerDidChange. Even though
CompositedLayerMapping::UpdateScrollingLayerGeometry is always called
after this method, in ScrollingCoordinator the bounds are set directly
on the WebLayer. In CLM we set the bounds on a GraphicsLayer which
caches the size itself. It'll only update the WebLayer bounds if the
new size differs from the cached size. Thus, even though the bounds
were changed in ScrollingCoordinator, CLM would early out since it
the GraphicsLayer size hasn't changed.

This CL applies the same fix in ScrollingCoordinator and also adds a
new test that covers the linked bug more end-to-end.

The original patch is uploaded in patchset 1 for easy diff'ing.

Bug: 801381
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I28f7088ef6b429875c6c984c45fd1c53cf813c02
Reviewed-on: https://chromium-review.googlesource.com/897962Reviewed-by: default avatarTien-Ren Chen <trchen@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533913}
parent 3560431c
Hide whitespace changes
Inline Side-by-side
Showing with 175 additions and 3 deletions
third_party/WebKit/LayoutTests/fast/compositor-wheel-scroll-latching/subpixel-accumulation.html 0 → 100644
View file @ 19916d29
<!DOCTYPE html>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
let t;
if (window.internals && chrome && chrome.gpuBenchmarking) {
window.internals.settings.setPreferCompositingToLCDTextEnabled(true);
window.internals.settings.setScrollAnimatorEnabled(false);
t = async_test("Vertical scroll shouldn't latch to horizontal scroller.");
}
// Helper async function to block execution for n number of rAFs.
async function nFrames(n) {
return new Promise(resolve => {
let remainingFrames = n;
let func = function() {
--remainingFrames;
if (remainingFrames === 0)
resolve();
else {
requestAnimationFrame(func);
}
};
if (n === 0) {
resolve();
} else {
requestAnimationFrame(() => {
func(resolve);
});
}
});
}
async function scroll(pixels_to_scroll, direction, start_x, start_y, speed_in_pixels_s) {
return new Promise((resolve, reject) => {
const WHEEL_SOURCE_TYPE = 2; // MOUSE_INPUT from synthetic_gesture_params.h
chrome.gpuBenchmarking.smoothScrollBy(pixels_to_scroll,
resolve,
start_x,
start_y,
WHEEL_SOURCE_TYPE,
direction,
speed_in_pixels_s);
});
}
async function runTest() {
if (!chrome.gpuBenchmarking)
return;
// Wait a bit to ensure we don't start sending scroll gestures to the
// compositor before the page layers have fully propagated to the CC active
// tree.
await nFrames(2);
// Scroll horizontally first. Ensure we scrolled the div since otherwise we
// may pass the test vacuously. See crbug.com/801381 for the case we're
// guarding against.
await scroll(10, 'right', 50, 50, 1000);
// Wait a bit since the scroll gesture resolves when the ScrollEnd is sent
// from the browser, not when it's processed by the renderer.
await nFrames(2);
t.step(() => {
// Approx because smoothScrollBy isn't perfectly accurate.
assert_approx_equals(
document.getElementById("scroller").scrollLeft, 10, 1,
"Sanity check, if this fails the test isn't in the correct state.");
});
// Now scroll vertically. The scroller shouldn't have any vertical scroll
// extent so the window should scroll.
await scroll(10, 'down', 50, 50, 1000);
await nFrames(2);
t.step(() => {
assert_approx_equals(window.scrollY, 10, 1);
});
t.done();
}
addEventListener('load', runTest);
</script>
<style>
::-webkit-scrollbar {
width: 0px;
height: 0px;
}
body {
margin: 0;
height: 2000px;
}
#scroller {
position: relative;
top: 0.5625px;
width: 200px;
height: 200.8125px;
overflow: auto;
}
#space {
width: 1000px;
background-color: coral;
height: 200.8125px;
}
</style>
<div id="scroller">
<div id="space"></div>
</div>
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
View file @ 19916d29
...@@ -474,7 +474,22 @@ bool ScrollingCoordinator::ScrollableAreaScrollLayerDidChange( ...@@ -474,7 +474,22 @@ bool ScrollingCoordinator::ScrollableAreaScrollLayerDidChange(
FloatPoint scroll_position(scrollable_area->ScrollOrigin() + FloatPoint scroll_position(scrollable_area->ScrollOrigin() +
scrollable_area->GetScrollOffset()); scrollable_area->GetScrollOffset());
web_layer->SetScrollPosition(scroll_position); web_layer->SetScrollPosition(scroll_position);
web_layer->SetBounds(scrollable_area->ContentsSize()); // TODO(bokan): This method shouldn't be resizing the layer geometry. That
// happens in CompositedLayerMapping::UpdateScrollingLayerGeometry.
LayoutSize subpixel_accumulation =
scrollable_area->Layer()
? scrollable_area->Layer()->SubpixelAccumulation()
: LayoutSize();
LayoutSize contents_size =
scrollable_area->GetLayoutBox()
? LayoutSize(scrollable_area->GetLayoutBox()->ScrollWidth(),
scrollable_area->GetLayoutBox()->ScrollHeight())
: LayoutSize(scrollable_area->ContentsSize());
IntSize scroll_contents_size =
PixelSnappedIntRect(
LayoutRect(LayoutPoint(subpixel_accumulation), contents_size))
.Size();
web_layer->SetBounds(scroll_contents_size);
// VisualViewport scrolling may involve pinch zoom and gets routed through // VisualViewport scrolling may involve pinch zoom and gets routed through
// WebViewImpl explicitly rather than via ScrollingCoordinator::DidScroll // WebViewImpl explicitly rather than via ScrollingCoordinator::DidScroll
// since it needs to be set in tandem with the page scale delta. // since it needs to be set in tandem with the page scale delta.
......
third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
View file @ 19916d29
...@@ -1651,8 +1651,13 @@ void CompositedLayerMapping::UpdateScrollingLayerGeometry( ...@@ -1651,8 +1651,13 @@ void CompositedLayerMapping::UpdateScrollingLayerGeometry(
bool overflow_clip_rect_offset_changed = bool overflow_clip_rect_offset_changed =
old_scrolling_layer_offset != scrolling_layer_->OffsetFromLayoutObject(); old_scrolling_layer_offset != scrolling_layer_->OffsetFromLayoutObject();
IntSize scroll_size(layout_box.PixelSnappedScrollWidth(), IntSize scroll_size =
layout_box.PixelSnappedScrollHeight()); PixelSnappedIntRect(
LayoutRect(
LayoutPoint(owning_layer_.SubpixelAccumulation()),
LayoutSize(layout_box.ScrollWidth(), layout_box.ScrollHeight())))
.Size();
if (overflow_clip_rect_offset_changed) if (overflow_clip_rect_offset_changed)
scrolling_contents_layer_->SetNeedsDisplay(); scrolling_contents_layer_->SetNeedsDisplay();
......
third_party/WebKit/Source/core/paint/compositing/CompositedLayerMappingTest.cpp
View file @ 19916d29
...@@ -2475,4 +2475,47 @@ TEST_P(CompositedLayerMappingTest, ForegroundLayerSizing) { ...@@ -2475,4 +2475,47 @@ TEST_P(CompositedLayerMappingTest, ForegroundLayerSizing) {
EXPECT_EQ(FloatSize(100, 100), mapping->ForegroundLayer()->Size()); EXPECT_EQ(FloatSize(100, 100), mapping->ForegroundLayer()->Size());
} }
TEST_P(CompositedLayerMappingTest, ScrollLayerSizingSubpixelAccumulation) {
// This test verifies that when subpixel accumulation causes snapping it
// applies to both the scrolling and scrolling contents layers. Verify that
// the mapping doesn't have any vertical scrolling introduced as a result of
// the snapping behavior. https://crbug.com/801381.
GetDocument().GetFrame()->GetSettings()->SetPreferCompositingToLCDTextEnabled(
true);
// The values below are chosen so that the subpixel accumulation causes the
// pixel snapped height to be increased relative to snapping without it.
SetBodyInnerHTML(R"HTML(
<!DOCTYPE html>
<style>
body {
margin: 0;
}
#scroller {
position: relative;
top: 0.5625px;
width: 200px;
height: 200.8125px;
overflow: auto;
}
#space {
width: 1000px;
height: 200.8125px;
}
</style>
<div id="scroller">
<div id="space"></div>
</div>
)HTML");
GetDocument().View()->UpdateAllLifecyclePhases();
auto* mapping = ToLayoutBoxModelObject(GetLayoutObjectByElementId("scroller"))
->Layer()
->GetCompositedLayerMapping();
ASSERT_TRUE(mapping);
ASSERT_TRUE(mapping->ScrollingLayer());
ASSERT_TRUE(mapping->ScrollingContentsLayer());
EXPECT_EQ(mapping->ScrollingLayer()->Size().Height(),
mapping->ScrollingContentsLayer()->Size().Height());
}
} // namespace blink } // namespace blink
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment