Fix Mac overlay scrollbars to show when programmatically scrolled.

Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. The problem is
that ScrollAnimatorMac::cancelAnimations, which is part of the
ScrollableArea interface, was cancelling the internal scrollbar painting
animations and not the user scroll animation as expected.

This patch replaces the paint cancelling code in cancelAnimations with
_stopRun which stops the actual scroll instead.

BUG=535684
Committed: https://crrev.com/bb01745001880da55a4e56416e35cc459204c042
Cr-Commit-Position: refs/heads/master@{#378488}

[bokan, 2016-01-04 19:16:28]

Description was changed from

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. This shouldn't be
a problem but I suspect that the cancelAnimation method was leaving the
scrollbar painter in a bad state. Explicitly hiding the overlay scrollbars
in cancelAnimations fixes this issue.

BUG=535684
==========

to

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. This shouldn't be
a problem but I suspect that the cancelAnimation method was leaving the
scrollbar painter in a bad state. Explicitly hiding the overlay scrollbars
in cancelAnimations fixes this issue.

BUG=535684
==========

[bokan, 2016-01-04 19:16:28]

bokan@chromium.org changed reviewers:
+ pinkerton@chromium.org

[bokan, 2016-01-04 19:16:28]

Description was changed from

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. This shouldn't be
a problem but I suspect that the cancelAnimation method was leaving the
scrollbar painter in a bad state. Explicitly hiding the overlay scrollbars
in cancelAnimations fixes this issue.

BUG=535684
==========

to

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. This shouldn't be
a problem but I suspect that the cancelAnimation method was leaving the
scrollbar painter in a bad state. Explicitly hiding the overlay scrollbars
in cancelAnimations fixes this issue.

BUG=535684
==========

[bokan, 2016-01-04 19:16:28]

bokan@chromium.org changed reviewers:
+ pinkerton@chromium.org

[bokan, 2016-01-04 19:20:33]

pinkerton@: It looks like canceling the animation prevents the scrollbars from
showing at all in subsequent scrolls. I suspect it's because it leaves the
animation in a bad state but I can't tell since the API seems to be provided by
OSX but I can't find any docs on it.

Could you please take a look or add someone who knows about this? Thanks.

[bokan, 2016-01-04 19:20:35]

pinkerton@: It looks like canceling the animation prevents the scrollbars from
showing at all in subsequent scrolls. I suspect it's because it leaves the
animation in a bad state but I can't tell since the API seems to be provided by
OSX but I can't find any docs on it.

Could you please take a look or add someone who knows about this? Thanks.

[pink (ping after 24hrs), 2016-01-04 19:26:14]

pinkerton@chromium.org changed reviewers:
+ ellyjones@chromium.org

[pink (ping after 24hrs), 2016-01-04 19:26:14]

+elly who has been digging a lot into scrollbars lately

[bokan, 2016-01-08 00:39:51]

friendly ping

[Elly Fong-Jones, 2016-01-08 14:17:05]

lgtm

[bokan, 2016-01-08 16:40:19]

bokan@chromium.org changed reviewers:
+ vollick@chromium.org

[bokan, 2016-01-08 16:40:19]

+vollick@ For OWNER

[bokan, 2016-01-09 18:10:52]

Re: why no test?

It seems that layout tests run with overlay scrollbars turned off (I'm guessing
they make pixel tests flaky) so I can't do a layout test. The Mac API seems to
be pretty opaque and undocumented so I don't know if/how I could do a unit test
to check its state.

[Ian Vollick, 2016-01-11 15:33:46]

On 2016/01/09 18:10:52, bokan wrote:
> Re: why no test?
> 
> It seems that layout tests run with overlay scrollbars turned off (I'm
guessing
> they make pixel tests flaky) so I can't do a layout test. The Mac API seems to
> be pretty opaque and undocumented so I don't know if/how I could do a unit
test
> to check its state.

You can enable scrollbars with internal settings:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

You can also disable mock scrollbars. Will that give you what you need?

[bokan, 2016-01-19 21:23:02]

On 2016/01/11 15:33:46, vollick wrote:
> On 2016/01/09 18:10:52, bokan wrote:
> > Re: why no test?
> > 
> > It seems that layout tests run with overlay scrollbars turned off (I'm
> guessing
> > they make pixel tests flaky) so I can't do a layout test. The Mac API seems
to
> > be pretty opaque and undocumented so I don't know if/how I could do a unit
> test
> > to check its state.
> 
> You can enable scrollbars with internal settings:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> You can also disable mock scrollbars. Will that give you what you need?

Sorry for the delay.

I've tried various combinations. The mock overlay scrollbars show up but they
don't have the fade in/out behaviour we're testing (and it's a separate code
path I'd imagine). Try as I might, I can't get the non-mock scrollbars to show
up. When I run content shell with --enable-overlay-scrollbar (it's the correct
flag because the non-overlay scrollbars disappear) I don't get any scrollbars at
all.

Elly, do you know if this is something that's expected to work?

[Elly Fong-Jones, 2016-01-21 18:54:09]

On 2016/01/19 21:23:02, bokan wrote:
> On 2016/01/11 15:33:46, vollick wrote:
> > On 2016/01/09 18:10:52, bokan wrote:
> > > Re: why no test?
> > > 
> > > It seems that layout tests run with overlay scrollbars turned off (I'm
> > guessing
> > > they make pixel tests flaky) so I can't do a layout test. The Mac API
seems
> to
> > > be pretty opaque and undocumented so I don't know if/how I could do a unit
> > test
> > > to check its state.
> > 
> > You can enable scrollbars with internal settings:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > You can also disable mock scrollbars. Will that give you what you need?
> 
> Sorry for the delay.
> 
> I've tried various combinations. The mock overlay scrollbars show up but they
> don't have the fade in/out behaviour we're testing (and it's a separate code
> path I'd imagine). Try as I might, I can't get the non-mock scrollbars to show
> up. When I run content shell with --enable-overlay-scrollbar (it's the correct
> flag because the non-overlay scrollbars disappear) I don't get any scrollbars
at
> all.
> 
> Elly, do you know if this is something that's expected to work?

It should work, yeah. What's your General > Show scroll bars setting?

[bokan, 2016-01-21 19:59:44]

On 2016/01/21 18:54:09, Elly Jones wrote:
> On 2016/01/19 21:23:02, bokan wrote:
> > On 2016/01/11 15:33:46, vollick wrote:
> > > On 2016/01/09 18:10:52, bokan wrote:
> > > > Re: why no test?
> > > > 
> > > > It seems that layout tests run with overlay scrollbars turned off (I'm
> > > guessing
> > > > they make pixel tests flaky) so I can't do a layout test. The Mac API
> seems
> > to
> > > > be pretty opaque and undocumented so I don't know if/how I could do a
unit
> > > test
> > > > to check its state.
> > > 
> > > You can enable scrollbars with internal settings:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > You can also disable mock scrollbars. Will that give you what you need?
> > 
> > Sorry for the delay.
> > 
> > I've tried various combinations. The mock overlay scrollbars show up but
they
> > don't have the fade in/out behaviour we're testing (and it's a separate code
> > path I'd imagine). Try as I might, I can't get the non-mock scrollbars to
show
> > up. When I run content shell with --enable-overlay-scrollbar (it's the
correct
> > flag because the non-overlay scrollbars disappear) I don't get any
scrollbars
> at
> > all.
> > 
> > Elly, do you know if this is something that's expected to work?
> 
> It should work, yeah. What's your General > Show scroll bars setting?

I'm away from my Mac right now but it's the default, running Chrome/Chromium
shows overlay scrollbars as expected. ContentShell --enable-overlay-scrollbar
results in no scrollbars at all, LayoutTests that turn off mock scrollbars and
*on* overlay scrollbars don't show anything either. If it's expected to work,
I'll dig a little deeper and see what's going wrong.

[bokan, 2016-02-24 15:41:00]

Sorry for the delay, I had to do some side work to allow scroll animations in
layout tests on Mac.

Now that that's done, I've written up some tests. Turns out that
cancelAnimations, which is part of the ScrollableArea interface, was canceling
the internal scrollbar animations rather than the scroll animation. This means
that cancelAnimations didn't stop an in-flight scroll animation, which is
another bug. I've got a test for both the scrollbar painting and canceling
behaviour.

Unfortunately, these tests are very dependent on timing as we're dealing with
animations. I suspect they may be more flaky than useful. However, with the
recent changes to how LayoutTests handle flakiness maybe they'll be fine. I'm
willing to land these and remove them if they turn out to be more trouble than
they're worth. vollick@, WDYT?

[bokan, 2016-02-25 15:53:00]

vollick@: ping

[Ian Vollick, 2016-02-27 02:12:50]

On 2016/02/24 15:41:00, bokan wrote:
> Sorry for the delay, I had to do some side work to allow scroll animations in
> layout tests on Mac.
> 
> Now that that's done, I've written up some tests. Turns out that
> cancelAnimations, which is part of the ScrollableArea interface, was canceling
> the internal scrollbar animations rather than the scroll animation. This means
> that cancelAnimations didn't stop an in-flight scroll animation, which is
> another bug. I've got a test for both the scrollbar painting and canceling
> behaviour.

Sorry, slightly confused by this. You're saying that your change happened to fix
a second, unanticipated bug or are you saying that there's a second bug yet to
be fixed?
> 
> Unfortunately, these tests are very dependent on timing as we're dealing with
> animations. I suspect they may be more flaky than useful. However, with the
> recent changes to how LayoutTests handle flakiness maybe they'll be fine. I'm
> willing to land these and remove them if they turn out to be more trouble than
> they're worth. vollick@, WDYT?

[bokan, 2016-02-29 14:23:54]

On 2016/02/27 02:12:50, vollick wrote:
> On 2016/02/24 15:41:00, bokan wrote:
> > Sorry for the delay, I had to do some side work to allow scroll animations
in
> > layout tests on Mac.
> > 
> > Now that that's done, I've written up some tests. Turns out that
> > cancelAnimations, which is part of the ScrollableArea interface, was
canceling
> > the internal scrollbar animations rather than the scroll animation. This
means
> > that cancelAnimations didn't stop an in-flight scroll animation, which is
> > another bug. I've got a test for both the scrollbar painting and canceling
> > behaviour.
> 
> Sorry, slightly confused by this. You're saying that your change happened to
fix
> a second, unanticipated bug or are you saying that there's a second bug yet to
> be fixed?

The cause of the scrollbar non-paint issue was that
ScrollAnimatorMac::cancelAnimation was cancelling animations related to
scrollbar painting, not the scroll itself as it's meant to. So removing those
PainterDelegate::cancelAnimation calls fixes the scrollbars not being painted
but it still doesn't cause the scroll animation to cancel so I added the call to
_stopRun which does that. This is the second bug I was referring to. The first
being the scrollbars not painting on a programmatic scroll. The second is that
calling cancelAnimation doesn't stop an in-flight scroll animation. I added a
test for each of these.

BTW, I'm en route to SFO today so I likely wont be very responsive.

[Rick Byers, 2016-02-29 19:06:32]

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

[Rick Byers, 2016-02-29 19:06:33]

https://codereview.chromium.org/1553003002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/compositing/scrollbars/mac-programmatic-scroll-shows-overlay-scrollbars.html
(right):

https://codereview.chromium.org/1553003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/compositing/scrollbars/mac-programmatic-scroll-shows-overlay-scrollbars.html:31:
// likely to cause some flakes on slow runs but we have
If there's no good reliable way to test this (without a ton of test-specific
engineering work), I suggest we just land without a test.  Flaky tests are
generally worse than test holes (as they typically waste a bunch of time dealing
with the flakes and slowing down test runs while tending not to provide much
regression-prevention value).

[Rick Byers, 2016-02-29 19:08:31]

On 2016/02/29 19:06:33, Rick Byers wrote:
>
https://codereview.chromium.org/1553003002/diff/60001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/compositing/scrollbars/mac-programmatic-scroll-shows-overlay-scrollbars.html
> (right):
> 
>
https://codereview.chromium.org/1553003002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/compositing/scrollbars/mac-programmatic-scroll-shows-overlay-scrollbars.html:31:
> // likely to cause some flakes on slow runs but we have
> If there's no good reliable way to test this (without a ton of test-specific
> engineering work), I suggest we just land without a test.  Flaky tests are
> generally worse than test holes (as they typically waste a bunch of time
dealing
> with the flakes and slowing down test runs while tending not to provide much
> regression-prevention value).

Thanks for putting some real effort into trying to test this though!  Sometimes
people claim something is hard to test reliably, but after a bit of effort find
that they were wrong - we definitely don't want to fall into that trap of
avoiding the effort for proper testing :-)

[Ian Vollick, 2016-02-29 19:30:04]

On 2016/02/29 19:08:31, Rick Byers wrote:
> On 2016/02/29 19:06:33, Rick Byers wrote:
> >
>
https://codereview.chromium.org/1553003002/diff/60001/third_party/WebKit/Layo...
> > File
> >
>
third_party/WebKit/LayoutTests/compositing/scrollbars/mac-programmatic-scroll-shows-overlay-scrollbars.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/1553003002/diff/60001/third_party/WebKit/Layo...
> >
>
third_party/WebKit/LayoutTests/compositing/scrollbars/mac-programmatic-scroll-shows-overlay-scrollbars.html:31:
> > // likely to cause some flakes on slow runs but we have
> > If there's no good reliable way to test this (without a ton of test-specific
> > engineering work), I suggest we just land without a test.  Flaky tests are
> > generally worse than test holes (as they typically waste a bunch of time
> dealing
> > with the flakes and slowing down test runs while tending not to provide much
> > regression-prevention value).
> 
> Thanks for putting some real effort into trying to test this though! 
Sometimes
> people claim something is hard to test reliably, but after a bit of effort
find
> that they were wrong - we definitely don't want to fall into that trap of
> avoiding the effort for proper testing :-)

The change itself lgtm, and I agree with Rick about the test situation. This
test is potentially flaky, as you mentioned, and will take a fair bit of time.
Better to land without. Thanks again for writing it up.

[bokan, 2016-03-01 17:31:22]

Thanks for the guidance, that's what I was thinking as well. I'll land without
tests.

[bokan, 2016-03-01 17:31:29]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-03-01 17:31:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org, vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/1553003002/#ps80001
(title: "Removed tests")

[commit-bot: I haz the power, 2016-03-01 17:32:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1553003002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1553003002/80001

[bokan, 2016-03-01 17:33:48]

Description was changed from

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. This shouldn't be
a problem but I suspect that the cancelAnimation method was leaving the
scrollbar painter in a bad state. Explicitly hiding the overlay scrollbars
in cancelAnimations fixes this issue.

BUG=535684
==========

to

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. The problem is
that ScrollAnimatorMac::cancelAnimations, which is part of the
ScrollableArea interface, was cancelling the internal scrollbar painting
animations and not the user scroll animation as expected.

This patch replaces the paint cancelling code in cancelAnimations with
_stopRun which stops the actual scroll instead.

BUG=535684
==========

[commit-bot: I haz the power, 2016-03-01 18:34:27]

Description was changed from

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. The problem is
that ScrollAnimatorMac::cancelAnimations, which is part of the
ScrollableArea interface, was cancelling the internal scrollbar painting
animations and not the user scroll animation as expected.

This patch replaces the paint cancelling code in cancelAnimations with
_stopRun which stops the actual scroll instead.

BUG=535684
==========

to

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. The problem is
that ScrollAnimatorMac::cancelAnimations, which is part of the
ScrollableArea interface, was cancelling the internal scrollbar painting
animations and not the user scroll animation as expected.

This patch replaces the paint cancelling code in cancelAnimations with
_stopRun which stops the actual scroll instead.

BUG=535684
==========

[commit-bot: I haz the power, 2016-03-01 18:34:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-01 18:35:30]

Description was changed from

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. The problem is
that ScrollAnimatorMac::cancelAnimations, which is part of the
ScrollableArea interface, was cancelling the internal scrollbar painting
animations and not the user scroll animation as expected.

This patch replaces the paint cancelling code in cancelAnimations with
_stopRun which stops the actual scroll instead.

BUG=535684
==========

to

==========
Fix Mac overlay scrollbars to show when programmatically scrolled.

This regressed after Blink r197500 since we now (correctly) cancel user
scroll animations when starting a programmatic scroll. The problem is
that ScrollAnimatorMac::cancelAnimations, which is part of the
ScrollableArea interface, was cancelling the internal scrollbar painting
animations and not the user scroll animation as expected.

This patch replaces the paint cancelling code in cancelAnimations with
_stopRun which stops the actual scroll instead.

BUG=535684

Committed: https://crrev.com/bb01745001880da55a4e56416e35cc459204c042
Cr-Commit-Position: refs/heads/master@{#378488}
==========

[commit-bot: I haz the power, 2016-03-01 18:35:31]

Patchset 5 (id:??) landed as
https://crrev.com/bb01745001880da55a4e56416e35cc459204c042
Cr-Commit-Position: refs/heads/master@{#378488}