New issue
Advanced search Search tips

Issue 717222 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Overlay scrollbar did not fade out after the first appear

Project Member Reported by chaopeng@chromium.org, May 1 2017

Issue description

Chrome Version: git: da4b99838d45
OS: Linux

What steps will reproduce the problem?
(0) Enable overlay-scrollbars
(1) open a page has scrollbar

What is the expected result?

Scrollbar fade out after a while.

What happens instead?

Scrollbar does not fade out until a scroll update.
 
This issue is from 697a467f819ce09da5209a3df13b8b92f33e35a4
This issue is caused by Aura Overlay Scrollbar is created with opacity>0. We used to call LayerTreeImpl::UpdateScrollbars to fade out when initialization.

```
#2 0x7f23d0dfe1ea cc::LayerTreeImpl::UpdateScrollbars()
#3 0x7f23d0dfd999 cc::LayerTreeImpl::DidUpdateScrollState()
#4 0x7f23d0e07231 cc::LayerTreeImpl::RegisterScrollLayer()
```

But we remove ScrollbarAnimationController::DidScrollUpdate call in UpdateScrollbars at 697a467f819ce09da5209a3df13b8b92f33e35a4. Then Overlay Scrollbar can not fade out until we have real scroll.
Project Member

Comment 3 by bugdroid1@chromium.org, May 4 2017

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

commit b14926c905396fbeb0a77bfc6cce569c8a35fa5f
Author: chaopeng <chaopeng@chromium.org>
Date: Thu May 04 19:36:19 2017

Fade out overlay scrollbar after page load

This issue is caused by Aura Overlay Scrollbar is created with
opacity>0. We
used to call SAC::DidScrollUpdate - LTI::UpdateScrollbars to fade out
when RegisterScrollLayer.

```
#2 0x7f23d0dfe1ea cc::LayerTreeImpl::UpdateScrollbars()
#3 0x7f23d0dfd999 cc::LayerTreeImpl::DidUpdateScrollState()
#4 0x7f23d0e07231 cc::LayerTreeImpl::RegisterScrollLayer()
```

But we remove ScrollbarAnimationController::DidScrollUpdate call in
UpdateScrollbars at
https://crrev.com/697a467f819ce09da5209a3df13b8b92f33e35a4.
Then Overlay Scrollbar can not fade out until we have real scroll.

In this patch, we apply set_needs_show_scrollbars(true) to the layer in
LayerTreeImpl::RegisterScrollLayer that will let
LTI::HandleScrollbarShowRequestsFromMain pickup and call
SAC::DidScrollUpdate.

For cc_unittest, remove set_needs_show_scrollbars call in
LayerTreeHostImplTestScrollbarAnimation.* and
LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit.

BUG= 717222 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2858973002
Cr-Commit-Position: refs/heads/master@{#469425}

[modify] https://crrev.com/b14926c905396fbeb0a77bfc6cce569c8a35fa5f/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/b14926c905396fbeb0a77bfc6cce569c8a35fa5f/cc/trees/layer_tree_impl.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 5 2017

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

commit 9320fa40f77c26a054be97c9e2c7543e277c3081
Author: chaopeng <chaopeng@chromium.org>
Date: Fri May 05 20:07:03 2017

Revert of Fade out overlay scrollbar after page load (patchset #1 id:1 of https://codereview.chromium.org/2858973002/ )

Reason for revert:
Revert for regression

Details: https://bugs.chromium.org/p/chromium/issues/detail?id=718863

Original issue's description:
> Fade out overlay scrollbar after page load
>
> This issue is caused by Aura Overlay Scrollbar is created with
> opacity>0. We
> used to call SAC::DidScrollUpdate - LTI::UpdateScrollbars to fade out
> when RegisterScrollLayer.
>
>
> ```
> #2 0x7f23d0dfe1ea cc::LayerTreeImpl::UpdateScrollbars()
> #3 0x7f23d0dfd999 cc::LayerTreeImpl::DidUpdateScrollState()
> #4 0x7f23d0e07231 cc::LayerTreeImpl::RegisterScrollLayer()
> ```
>
> But we remove ScrollbarAnimationController::DidScrollUpdate call in
> UpdateScrollbars at
> https://crrev.com/697a467f819ce09da5209a3df13b8b92f33e35a4.
> Then Overlay Scrollbar can not fade out until we have real scroll.
>
> In this patch, we apply set_needs_show_scrollbars(true) to the layer in
> LayerTreeImpl::RegisterScrollLayer that will let
> LTI::HandleScrollbarShowRequestsFromMain pickup and call
> SAC::DidScrollUpdate.
>
> For cc_unittest, remove set_needs_show_scrollbars call in
> LayerTreeHostImplTestScrollbarAnimation.* and
> LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit.
>
> BUG= 717222 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
>
> Review-Url: https://codereview.chromium.org/2858973002
> Cr-Commit-Position: refs/heads/master@{#469425}
> Committed: https://chromium.googlesource.com/chromium/src/+/b14926c905396fbeb0a77bfc6cce569c8a35fa5f

TBR=weiliangc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 717222 

Review-Url: https://codereview.chromium.org/2863153002
Cr-Commit-Position: refs/heads/master@{#469763}

[modify] https://crrev.com/9320fa40f77c26a054be97c9e2c7543e277c3081/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/9320fa40f77c26a054be97c9e2c7543e277c3081/cc/trees/layer_tree_impl.cc

Comment 5 by bokan@chromium.org, May 9 2017

The patch skobes@ landed had that as an intended change. We can't rely on RegisterScrollLayer/Scrollbars since that might happen before the scroll bounds are pushed. Instead, we should be relying on the main thread telling the compositor to show the scrollbars. In this case, why isn't the call to ShowOverlayScrollbars in FrameView::ViewportSizeChanged enough?
Project Member

Comment 6 by bugdroid1@chromium.org, May 10 2017

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

commit 4180d24ef895be388449653c9fa14b105cd49a45
Author: chaopeng <chaopeng@chromium.org>
Date: Wed May 10 20:38:57 2017

Reland Fade out overlay scrollbar after page load

This issue is caused by Aura Overlay Scrollbar is created with
opacity>0. We
used to call SAC::DidScrollUpdate - LTI::UpdateScrollbars to fade out
when RegisterScrollLayer.

```
#2 0x7f23d0dfe1ea cc::LayerTreeImpl::UpdateScrollbars()
#3 0x7f23d0dfd999 cc::LayerTreeImpl::DidUpdateScrollState()
#4 0x7f23d0e07231 cc::LayerTreeImpl::RegisterScrollLayer()
```

But we remove ScrollbarAnimationController::DidScrollUpdate call in
UpdateScrollbars at
https://crrev.com/697a467f819ce09da5209a3df13b8b92f33e35a4.
Then Overlay Scrollbar can not fade out until we have real scroll.

In this patch, we apply set_needs_show_scrollbars(true) to the layer in
LayerTreeImpl::RegisterScrollLayer for Aura that will let
LTI::HandleScrollbarShowRequestsFromMain pickup and call
SAC::DidScrollUpdate.

For cc_unittest, remove set_needs_show_scrollbars call in
LayerTreeHostImplTestScrollbarAnimation.* and
LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit.

BUG= 717222 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2868553002
Cr-Commit-Position: refs/heads/master@{#470695}

[modify] https://crrev.com/4180d24ef895be388449653c9fa14b105cd49a45/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/4180d24ef895be388449653c9fa14b105cd49a45/cc/trees/layer_tree_impl.cc

Status: Fixed (was: Assigned)

Sign in to add a comment