GitLab

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: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754808}