Re-add snap-back behavior (Blink side)

After Aura copied the original Linux scrollbar behavior, it never got updated
with Windows-specific native behaviors from ScrollbarThemeWin.cpp:

* Snap the scrollbar back to its drag origin when dragged too far off the track.
  This code matches the old code we had for years and also matches my Win 7
  native scrollbars pixel-for-pixel.

* Don't jump the scroll thumb to the mouse cursor on a middle-click, only on a
  shift-click.  Middle-click must be a GTK-ism.

This is the Blink side of the changes to re-add this behavior.

BUG=337919
TEST=Drag a scroll thumb to somewhere in the track, then without releasing, move the mouse far away from the track.  The scroll thumb should snap back to the scroll origin.
R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168177

[Peter Kasting, 2014-02-08 00:28:24]

I assumed that the correct way to add non-GTK behavior to
ScrollbarThemeGtkOrAura.cpp was to use #if !defined(TOOLKIT_GTK).  Let me know
if I should instead be creating some subclass of this class or something.

(I chose "not GTK" as the condition because AFAIK we want CrOS behavior to match
Windows behavior, and thus OS(WIN) wouldn't be the right #define to look at.)

[Dirk Pranke, 2014-02-08 01:21:21]

I am unqualified to weigh in here, and defer to sky or erg.

[sky, 2014-02-10 15:14:29]

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp:157: #if defined(TOOLKIT_GTK)
Don't you want !win here? Otherwise won't chromeos pick up this behavior?

[Peter Kasting, 2014-02-10 23:09:16]

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp:157: #if defined(TOOLKIT_GTK)
On 2014/02/10 15:14:30, sky wrote:
> Don't you want !win here? Otherwise won't chromeos pick up this behavior?

To re-quote my initial comment on this CL:

"(I chose "not GTK" as the condition because AFAIK we want CrOS behavior to
match Windows behavior, and thus OS(WIN) wouldn't be the right #define to look
at.)"

Am I correct that in general we want CrOS to be copying Windows behavior and not
GTK behavior?  Classically I've always tried to port Windows' small behavioral
details over to CrOS.

[sky, 2014-02-11 00:35:24]

I don't think you can make a general statement about the two. Look at
the scrollbar buttons for proof of that.

I suspect most of the UX folks do not want the snap back behavior on
cros. It's worth asking them though.

  -Scott

On Mon, Feb 10, 2014 at 3:09 PM,  <pkasting@chromium.org> wrote:
>
>
https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
> File Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp (right):
>
>
https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
> Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp:157: #if
> defined(TOOLKIT_GTK)
> On 2014/02/10 15:14:30, sky wrote:
>>
>> Don't you want !win here? Otherwise won't chromeos pick up this
>
> behavior?
>
> To re-quote my initial comment on this CL:
>
> "(I chose "not GTK" as the condition because AFAIK we want CrOS behavior
> to match Windows behavior, and thus OS(WIN) wouldn't be the right
> #define to look at.)"
>
> Am I correct that in general we want CrOS to be copying Windows behavior
> and not GTK behavior?  Classically I've always tried to port Windows'
> small behavioral details over to CrOS.
>
> https://codereview.chromium.org/151483008/

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

[Peter Kasting, 2014-02-11 18:16:12]

On 2014/02/11 00:35:24, sky wrote:
> I don't think you can make a general statement about the two. Look at
> the scrollbar buttons for proof of that.
> 
> I suspect most of the UX folks do not want the snap back behavior on
> cros. It's worth asking them though.

You should be CCed on the relevant internal thread, where it looks like Glen and
Alex are fine with this on CrOS.  So for now let's go ahead and do this as
"!GTK".

[Elliot Glaysher, 2014-02-11 18:36:56]

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp:148: #if defined(TOOLKIT_GTK)
You probably want defined(TOOLKIT_GTK) || (defined(OS_LINUX) &&
!defined(OS_CHROMEOS)) here.

(I'm fine experimenting with turning on the snapback behaviour on linux aura for
now, since qt does it even if gtk doesn't.)

[sky, 2014-02-11 18:52:59]

LGTM

[Peter Kasting, 2014-02-12 03:17:58]

I tried to make Elliot's suggested change, but got this presubmit warning:

Found use of Chromium OS_* macro in
Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp:148. Use WTF platform macros
instead.

I don't know what macros to use, and the warning doesn't tell me where to look. 
I tried to find some PLATFORM_ defines in wtf/, but failed.  :(

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/151483008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollbarThemeGtkOrAura.cpp:148: #if defined(TOOLKIT_GTK)
On 2014/02/11 18:36:56, Elliot Glaysher wrote:
> You probably want defined(TOOLKIT_GTK) || (defined(OS_LINUX) &&
> !defined(OS_CHROMEOS)) here.
> 
> (I'm fine experimenting with turning on the snapback behaviour on linux aura
for
> now, since qt does it even if gtk doesn't.)

Done.

[Peter Kasting, 2014-02-24 23:58:44]

Adam: This compiles and seems to work (on Windows at least) now, so if you could
review or send this to someone more appropriate, that'd be great.

[Peter Kasting, 2014-02-25 00:01:57]

Here are a few explanatory comments.

https://codereview.chromium.org/151483008/diff/230001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbarTheme.h (right):

https://codereview.chromium.org/151483008/diff/230001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbarTheme.h:49: virtual bool
shouldSnapBackToDragOrigin(ScrollbarThemeClient* scrollbar, const
PlatformMouseEvent& event) OVERRIDE { return
ScrollbarTheme::theme()->shouldSnapBackToDragOrigin(scrollbar, event); }
This is necessary so that CSS-styled scrollbars also snap back correctly.

https://codereview.chromium.org/151483008/diff/230001/Source/platform/scroll/...
File Source/platform/scroll/ScrollbarTheme.cpp (left):

https://codereview.chromium.org/151483008/diff/230001/Source/platform/scroll/...
Source/platform/scroll/ScrollbarTheme.cpp:42: ScrollbarTheme*
ScrollbarTheme::theme()
Most of the code changes in this file are just moving code to match the
declaration order in the .h file.  Only the shouldCenterOnThumb() and
shouldSnapBackToDragOrigin() methods are new.

I can split the reordering changes to a separate CL if you wish.  I just noticed
the disparity in passing.

https://codereview.chromium.org/151483008/diff/230001/public/platform/WebScro...
File public/platform/WebScrollbarBehavior.h (right):

https://codereview.chromium.org/151483008/diff/230001/public/platform/WebScro...
public/platform/WebScrollbarBehavior.h:15: // These match the values for
WebCore::MouseButton.
Let me know if I need a comment over on that enum (I assume not, since
WebInputEvent.h has this same sort of enum) or some kind of compile-time
equality check of the values.

[abarth-chromium, 2014-02-25 07:18:37]

LGTM assuming you add the COMPILE_ASSERTS

https://codereview.chromium.org/151483008/diff/230001/public/platform/WebScro...
File public/platform/WebScrollbarBehavior.h (right):

https://codereview.chromium.org/151483008/diff/230001/public/platform/WebScro...
public/platform/WebScrollbarBehavior.h:15: // These match the values for
WebCore::MouseButton.
On 2014/02/25 00:01:58, Peter Kasting wrote:
> Let me know if I need a comment over on that enum (I assume not, since
> WebInputEvent.h has this same sort of enum) or some kind of compile-time
> equality check of the values.

Checkout out AssertMatchingEnums.cpp.  We usually just have compile asserts
instead of comments.

[Peter Kasting, 2014-03-01 01:21:17]

Committed patchset #4 manually as r168177 (presubmit successful).