Hide scrollbars when their dimensions are not scrollable.

Hide scrollbars when their dimensions are not scrollable.

Make scrollbars disappear by setting their opacity to 0
(i.e. making them completely transparent) when they
cannot be scrolled.

BUG=370892

[timav, 2014-10-08 22:25:24]

timav@google.com changed reviewers:
+ aelias@chromium.org, bokan@chromium.org, timav@google.com

[timav, 2014-10-08 22:25:25]

This simple change seems to work. Please take a look, if it's ok I will add a
unit test.

[bokan, 2014-10-09 15:22:15]

https://codereview.chromium.org/640063002/diff/1/Source/core/frame/PinchViewp...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/640063002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.cpp:164: adjustScrollbarExistence();
Should we also call this in setSize?

https://codereview.chromium.org/640063002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.cpp:331: m_hasVerticalScrollbar =
visibleSize.height() < documentSize.height();
Rather than comparing the sizes yourself here, just check if
maximumScrollPosition.x/y > 0 for this and mainFrame. There's a bunch of
subtleties to do with top controls and floating point snapping that we should
account for (coming up in a patch of mine).

https://codereview.chromium.org/640063002/diff/1/Source/core/frame/PinchViewp...
Source/core/frame/PinchViewport.cpp:342:
m_innerViewportContainerLayer->setChildren(children);
Hmm...I'm not sure what the implications of frequently adding/removing these
layers might be. Is there no "enabled" or "visible" feature on scrollbar layers?
In any case, I'll let aelias@ comment.

[aelias_OOO_until_Jul13, 2014-10-09 18:20:19]

I don't think it's correct to remove these scrollbars on the Blink side.  They
need to appear when we pinch, which will happen first on the impl thread.  So
the way to fix this problem is to leave them here, but make a change in CC that
suppresses showing them.

[blink-reviews, 2014-10-10 02:41:06]

While trying to disable scrollbar by not calling

   scrollbar_animation_controller_->DidScrollUpdate(on_resize):

As far as I understand, DidScrollUpdate(), when called for one (vertical)
scrollbar will show all scrolbars.

On Thu, Oct 9, 2014 at 11:20 AM, <aelias@chromium.org> wrote:

> I don't think it's correct to remove these scrollbars on the Blink side.
> They
> need to appear when we pinch, which will happen first on the impl thread.
> So
> the way to fix this problem is to leave them here, but make a change in CC
> that
> suppresses showing them.
>
> https://codereview.chromium.org/640063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-10-10 02:51:01]

Right, I forgot that the scrollbar_animation_controller was attached to the
scroll layer, not the scrollbar layer.  I guess we could change
ScrollbarAnimationControllerLinearFade::ApplyOpacityToScrollbars to use a 0
opacity when the scroll layer's MaxScrollOffset is 0 on that axis or
user_scrollable_horizontal/vertical is false.

[timav, 2014-10-10 18:53:50]

On 2014/10/10 02:51:01, aelias wrote:
> Right, I forgot that the scrollbar_animation_controller was attached to the
> scroll layer, not the scrollbar layer.  I guess we could change
> ScrollbarAnimationControllerLinearFade::ApplyOpacityToScrollbars to use a 0
> opacity when the scroll layer's MaxScrollOffset is 0 on that axis or
> user_scrollable_horizontal/vertical is false.

That works!

The properties user_scrollable_horizontal_ and user_scrollable_vertical are the
properties of the LayerImpl object
to which ScrollbarAnimationController is attached, right? In other words, they
are meaningless to the ScrollbarLayerImplBase objects,
are they?

[aelias_OOO_until_Jul13, 2014-10-10 19:00:35]

On 2014/10/10 at 18:53:50, timav wrote:
> The properties user_scrollable_horizontal_ and user_scrollable_vertical are
the properties of the LayerImpl object
> to which ScrollbarAnimationController is attached, right? In other words, they
are meaningless to the ScrollbarLayerImplBase objects,
> are they?

Correct, they are properties of the scroll layer (the same layer where the
MaxScrollOffset is meaningful).  Note that those user_scrollable booleans are
set by Blink special conditions like overflow-x: hidden CSS properties that
disable scrolling, they're not automatically derived from any nonscrollability.

[timav, 2014-10-11 00:16:49]

Alex and David,

Please check whether this patch is what you meant. If it is, I'll write the unit
test which is still missing.

I have found only two places where non-scrollability is set (both in Blink by
setUserScrollable()) but it did not have any effect and was overridden.

[aelias_OOO_until_Jul13, 2014-10-13 18:30:03]

The approach looks fine.

https://codereview.chromium.org/640063002/diff/70001/cc/animation/scrollbar_a...
File cc/animation/scrollbar_animation_controller_linear_fade.cc (right):

https://codereview.chromium.org/640063002/diff/70001/cc/animation/scrollbar_a...
cc/animation/scrollbar_animation_controller_linear_fade.cc:66: bool visible =
scroll_layer_->user_scrollable(scrollbar->orientation()) &&
How about moving this logic to a method on ScrollbarLayerImplBase, called
scrollable()?

[timav, 2014-10-14 00:52:15]

On 2014/10/13 18:30:03, aelias wrote:
> The approach looks fine.
> 
>
https://codereview.chromium.org/640063002/diff/70001/cc/animation/scrollbar_a...
> File cc/animation/scrollbar_animation_controller_linear_fade.cc (right):
> 
>
https://codereview.chromium.org/640063002/diff/70001/cc/animation/scrollbar_a...
> cc/animation/scrollbar_animation_controller_linear_fade.cc:66: bool visible =
> scroll_layer_->user_scrollable(scrollbar->orientation()) &&
> How about moving this logic to a method on ScrollbarLayerImplBase, called
> scrollable()?

Please check Patch Set 3, I had to make this method virtual.

[aelias_OOO_until_Jul13, 2014-10-14 01:01:14]

Hmm, sorry, I wasn't thinking that there was already a base class method with
that name.  Making it virtual seems wrong as there's no intended polymorphic
behavior here, can we call it something else like is_axis_scrollable()?

[timav, 2014-10-14 01:51:24]

On 2014/10/14 01:01:14, aelias wrote:
> Hmm, sorry, I wasn't thinking that there was already a base class method with
> that name.  Making it virtual seems wrong as there's no intended polymorphic
> behavior here, can we call it something else like is_axis_scrollable()?

I used the name can_scroll_orientation() because I wanted something specific to
the
scrollbar, and scrollable_for_orientation() was too long. Could not come up with
anything better.

[aelias_OOO_until_Jul13, 2014-10-14 01:53:29]

OK, looks fine.  Just needs a test.

[timav, 2014-10-14 23:18:27]

On 2014/10/14 01:53:29, aelias wrote:
> OK, looks fine.  Just needs a test.

Added unit tests to animation controller tests. Also, the existing test broke
there, I had to fix it by making the scroll rectangle bigger than the clip
rectangle in the default case.

[aelias_OOO_until_Jul13, 2014-10-14 23:23:41]

https://codereview.chromium.org/640063002/diff/200001/cc/animation/scrollbar_...
File cc/animation/scrollbar_animation_controller_linear_fade_unittest.cc
(right):

https://codereview.chromium.org/640063002/diff/200001/cc/animation/scrollbar_...
cc/animation/scrollbar_animation_controller_linear_fade_unittest.cc:142:
base::TimeTicks time;
You're not doing anything with this time variable?  You need to call
scrollbar_controller_->Animate(time); for it to have any effect.  Please either
do that or remove it.

[timav, 2014-10-14 23:39:21]

On 2014/10/14 23:23:41, aelias wrote:
>
https://codereview.chromium.org/640063002/diff/200001/cc/animation/scrollbar_...
> File cc/animation/scrollbar_animation_controller_linear_fade_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/640063002/diff/200001/cc/animation/scrollbar_...
> cc/animation/scrollbar_animation_controller_linear_fade_unittest.cc:142:
> base::TimeTicks time;
> You're not doing anything with this time variable?  You need to call
> scrollbar_controller_->Animate(time); for it to have any effect.  Please
either
> do that or remove it.

Removed.

[aelias_OOO_until_Jul13, 2014-10-14 23:41:24]

lgtm, except please edit the patch description to match the way this patch does
it.

[timav, 2014-10-14 23:53:47]

The CQ bit was checked by timav@google.com

[commit-bot: I haz the power, 2014-10-14 23:54:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640063002/220001

[commit-bot: I haz the power, 2014-10-14 23:54:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-14 23:54:21]

Failed to apply patch for
cc/animation/scrollbar_animation_controller_linear_fade.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         cc
  Created missing directory cc.
  A         cc/animation
  Created missing directory cc/animation.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: cc/animation/scrollbar_animation_controller_linear_fade.cc
  |diff --git a/cc/animation/scrollbar_animation_controller_linear_fade.cc
b/cc/animation/scrollbar_animation_controller_linear_fade.cc
  |index
90437b3835b322c09649fe35d024d603d05bcc0b..f65fd727ac4fe50f9c8666ac3489a8426eb59b6a
100644
  |--- a/cc/animation/scrollbar_animation_controller_linear_fade.cc
  |+++ b/cc/animation/scrollbar_animation_controller_linear_fade.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       cc/animation/scrollbar_animation_controller_linear_fade.cc
Index: cc/animation/scrollbar_animation_controller_linear_fade.cc
diff --git a/cc/animation/scrollbar_animation_controller_linear_fade.cc
b/cc/animation/scrollbar_animation_controller_linear_fade.cc
index
90437b3835b322c09649fe35d024d603d05bcc0b..f65fd727ac4fe50f9c8666ac3489a8426eb59b6a
100644
--- a/cc/animation/scrollbar_animation_controller_linear_fade.cc
+++ b/cc/animation/scrollbar_animation_controller_linear_fade.cc
@@ -62,8 +62,9 @@ void
ScrollbarAnimationControllerLinearFade::ApplyOpacityToScrollbars(
        it != scrollbars->end();
        ++it) {
     ScrollbarLayerImplBase* scrollbar = *it;
+
     if (scrollbar->is_overlay_scrollbar())
-      scrollbar->SetOpacity(opacity);
+      scrollbar->SetOpacity(scrollbar->can_scroll_orientation() ? opacity : 0);
   }
 }

[bokan, 2014-11-19 01:01:00]

bokan@chromium.org changed reviewers:
- bokan@chromium.org