Fix event targeting for overlay scrollbar thumbs (in native UI).

Fix event targeting for overlay scrollbar thumbs (in native UI).

Views with layers are targeted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798
Committed: https://crrev.com/03722d39ae28885ebd6de91429325776c77da2a7
Cr-Commit-Position: refs/heads/master@{#433664}

[Evan Stade, 2016-11-18 19:06:54]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2016-11-18 19:07:40]

Description was changed from

==========
Fix event targetting for overlay scrollbar thumbs (in native UI).

Views with layers are targetted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798
==========

to

==========
Fix event targeting for overlay scrollbar thumbs (in native UI).

Views with layers are targeted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798
==========

[Evan Stade, 2016-11-18 19:07:40]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org

[commit-bot: I haz the power, 2016-11-18 19:08:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-11-18 20:27:40]

varkha@chromium.org changed reviewers:
+ varkha@chromium.org

[varkha, 2016-11-18 20:27:41]

There is some artifact painted when I try this (near the bottom left edge) when
the scrollbar thumb is painted narrow. Is that due to this patch?

[commit-bot: I haz the power, 2016-11-18 20:56:27]

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

[commit-bot: I haz the power, 2016-11-18 20:56:28]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-11-19 01:43:04]

https://codereview.chromium.org/2520433003/diff/1/ui/views/controls/scrollbar...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2520433003/diff/1/ui/views/controls/scrollbar...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:45: kThumbThickness +
kThumbHoverOffset);
This would affect layout (and thus painting) too, right? Can that be the cause
of the visual glitch varkha@ mentioned in the earlier comment?

[Evan Stade, 2016-11-19 02:23:34]

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:31: // |scroll_bar| isn't done
being constructed; it's not safe to do anything
different bug here --- don't do work in constructors/"this in initializer list",
learned the hard way: IsHorizontal() was returning 16 (haha) because scroll_bar
hadn't yet been constructed; this meant our first call to OnStateChanged() was
using the wrong translation.

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:39:
layer()->SetFillsBoundsOpaquely(false);
this is the fix for the graphical glitch Valery pointed out.

[varkha, 2016-11-19 02:58:01]

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:31: // |scroll_bar| isn't done
being constructed; it's not safe to do anything
On 2016/11/19 02:23:34, Evan Stade wrote:
> different bug here --- don't do work in constructors/"this in initializer
list",
> learned the hard way: IsHorizontal() was returning 16 (haha) because
scroll_bar
> hadn't yet been constructed; this meant our first call to OnStateChanged() was
> using the wrong translation.

Is this responsible for the first Thumb's paint being in the wrong spot?

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:39:
layer()->SetFillsBoundsOpaquely(false);
On 2016/11/19 02:23:34, Evan Stade wrote:
> this is the fix for the graphical glitch Valery pointed out.

Yay! Glad you caught it.

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:113:
static_cast<Thumb*>(GetThumb())->Init();
Is it possible to make a virtual BaseScrollBarThumb::Init, call it in
BaseScrollBar ctor and override Init in OverlayScrollBar::Thumb? Not sure if
this would be strictly simpler but should eliminate the need for cast.

[Evan Stade, 2016-11-21 15:26:21]

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:31: // |scroll_bar| isn't done
being constructed; it's not safe to do anything
On 2016/11/19 02:58:00, varkha wrote:
> On 2016/11/19 02:23:34, Evan Stade wrote:
> > different bug here --- don't do work in constructors/"this in initializer
> list",
> > learned the hard way: IsHorizontal() was returning 16 (haha) because
> scroll_bar
> > hadn't yet been constructed; this meant our first call to OnStateChanged()
was
> > using the wrong translation.
> 
> Is this responsible for the first Thumb's paint being in the wrong spot?

yes

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:113:
static_cast<Thumb*>(GetThumb())->Init();
On 2016/11/19 02:58:00, varkha wrote:
> Is it possible to make a virtual BaseScrollBarThumb::Init, call it in
> BaseScrollBar ctor and override Init in OverlayScrollBar::Thumb? Not sure if
> this would be strictly simpler but should eliminate the need for cast.

"Avoid virtual method calls in constructors"

https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

[varkha, 2016-11-21 17:16:46]

LGTM with attempt to clarify the guideline.

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):

https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
ui/views/controls/scrollbar/overlay_scroll_bar.cc:113:
static_cast<Thumb*>(GetThumb())->Init();
On 2016/11/21 15:26:20, Evan Stade wrote:
> On 2016/11/19 02:58:00, varkha wrote:
> > Is it possible to make a virtual BaseScrollBarThumb::Init, call it in
> > BaseScrollBar ctor and override Init in OverlayScrollBar::Thumb? Not sure if
> > this would be strictly simpler but should eliminate the need for cast.
> 
> "Avoid virtual method calls in constructors"
> 
> https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

Fair enough, although I thought this was about calling Class::VirtualMethod()
from Class::Class(), not from SomeOtherClass::SomeOtherClass(). In this case the
Thumb would be fully constructed so there should be no harm.

[sadrul, 2016-11-21 17:25:56]

lgtm

[Evan Stade, 2016-11-21 20:40:44]

On 2016/11/21 17:16:46, varkha wrote:
> LGTM with attempt to clarify the guideline.
> 
>
https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
> File ui/views/controls/scrollbar/overlay_scroll_bar.cc (right):
> 
>
https://codereview.chromium.org/2520433003/diff/20001/ui/views/controls/scrol...
> ui/views/controls/scrollbar/overlay_scroll_bar.cc:113:
> static_cast<Thumb*>(GetThumb())->Init();
> On 2016/11/21 15:26:20, Evan Stade wrote:
> > On 2016/11/19 02:58:00, varkha wrote:
> > > Is it possible to make a virtual BaseScrollBarThumb::Init, call it in
> > > BaseScrollBar ctor and override Init in OverlayScrollBar::Thumb? Not sure
if
> > > this would be strictly simpler but should eliminate the need for cast.
> > 
> > "Avoid virtual method calls in constructors"
> > 
> > https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
> 
> Fair enough, although I thought this was about calling Class::VirtualMethod()
> from Class::Class(), not from SomeOtherClass::SomeOtherClass(). In this case
the
> Thumb would be fully constructed so there should be no harm.

I see what you mean. I still don't like adding excess virtual stuff though. I
think the root problem is this circular construction-time dependency, so I've
moved the thumb from a scrollbar ctor param to a setter.

[Evan Stade, 2016-11-21 20:40:55]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-21 20:40:55]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2520433003/#ps40001
(title: "remove circular construction dependency")

[commit-bot: I haz the power, 2016-11-21 20:41:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 22:00:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479760855372950,
"parent_rev": "bf8102aa52d76167698c5e607636c8df21d27205", "commit_rev":
"643d453b057a1318f2daea7cb9e5dcfdcc0bf734"}

[commit-bot: I haz the power, 2016-11-21 22:00:36]

Description was changed from

==========
Fix event targeting for overlay scrollbar thumbs (in native UI).

Views with layers are targeted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798
==========

to

==========
Fix event targeting for overlay scrollbar thumbs (in native UI).

Views with layers are targeted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798
==========

[commit-bot: I haz the power, 2016-11-21 22:00:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-21 22:08:19]

Description was changed from

==========
Fix event targeting for overlay scrollbar thumbs (in native UI).

Views with layers are targeted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798
==========

to

==========
Fix event targeting for overlay scrollbar thumbs (in native UI).

Views with layers are targeted based on their layer's transform rather
than their local coordinates. See View::GetTransform(). Hence the thumb
did not receive events until the translated area was hovered. To resolve
this, make the thumb 11 + 4 = 15 dp wide all the time, and just slide
back and forth so that 4dp is off the left or 4dp is off the right of
the 11dp track, making sure the thumb always covers the whole track
(it won't extend beyond visually or in terms of event handling because
of the track's layer clipping).

BUG=666798

Committed: https://crrev.com/03722d39ae28885ebd6de91429325776c77da2a7
Cr-Commit-Position: refs/heads/master@{#433664}
==========

[commit-bot: I haz the power, 2016-11-21 22:08:20]

Patchset 3 (id:??) landed as
https://crrev.com/03722d39ae28885ebd6de91429325776c77da2a7
Cr-Commit-Position: refs/heads/master@{#433664}