On some webpages, touch scrolling speed is too slow / fast

On some webpages (for example: stackoverflow.com), touch scrolling speed is too slow / fast when zoomed in / out.

The problem was that the zoom multiplier was applied twice:
- in handleGestureScrollUpdate
- somewhere deeper when handling wheel event

This patch fixes this issue, by passing unscaled value to EventHandler::sendScrollEventToView(...).

BUG=403304
TEST=Open stackoverflow.com, Set browser zoom to 50%, touch scroll  (sendScrollEventToView path)
TEST=Open http://goo.gl/OYhULR, Set browser zoom to 200%, touch scroll text area  (RenderBox path)

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

[kjakubowski, 2014-08-14 10:17:03]

Hi,

Can you have a look at my review?

[Rick Byers, 2014-08-15 16:11:50]

On 2014/08/14 10:17:03, kjakubowski wrote:
> Hi,
> 
> Can you have a look at my review?

I added some notes to the bug - I can't seem to reproduce the issue in Chrome. 
I can believe there is a bug along these lines though, but I think we need to
understand it better to avoid regressing some scenarios.  What conditions
exactly are necessary to hit the bug?  Also your patch needs a test.

[Rick Byers, 2014-08-15 16:19:39]

On 2014/08/15 16:11:50, Rick Byers wrote:
> On 2014/08/14 10:17:03, kjakubowski wrote:
> > Hi,
> > 
> > Can you have a look at my review?
> 
> I added some notes to the bug - I can't seem to reproduce the issue in Chrome.

> I can believe there is a bug along these lines though, but I think we need to
> understand it better to avoid regressing some scenarios.  What conditions
> exactly are necessary to hit the bug?  Also your patch needs a test.

I took a quick look through the frame scrolling code and I don't see it applying
the page scale factor to the delta anywhere (but I may have missed it) - eg. see
ScrollAnimator::handleWheelEvent.  So I think this code path expects a scaled
delta as written.

One complexity here is that in Chrome we now work hard to do at least all page
scrolling in the compositor on all platforms.  So this main thread frame scroll
path doesn't get hit often - maybe just for iframes on low-dpi devices (where we
can't use accelerated overflow scrolling).  Perhaps Opera is different in this
regard?

Adding bokan@ since he's done some work here recently with the virtual viewport
and may have some insight on how scrolling interacts with the page scale factor.

[kjakubowski, 2014-08-18 10:57:51]

Thanks for looking at this issue.

I've added some more notes to the bug about how to reproduce it. For me it
reproduces
just the same on Opera and on Chrome Canary.

> One complexity here is that in Chrome we now work hard to do at least all page
scrolling in the compositor on all platforms.  So this main thread frame scroll
path doesn't get hit often - maybe just for iframes on low-dpi devices (where we
can't use accelerated overflow scrolling).  Perhaps Opera is different in this
regard?
I'm actually doing all the testing in HiDPI (150%), I've also tried few
different
DPI-settings and it seems to be unrelated.

[bokan, 2014-08-18 15:08:19]

On 2014/08/15 16:19:39, Rick Byers wrote:
> On 2014/08/15 16:11:50, Rick Byers wrote:
> > On 2014/08/14 10:17:03, kjakubowski wrote:
> > > Hi,
> > > 
> > > Can you have a look at my review?
> > 
> > I added some notes to the bug - I can't seem to reproduce the issue in
Chrome.
> 
> > I can believe there is a bug along these lines though, but I think we need
to
> > understand it better to avoid regressing some scenarios.  What conditions
> > exactly are necessary to hit the bug?  Also your patch needs a test.
> 
> I took a quick look through the frame scrolling code and I don't see it
applying
> the page scale factor to the delta anywhere (but I may have missed it) - eg.
see
> ScrollAnimator::handleWheelEvent.  So I think this code path expects a scaled
> delta as written.
> 
> One complexity here is that in Chrome we now work hard to do at least all page
> scrolling in the compositor on all platforms.  So this main thread frame
scroll
> path doesn't get hit often - maybe just for iframes on low-dpi devices (where
we
> can't use accelerated overflow scrolling).  Perhaps Opera is different in this
> regard?
> 
> Adding bokan@ since he's done some work here recently with the virtual
viewport
> and may have some insight on how scrolling interacts with the page scale
factor.

I managed to repro on a Windows box, not on ChromeOS. I think this is probably
likely due to ChromeOS always taking the compositor-path. Note this is browser
zoom (I spent a few minutes unable to repro since I was trying with pinch).

I don't see a scale being applied anywhere in the wheel handling code either.
This is curious as the code path eventually merges with ScrollView's path in 
ScrollAnimator::scroll() so I can't see the difference - we should understand
this before committing anything. Another possibility is that the RenderBox's
scroll should also be unscaled (i.e. is currently wrong) but we never see it
since they're always composited.

[Rick Byers, 2014-08-18 15:36:54]

On 2014/08/18 15:08:19, bokan wrote:
> On 2014/08/15 16:19:39, Rick Byers wrote:
> > On 2014/08/15 16:11:50, Rick Byers wrote:
> > > On 2014/08/14 10:17:03, kjakubowski wrote:
> > > > Hi,
> > > > 
> > > > Can you have a look at my review?
> > > 
> > > I added some notes to the bug - I can't seem to reproduce the issue in
> Chrome.
> > 
> > > I can believe there is a bug along these lines though, but I think we need
> to
> > > understand it better to avoid regressing some scenarios.  What conditions
> > > exactly are necessary to hit the bug?  Also your patch needs a test.
> > 
> > I took a quick look through the frame scrolling code and I don't see it
> applying
> > the page scale factor to the delta anywhere (but I may have missed it) - eg.
> see
> > ScrollAnimator::handleWheelEvent.  So I think this code path expects a
scaled
> > delta as written.
> > 
> > One complexity here is that in Chrome we now work hard to do at least all
page
> > scrolling in the compositor on all platforms.  So this main thread frame
> scroll
> > path doesn't get hit often - maybe just for iframes on low-dpi devices
(where
> we
> > can't use accelerated overflow scrolling).  Perhaps Opera is different in
this
> > regard?
> > 
> > Adding bokan@ since he's done some work here recently with the virtual
> viewport
> > and may have some insight on how scrolling interacts with the page scale
> factor.
> 
> I managed to repro on a Windows box, not on ChromeOS. I think this is probably
> likely due to ChromeOS always taking the compositor-path. Note this is browser
> zoom (I spent a few minutes unable to repro since I was trying with pinch).
> 
> I don't see a scale being applied anywhere in the wheel handling code either.
> This is curious as the code path eventually merges with ScrollView's path in 
> ScrollAnimator::scroll() so I can't see the difference - we should understand
> this before committing anything. Another possibility is that the RenderBox's
> scroll should also be unscaled (i.e. is currently wrong) but we never see it
> since they're always composited.

That appears to be the case (see bug) - although that's easy to repro on low-dpi
where overflor:scroll is usually not composited scrolling.  So this fix should
be updated to apply consistently to both the frame and RenderBox case (and a
test added for both).

Thanks for finding this!

[kjakubowski, 2014-09-15 09:43:49]

> 
> That appears to be the case (see bug) - although that's easy to repro on
low-dpi
> where overflor:scroll is usually not composited scrolling.  So this fix should
> be updated to apply consistently to both the frame and RenderBox case (and a
> test added for both).
> 
> Thanks for finding this!

I've updated the patch as You've suggested. Indeed, there was also a problem
with
RenderBox path (I've added another test case).

BTW. Sorry for late response :).

[kjakubowski, 2014-09-15 12:32:44]

As for the correctness of this patch:
Multiplication of delta by inverse of current browser zoom is not required,
because web-page elements are already scaled by this zoom.
For example: if in 100% zoom content rect of ScrollArea has 8000 units, in 50%
zoom it will have 4000 units.

[kjakubowski, 2014-09-17 10:46:17]

Patchset #1 (id:1) has been deleted

[kjakubowski, 2014-09-17 10:46:25]

The CQ bit was checked by kjakubowski@opera.com

[commit-bot: I haz the power, 2014-09-17 10:46:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/472463002/20001

[commit-bot: I haz the power, 2014-09-17 10:46:58]

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

[commit-bot: I haz the power, 2014-09-17 10:46:59]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Rick Byers, 2014-09-17 14:38:51]

On 2014/09/15 09:43:49, kjakubowski wrote:
> > 
> > That appears to be the case (see bug) - although that's easy to repro on
> low-dpi
> > where overflor:scroll is usually not composited scrolling.  So this fix
should
> > be updated to apply consistently to both the frame and RenderBox case (and a
> > test added for both).
> > 
> > Thanks for finding this!
> 
> I've updated the patch as You've suggested. Indeed, there was also a problem
> with
> RenderBox path (I've added another test case).
> 
> BTW. Sorry for late response :).

This fix looks reasonable to me, but we need automated tests for it.  Luckily I
don't think they'll be hard to add.  Take a look at
LayoutTests/fast/events/touch/gesture/*scroll*.  You should be able to take a
couple of the simple scrolling examples there and just apply browser zoom to
them using window.internals.setZoomFactor.  LayoutTests never get composited
scrolling (since they test blink, not all of chrome), so you don't need to do
anything special to provoke main thread scrolling there.

[kjakubowski, 2014-09-18 15:50:04]

Patchset #1 (id:20001) has been deleted

[kjakubowski, 2014-09-18 15:52:12]

> This fix looks reasonable to me, but we need automated tests for it.  Luckily
I
> don't think they'll be hard to add.  Take a look at
> LayoutTests/fast/events/touch/gesture/*scroll*.  You should be able to take a
> couple of the simple scrolling examples there and just apply browser zoom to
> them using window.internals.setZoomFactor.  LayoutTests never get composited
> scrolling (since they test blink, not all of chrome), so you don't need to do
> anything special to provoke main thread scrolling there.

I've modified two tests just as You've proposed. Now, those tests will only
pass if touch scrolling works for different zoom factors.

[Rick Byers, 2014-09-18 16:01:54]

On 2014/09/18 15:52:12, kjakubowski wrote:
> > This fix looks reasonable to me, but we need automated tests for it. 
Luckily
> I
> > don't think they'll be hard to add.  Take a look at
> > LayoutTests/fast/events/touch/gesture/*scroll*.  You should be able to take
a
> > couple of the simple scrolling examples there and just apply browser zoom to
> > them using window.internals.setZoomFactor.  LayoutTests never get composited
> > scrolling (since they test blink, not all of chrome), so you don't need to
do
> > anything special to provoke main thread scrolling there.
> 
> I've modified two tests just as You've proposed. Now, those tests will only
> pass if touch scrolling works for different zoom factors.

Thanks, but rather than modify the existing test cases, can you please add new
ones (ideally in new files if you can avoid excessive copy/paste).  That way we
can easily distinguish fundamental breakage in scrolling functionality from
breakages that only affect this (slightly unusual) case of browser zoom.

[kjakubowski, 2014-09-18 17:10:42]

> Thanks, but rather than modify the existing test cases, can you please add new
> ones (ideally in new files if you can avoid excessive copy/paste).  That way
we
> can easily distinguish fundamental breakage in scrolling functionality from
> breakages that only affect this (slightly unusual) case of browser zoom.

OK, I will do it next Monday.

[kjakubowski, 2014-09-22 12:42:20]

Patchset #1 (id:40001) has been deleted

[kjakubowski, 2014-09-22 12:46:15]

On 2014/09/18 17:10:42, kjakubowski wrote:
> > Thanks, but rather than modify the existing test cases, can you please add
new
> > ones (ideally in new files if you can avoid excessive copy/paste).  That way
> we
> > can easily distinguish fundamental breakage in scrolling functionality from
> > breakages that only affect this (slightly unusual) case of browser zoom.
> 
> OK, I will do it next Monday.

Done, I've added two new tests, which are very similar to normal touch-scrolling
tests, but
that would be hard to avoid.

[Rick Byers, 2014-09-22 21:21:16]

Thanks.  How are you uploading your patches?  Rietveld seems to be overwriting
your earlier versions (there's only "patch set 1" listed).  Normally "git cl
upload" will create a new patchset on an existing issue (letting someone track
the changes from patch to patch).

https://codereview.chromium.org/472463002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/touch/gesture/gesture-scroll-zoomed.html (right):

https://codereview.chromium.org/472463002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/touch/gesture/gesture-scroll-zoomed.html:79:
shouldBe('movingdiv.scrollTop', scrollAmountY[scrollsOccurred]);
This test is supposed to be checking page scrolling (not div scrolling), right? 
Looks like you modeled both tests after touch-gesture-scroll-div.html.  Did you
mean to modify touch-gesture-scroll-page.html here instead?

[Rick Byers, 2014-09-22 21:23:00]

On 2014/09/22 21:21:16, Rick Byers wrote:
> Thanks.  How are you uploading your patches?  Rietveld seems to be overwriting
> your earlier versions (there's only "patch set 1" listed).  Normally "git cl
> upload" will create a new patchset on an existing issue (letting someone track
> the changes from patch to patch).
> 
>
https://codereview.chromium.org/472463002/diff/60001/LayoutTests/fast/events/...
> File LayoutTests/fast/events/touch/gesture/gesture-scroll-zoomed.html (right):
> 
>
https://codereview.chromium.org/472463002/diff/60001/LayoutTests/fast/events/...
> LayoutTests/fast/events/touch/gesture/gesture-scroll-zoomed.html:79:
> shouldBe('movingdiv.scrollTop', scrollAmountY[scrollsOccurred]);
> This test is supposed to be checking page scrolling (not div scrolling),
right? 
> Looks like you modeled both tests after touch-gesture-scroll-div.html.  Did
you
> mean to modify touch-gesture-scroll-page.html here instead?

By the way https://codereview.chromium.org/571963003/ will make it much easier
to see this bug in practice.  Just enable this flag to force us to always take
the blink scrolling path.

[kjakubowski, 2014-09-23 07:08:55]

On 2014/09/22 21:21:16, Rick Byers wrote:
> Thanks.  How are you uploading your patches?  Rietveld seems to be overwriting
> your earlier versions (there's only "patch set 1" listed).  Normally "git cl
> upload" will create a new patchset on an existing issue (letting someone track
> the changes from patch to patch).
I've removed older patches myself, I thought they weren't required any more :).

[Rick Byers, 2014-09-23 13:34:26]

On 2014/09/23 07:08:55, kjakubowski wrote:
> On 2014/09/22 21:21:16, Rick Byers wrote:
> > Thanks.  How are you uploading your patches?  Rietveld seems to be
overwriting
> > your earlier versions (there's only "patch set 1" listed).  Normally "git cl
> > upload" will create a new patchset on an existing issue (letting someone
track
> > the changes from patch to patch).
> I've removed older patches myself, I thought they weren't required any more
:).

Normally we leave the patch history in place, it helps people to see how the
patch evolved.  Eg. I wanted to go back and see if the problem of testing div
scrolling in both cases was something I missed in the previous patch, or a new
problem you introduced in the latest patch.

[kjakubowski, 2014-09-23 13:48:41]

OK, next time I will leave old patches alone :).

[kjakubowski, 2014-09-23 13:49:45]

>
https://codereview.chromium.org/472463002/diff/60001/LayoutTests/fast/events/...
> LayoutTests/fast/events/touch/gesture/gesture-scroll-zoomed.html:79:
> shouldBe('movingdiv.scrollTop', scrollAmountY[scrollsOccurred]);
> This test is supposed to be checking page scrolling (not div scrolling),
right? 
> Looks like you modeled both tests after touch-gesture-scroll-div.html.  Did
you
> mean to modify touch-gesture-scroll-page.html here instead?

You are absolutely right. I will fix it and reupload the patch. Thanks for the
patience :)

[kjakubowski, 2014-09-23 15:15:03]

I've uploaded fixed tests:
- recordScroll function had to be modified, to pass presubmit tests (it
contained tabs)
- instead of gesture-scroll.html, touch-gesture-scroll-page.html is used as a
base for
  zoomed page scroll test

These tests should fail when touch scroll is wrong when zoomed. I will test this
tomorrow (and apply some fixes if necessary).

[Rick Byers, 2014-09-23 17:57:10]

On 2014/09/23 15:15:03, kjakubowski wrote:
> I've uploaded fixed tests:
> - recordScroll function had to be modified, to pass presubmit tests (it
> contained tabs)
> - instead of gesture-scroll.html, touch-gesture-scroll-page.html is used as a
> base for
>   zoomed page scroll test
> 
> These tests should fail when touch scroll is wrong when zoomed. I will test
this
> tomorrow (and apply some fixes if necessary).

This looks good, thanks.  LGTM, feel free to put this into the commit queue once
you verify the tests do both indeed fail without your fix.

Note that there's other things I'd like to clean up about these old tests
(they're way more complicated than they need to be), but that's a separate
pre-existing issue as far as I'm concerned.  Copying the pattern again here is
fine with me - someone can clean them all up together at some point in the
future.

Thanks again for finding this and pushing on a fix!  As we start to experiment
more with blink-driven synchronized scrolling (as part of
http://crbug.com/410974) I'm sure this would have become urgent for us too.

[kjakubowski, 2014-09-24 06:58:47]

Verified:
- touch-gesture-scroll-div-zoomed.html checks RenderBox path in
EventHandler::handleGestureScrollUpdate
- touch-gesture-scroll-page-zoomed.html checks default path
(sendScrollEventToView) in EventHandler::handleGestureScrollUpdate

[kjakubowski, 2014-09-24 07:00:08]

The CQ bit was checked by kjakubowski@opera.com

[commit-bot: I haz the power, 2014-09-24 07:01:02]

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

[kjakubowski, 2014-09-24 08:05:49]

On 2014/09/24 07:01:02, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/472463002/80001

touch-gesture-scroll-page-zoomed failed. I'm guessing that, when zoomed-out,
there wasn't enough page-space to scroll. I've increased number of rectangles.
This should solve the problem.

[kjakubowski, 2014-09-24 08:13:42]

On 2014/09/24 08:05:49, kjakubowski wrote:
> On 2014/09/24 07:01:02, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/472463002/80001
> 
> touch-gesture-scroll-page-zoomed failed. I'm guessing that, when zoomed-out,
> there wasn't enough page-space to scroll. I've increased number of rectangles.
> This should solve the problem.

It's strange though, because both tests work without any problems,
when I run them on my local machine.

[Rick Byers, 2014-10-01 16:09:04]

On 2014/09/24 08:13:42, kjakubowski wrote:
> On 2014/09/24 08:05:49, kjakubowski wrote:
> > On 2014/09/24 07:01:02, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/472463002/80001
> > 
> > touch-gesture-scroll-page-zoomed failed. I'm guessing that, when zoomed-out,
> > there wasn't enough page-space to scroll. I've increased number of
rectangles.
> > This should solve the problem.
> 
> It's strange though, because both tests work without any problems,
> when I run them on my local machine.

Did you see that you can click on the layout_test_results link and see the
results?
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

It looks like you need to update your test for my recent change to expose
fractional scroll offsets, see http://crbug.com/373731.  The hit-test-counts
failure looks unrelated (a chromium/blink version mismatch issue - should be
cleared up now).

[kjakubowski, 2014-10-01 19:30:27]

> Did you see that you can click on the layout_test_results link and see the
> results?
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
That's good to know, I won't have to guess what went wrong :)

> It looks like you need to update your test for my recent change to expose
> fractional scroll offsets, see http://crbug.com/373731.  The hit-test-counts
> failure looks unrelated (a chromium/blink version mismatch issue - should be
> cleared up now).
I will fix the tests and upload improved solution as soon as I get back to work
(Monday probably).

[kjakubowski, 2014-10-06 08:33:23]

Patchset #3 (id:100001) has been deleted

[kjakubowski, 2014-10-06 11:22:11]

> > It looks like you need to update your test for my recent change to expose
> > fractional scroll offsets, see http://crbug.com/373731.  The hit-test-counts
> > failure looks unrelated (a chromium/blink version mismatch issue - should be
> > cleared up now).
> I will fix the tests and upload improved solution as soon as I get back to
work
> (Monday probably).

Done.

[Rick Byers, 2014-10-06 21:14:51]

On 2014/10/06 11:22:11, kjakubowski wrote:
> > > It looks like you need to update your test for my recent change to expose
> > > fractional scroll offsets, see http://crbug.com/373731.  The
hit-test-counts
> > > failure looks unrelated (a chromium/blink version mismatch issue - should
be
> > > cleared up now).
> > I will fix the tests and upload improved solution as soon as I get back to
> work
> > (Monday probably).
> 
> Done.

LGTM.

I've requested blink try jobs, if they pass feel free to use the 'commit' button
(it doesn't re-run the tests again when there are recent successes).

[kjakubowski, 2014-10-07 07:56:40]

The CQ bit was checked by kjakubowski@opera.com

[commit-bot: I haz the power, 2014-10-07 07:56:47]

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

[commit-bot: I haz the power, 2014-10-07 08:53:42]

Committed patchset #3 (id:120001) as 183318