Commit 2c6eb18b authored by David Bokan's avatar David Bokan Committed by Commit Bot

Overlay scrollbars work correctly without overflow

Overlay scrollbars - the type that fade in/out, not overflow: overlay -
should only ever be visible when the content is scrolling. Therefore, if
the content cannot scroll, we should never see an overlay scrollbar.
This is different from classic (i.e. non-overlay) scrollbars. An
overflow:scroll scroller requires showing a (disabled) scrollbar even
when there is no overflow. Today, when an overlay scrollbar is created
we check after-the-fact in UpdateAfterLayout whether there is overflow
and, if there isn't, we remove overlay scrollbars. This is how the above
mentioned behavior for |overflow:scroll| overlays is implemented.

However, this doesn't account for different kinds of updates.
UpdateAfterStyleChange must also recompute scrollbar states and didn't
have this no-overflow removal logic. If content caused a style update
that didn't require a layout, an overlay scrollbar would be attached in
this case. In the attached bug, the scrollbar is visible because the
fade out (on the main thread) is controlled by a timer that causes
scrollbars to be disabled (which causes them not to paint). In addition
to incorrectly creating the scrollbar, UpdateAfterStyleChange does not
compute the scrollbar enabled state so it defaults to true. Without
being able to scroll, the fade out timer is never started.

This CL fixes the issue by moving the overflow requirement for overlay
scrollbars into ComputeScrollbarExistence so that we account for it no
matter the point at which we recompute scrollbar existence. We also
refactor ComputeScrollbarExistence to make clear the distinction between
overflow dependent and independent computations.

A considered alternative was to always create the scrollbar but ensure
its enabled state is correctly set. This would have unified the
lifetimes of overlay and non-overlay scrollbars. However, it requires we
prevent creating composited layers for scrollbars that are overlay but
have no overflow since the compositor fades all of a scroller's
scrollbars together so such scrollbars would become visible if the other
axis is scrollable. We couldn't use the enabled state for this because
it is flipped every time the scrollbars fade in/out so that would cause
layers to constanty be created/destroyed so we'd have to introduce more
complexity to compositing decisions.

This change caused three tests to break (note: unit tests generally use
mock overlay scrollbars):

RepaintScrollableAreaLayersInMainThreadScrolling:

  A style change like this does not require invalidating paint on
  overlay scrollbars' compositing layers since the compositor can deal
  with resizes on its own.  However, this was causing an invalidation
  because of the bug fixed here: the style change adds a horizontal
  scrollbar and the layout removes it. Because it isn't composited, it
  causes the whole PaintLayer to be invalidated[1]. This causes the
  scrollbar layer to also need a repaint[2]. Since we no longer cause
  this creation and destruction of the horizontal scrollbar, we don't
  get this invalidation.

  This test now resizes the overflow so that the vertical scrollbar
  needs to be disabled which does cause an invalidation.

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc?l=2845&rcl=33a9a3956034a765124f034a8f7d86f19c9c17e6
[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc?l=2099&rcl=01e24ce806680bf99abf117e77d30d9566c72121

TraverseNonCompositingDescendantsInPaintOrder:

  The source of the failure here was that the |overflow:scroll| div no
  longer has overlay scrollbars after its style is updated (but before
  it does layout). When it performs PaintLayer::UpdateSelfPaintingLayer
  it will no longer be self painting[3]. Since it no longer has a self
  painting layer it will be added to the parent's visual overflow. This
  larger rect means that during overlap testing, the scroller's sibling
  (#stacked-child-of-composited-non-stacking-context) and its parent
  will overlap and so the sibling will require compositing. Finally,
  because it is now composited, it is now a paint invalidation container
  and so it won't be invalidated from the container element[5].

  The fix here is to simply add overflow to the scrolling div so that it
  continues to cause a self painting layer in [3].

[3] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_layer.cc?l=3054&rcl=fa18fef1b31a878fed89e2a543c3be0af2f57623
[4] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_box.cc?l=5780&rcl=0650ec2415a0f2583db48d11c6d8b6a31ff448c4
[5] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/object_paint_invalidator.cc?l=70&rcl=d9d7df43f1718c900afc470371b382c8dca085b6

mock-scrollbars.html:

  This was failing only on the legacy LayoutNG disabled bot so I didn't
  dig too deeply into it. The failure here was that the inner box would
  layout as if the parent had non-overlay scrollbars. I believe this is
  because the mock scrollbars were being enabled at load, after layout
  was already completed. I think this used to work because the scrollbar
  was being added and then removed which would cause a layout
  invalidation which no longer happens, thus we need to ensure mock
  scrollbars are enabled before we produce the first layout.

Bug: 1039501
Change-Id: Ic68522a4fdba8ffaa2c5ef508736c9a94eac7682
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2119933
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754808}
parent b56d074f
......@@ -568,6 +568,45 @@ TEST_F(ScrollbarsTest, OverlayScrollbarChangeToDisplayNoneDynamically) {
EXPECT_TRUE(scrollable_root->HorizontalScrollbar()->FrameRect().IsEmpty());
}
// Ensure that overlay scrollbars are not created, even in overflow:scroll,
// situations when there's no overflow. Specifically, after style-only changes.
TEST_F(ScrollbarsTest, OverlayScrolblarNotCreatedInUnscrollableAxis) {
// This test is specifically checking the behavior when overlay scrollbars
// are enabled.
ENABLE_OVERLAY_SCROLLBARS(true);
WebView().MainFrameWidget()->Resize(WebSize(800, 600));
SimRequest request("https://example.com/test.html", "text/html");
LoadURL("https://example.com/test.html");
request.Complete(R"HTML(
<!DOCTYPE html>
<style>
#target {
width: 100px;
height: 100px;
overflow-y: scroll;
opacity: 0.5;
}
</style>
<div id="target"></div>
)HTML");
Compositor().BeginFrame();
auto* target = GetDocument().getElementById("target");
auto* scrollable_area = target->GetLayoutBox()->GetScrollableArea();
ASSERT_FALSE(scrollable_area->VerticalScrollbar());
ASSERT_FALSE(scrollable_area->HorizontalScrollbar());
// Mutate the opacity so that we cause a style-only change.
target->setAttribute(html_names::kStyleAttr, "opacity: 0.9");
Compositor().BeginFrame();
ASSERT_FALSE(scrollable_area->VerticalScrollbar());
ASSERT_FALSE(scrollable_area->HorizontalScrollbar());
}
TEST_F(ScrollbarsTest, scrollbarIsNotHandlingTouchpadScroll) {
WebView().MainFrameWidget()->Resize(WebSize(200, 200));
SimRequest request("https://example.com/test.html", "text/html");
......
......@@ -1775,7 +1775,7 @@ TEST_F(CompositedLayerMappingTest,
EXPECT_FALSE(mapping->NeedsRepaint(*vertical_scrollbar_layer));
GetDocument().getElementById("child")->setAttribute(html_names::kStyleAttr,
"height: 300px");
"height: 50px");
GetDocument().View()->UpdateAllLifecyclePhasesExceptPaint(
DocumentUpdateReason::kTest);
EXPECT_TRUE(mapping->NeedsRepaint(*vertical_scrollbar_layer));
......
......@@ -56,7 +56,9 @@ TEST_F(ObjectPaintInvalidatorTest,
style='position: relative'></div>
<div
id='non-stacked-layered-child-of-composited-non-stacking-context'
style='overflow: scroll'></div>
style='overflow: scroll'>
<div style="height:40px"></div>
</div>
</div>
</div>
)HTML");
......
......@@ -1129,13 +1129,6 @@ void PaintLayerScrollableArea::UpdateAfterLayout() {
UpdateScrollbarProportions();
}
if (!scrollbars_are_frozen && HasOverlayScrollbars()) {
if (!ScrollSize(kHorizontalScrollbar))
SetHasHorizontalScrollbar(false);
if (!ScrollSize(kVerticalScrollbar))
SetHasVerticalScrollbar(false);
}
ClampScrollOffsetAfterOverflowChange();
if (!scrollbars_are_frozen) {
......@@ -1304,9 +1297,8 @@ void PaintLayerScrollableArea::UpdateAfterStyleChange(
bool needs_horizontal_scrollbar;
bool needs_vertical_scrollbar;
// We add auto scrollbars only during layout to prevent spurious activations.
ComputeScrollbarExistence(needs_horizontal_scrollbar,
needs_vertical_scrollbar, kForbidAddingAutoBars);
needs_vertical_scrollbar, kOverflowIndependent);
UpdateResizerStyle(old_style);
......@@ -1558,45 +1550,89 @@ void PaintLayerScrollableArea::ComputeScrollbarExistence(
return;
}
needs_horizontal_scrollbar = GetLayoutBox()->ScrollsOverflowX();
needs_vertical_scrollbar = GetLayoutBox()->ScrollsOverflowY();
mojom::blink::ScrollbarMode h_mode = mojom::blink::ScrollbarMode::kAuto;
mojom::blink::ScrollbarMode v_mode = mojom::blink::ScrollbarMode::kAuto;
// Don't add auto scrollbars if the box contents aren't visible.
if (GetLayoutBox()->HasAutoHorizontalScrollbar()) {
if (option == kForbidAddingAutoBars)
needs_horizontal_scrollbar &= HasHorizontalScrollbar();
needs_horizontal_scrollbar &=
GetLayoutBox()->IsRooted() && HasHorizontalOverflow() &&
VisibleContentRect(kIncludeScrollbars).Height();
}
// First, determine what behavior the scrollbars say they should have.
{
if (auto* layout_view = DynamicTo<LayoutView>(GetLayoutBox())) {
// LayoutView is special as there's various quirks and settings that
// style doesn't account for.
layout_view->CalculateScrollbarModes(h_mode, v_mode);
} else {
auto overflow_x = GetLayoutBox()->StyleRef().OverflowX();
if (overflow_x == EOverflow::kScroll) {
h_mode = mojom::blink::ScrollbarMode::kAlwaysOn;
} else if (overflow_x == EOverflow::kHidden ||
overflow_x == EOverflow::kVisible) {
h_mode = mojom::blink::ScrollbarMode::kAlwaysOff;
}
if (GetLayoutBox()->HasAutoVerticalScrollbar()) {
if (option == kForbidAddingAutoBars)
needs_vertical_scrollbar &= HasVerticalScrollbar();
needs_vertical_scrollbar &= GetLayoutBox()->IsRooted() &&
HasVerticalOverflow() &&
VisibleContentRect(kIncludeScrollbars).Width();
auto overflow_y = GetLayoutBox()->StyleRef().OverflowY();
if (overflow_y == EOverflow::kScroll) {
v_mode = mojom::blink::ScrollbarMode::kAlwaysOn;
} else if (overflow_y == EOverflow::kHidden ||
overflow_y == EOverflow::kVisible) {
v_mode = mojom::blink::ScrollbarMode::kAlwaysOff;
}
}
// Since overlay scrollbars (the fade-in/out kind, not overflow: overlay)
// only appear when scrolling, we don't create them if there isn't overflow
// to scroll. Thus, overlay scrollbars can't be "always on". i.e.
// |overlay:scroll| behaves like |overlay:auto|.
bool has_custom_scrollbar_style =
ScrollbarStyleSource(*GetLayoutBox())
.StyleRef()
.HasPseudoElementStyle(kPseudoIdScrollbar);
bool will_be_overlay = GetPageScrollbarTheme().UsesOverlayScrollbars() &&
!has_custom_scrollbar_style;
if (will_be_overlay) {
if (h_mode == mojom::blink::ScrollbarMode::kAlwaysOn)
h_mode = mojom::blink::ScrollbarMode::kAuto;
if (v_mode == mojom::blink::ScrollbarMode::kAlwaysOn)
v_mode = mojom::blink::ScrollbarMode::kAuto;
}
}
if (auto* layout_view = DynamicTo<LayoutView>(GetLayoutBox())) {
mojom::blink::ScrollbarMode h_mode;
mojom::blink::ScrollbarMode v_mode;
layout_view->CalculateScrollbarModes(h_mode, v_mode);
// By default, don't make any changes.
needs_horizontal_scrollbar = HasHorizontalScrollbar();
needs_vertical_scrollbar = HasVerticalScrollbar();
// Look for the scrollbarModes and reset the needs Horizontal & vertical
// Scrollbar values based on scrollbarModes, as during force style change
// StyleResolver::styleForDocument returns documentStyle with no overflow
// values, due to which we are destroying the scrollbars that were already
// present.
// If the behavior doesn't depend on overflow or any other information, we
// can set it now.
{
if (h_mode == mojom::blink::ScrollbarMode::kAlwaysOn)
needs_horizontal_scrollbar = true;
else if (h_mode == mojom::blink::ScrollbarMode::kAlwaysOff)
needs_horizontal_scrollbar = false;
if (v_mode == mojom::blink::ScrollbarMode::kAlwaysOn)
needs_vertical_scrollbar = true;
else if (v_mode == mojom::blink::ScrollbarMode::kAlwaysOff)
needs_vertical_scrollbar = false;
}
// If this is being performed before layout, we want to only update scrollbar
// existence if its based on purely style based reasons.
if (option == kOverflowIndependent)
return;
// If we have clean layout, we can make a decision on any scrollbars that
// depend on overflow.
{
if (h_mode == mojom::blink::ScrollbarMode::kAuto) {
// Don't add auto scrollbars if the box contents aren't visible.
needs_horizontal_scrollbar =
GetLayoutBox()->IsRooted() && HasHorizontalOverflow() &&
VisibleContentRect(kIncludeScrollbars).Height();
}
if (v_mode == mojom::blink::ScrollbarMode::kAuto) {
needs_vertical_scrollbar = GetLayoutBox()->IsRooted() &&
HasVerticalOverflow() &&
VisibleContentRect(kIncludeScrollbars).Width();
}
}
}
bool PaintLayerScrollableArea::TryRemovingAutoScrollbars(
......
......@@ -619,11 +619,21 @@ class CORE_EXPORT PaintLayerScrollableArea final
int HorizontalScrollbarStart() const;
IntSize ScrollbarOffset(const Scrollbar&) const;
enum ComputeScrollbarExistenceOption { kDefault, kForbidAddingAutoBars };
// If OverflowIndependent is specified, will only change current scrollbar
// existence if the new style doesn't depend on overflow which requires
// layout to be clean. It'd be nice if we could always determine existence at
// one point, after layout. Unfortunately, it seems that parts of layout are
// dependent on scrollbar existence in cases like |overflow:scroll|, removing
// the post style pass causes breaks in tests e.g. forms web_tests. Thus, we
// must do two scrollbar existence passes.
enum ComputeScrollbarExistenceOption {
kDependsOnOverflow,
kOverflowIndependent
};
void ComputeScrollbarExistence(
bool& needs_horizontal_scrollbar,
bool& needs_vertical_scrollbar,
ComputeScrollbarExistenceOption = kDefault) const;
ComputeScrollbarExistenceOption = kDependsOnOverflow) const;
// If the content fits entirely in the area without auto scrollbars, returns
// true to try to remove them. This is a heuristic and can be incorrect if the
......
<!DOCTYPE html>
<script>
if (window.internals)
internals.useMockOverlayScrollbars();
onload = ()=> {
child.style.display = "block";
}
</script>
<style>body { overflow:hidden; }</style>
<p>The scrollable container below should not appear to be scrollable.</p>
<div id="child" style="display:none; overflow:scroll; width:200px; height:200px;">
<div style="box-sizing:border-box; border:solid; width:100%; height:100%;"></div>
</div>
<script>
onload = ()=> {
if (!window.internals)
return;
internals.useMockOverlayScrollbars();
child.style.display = "block";
}
</script>
<!DOCTYPE html>
<script>
if (window.internals)
internals.useMockOverlayScrollbars();
</script>
<style>body { overflow:hidden; }</style>
<p>The scrollable container below should not appear to be scrollable.</p>
<div style="overflow:scroll; width:200px; height:200px;">
<div style="box-sizing:border-box; border:solid; width:100%; height:100%;"></div>
</div>
<script>
onload = ()=> {
if (!window.internals)
return;
internals.useMockOverlayScrollbars();
}
</script>
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