Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(205)

Issue 2605193002: Fix mouse wheel over-scrolls when display is scaled and scroll is paginated (Closed)

Created:
11 months, 2 weeks ago by chengx
Modified:
9 months, 4 weeks ago
CC:
brucedawson, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix mouse wheel over-scrolls when display is scaled and scroll is paginated In Windows, when it is set to "One screen at a time" per wheel notch vertical scrolling, and to over 100% display scaling, the page scrolls up or down by more than 100% (actually display_scaling * 100%), where approximately 100% is expected. Basically, the scrolling takes display scaling into account which it shouldn't. This makes paginated mouse wheel scrolling unusable when display is scaled to over 100%. This CL fixes this bug. BUG=666133 Review-Url: https://codereview.chromium.org/2605193002 Cr-Commit-Position: refs/heads/master@{#451438} Committed: https://chromium.googlesource.com/chromium/src/+/9c53350ce5b4233fedd89b6593451df30539b920

Patch Set 1 #

Total comments: 3

Patch Set 2 : Make the conversion specific to paginated scroll. #

Patch Set 3 : Git pull to make the files touched up to date. #

Patch Set 4 : Not scale paginated mouse scroll with the display scale factor at the beginning. #

Total comments: 6

Patch Set 5 : Add unittests and revert file TestExpectations. #

Total comments: 10

Patch Set 6 : Better function/variable names. #

Total comments: 6

Patch Set 7 : More unittests. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -10 lines) Patch
M ui/events/blink/blink_event_util.cc View 1 2 3 4 5 3 chunks +22 lines, -8 lines 0 comments Download
M ui/events/blink/blink_event_util_unittest.cc View 1 2 3 4 5 6 2 chunks +103 lines, -0 lines 0 comments Download
M ui/events/gesture_event_details.h View 1 2 3 4 5 6 4 chunks +26 lines, -1 line 2 comments Download
M ui/events/gesture_event_details.cc View 1 2 3 4 5 2 chunks +4 lines, -1 line 0 comments Download

Messages

Total messages: 172 (137 generated)
chengx
This CL is ready for review. PTAL~
11 months, 1 week ago (2017-01-05 00:34:15 UTC) #45
chengx
aelias@, sorry I just found that you are the owner for Android as indicated in ...
11 months, 1 week ago (2017-01-05 00:45:00 UTC) #47
Bret
https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode755 third_party/WebKit/Source/web/WebViewImpl.cpp:755: #if OS(WIN) Why is this Windows-only? It seems like ...
11 months, 1 week ago (2017-01-05 00:53:29 UTC) #48
aelias_OOO_until_Jul13
On 2017/01/05 at 00:45:00, chengx wrote: > aelias@, sorry I just found that you are ...
11 months, 1 week ago (2017-01-05 00:55:37 UTC) #50
aelias_OOO_until_Jul13
https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode756 third_party/WebKit/Source/web/WebViewImpl.cpp:756: if (event.type == WebInputEvent::GestureScrollUpdate) { CC impl thread also ...
11 months, 1 week ago (2017-01-05 00:55:45 UTC) #51
chengx
aelias@ Thanks a lot for being the reviewer of this CL. I am still quite ...
11 months, 1 week ago (2017-01-05 04:50:13 UTC) #56
Bret
https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode757 third_party/WebKit/Source/web/WebViewImpl.cpp:757: float scaleFactor = client()->GetOriginalDeviceScaleFactor(); On 2017/01/05 04:50:13, chengx wrote: ...
11 months, 1 week ago (2017-01-05 21:42:44 UTC) #57
aelias_OOO_until_Jul13
Sure, no problem, it can pretty nonobvious which reviewer to pick. https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): ...
11 months, 1 week ago (2017-01-06 02:05:46 UTC) #58
chengx
Something went wrong with my repo, so I had to git pull, which messed up ...
11 months, 1 week ago (2017-01-06 23:44:09 UTC) #68
chengx
Something went wrong with my repo, so I had to git pull, which messed up ...
11 months, 1 week ago (2017-01-06 23:46:37 UTC) #69
chengx
aelias@, gentle ping, PTAL~ Below is what I have investigated to your suggestions. Also a ...
11 months ago (2017-01-12 00:08:16 UTC) #72
skobes
The page scroll distance comes from ScrollableArea::pageStep, which should return physical pixels with --enable-use-zoom-for-dsf (or ...
11 months ago (2017-01-12 01:29:17 UTC) #74
chengx
On 2017/01/12 01:29:17, skobes wrote: > The page scroll distance comes from ScrollableArea::pageStep, which should ...
11 months ago (2017-01-12 01:48:41 UTC) #75
skobes
On 2017/01/12 01:48:41, chengx wrote: > On 2017/01/12 01:29:17, skobes wrote: > > The page ...
11 months ago (2017-01-12 01:57:16 UTC) #76
skobes
On 2017/01/12 01:48:41, chengx wrote: > When WebGestureEvent is generated in mouse_wheel_event_queue.cc mentioned above, > ...
11 months ago (2017-01-12 02:03:12 UTC) #77
chengx
On 2017/01/12 02:03:12, skobes wrote: > On 2017/01/12 01:48:41, chengx wrote: > > When WebGestureEvent ...
11 months ago (2017-01-12 02:08:19 UTC) #78
chengx
On 2017/01/12 01:29:17, skobes wrote: > The page scroll distance comes from ScrollableArea::pageStep, which should ...
11 months ago (2017-01-12 03:01:08 UTC) #79
aelias_OOO_until_Jul13
On 2017/01/12 at 01:48:41, chengx wrote: > On 2017/01/12 01:29:17, skobes wrote: > > The ...
11 months ago (2017-01-12 03:59:39 UTC) #80
aelias_OOO_until_Jul13
Err, I didn't read that skobes@ already suggested the exact same thing, sorry. If it's ...
11 months ago (2017-01-12 04:01:58 UTC) #81
chengx
Hi aelias@ bsep@ skobes@, it has been a little while since the last update I ...
10 months ago (2017-02-16 19:07:37 UTC) #89
Bret
On 2017/02/16 19:07:37, chengx wrote: > Hi aelias@ bsep@ skobes@, it has been a little ...
10 months ago (2017-02-16 20:31:45 UTC) #91
aelias_OOO_until_Jul13
Fixing it in that spot looks appropriate, thanks. https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/LayoutTests/TestExpectations#newcode861 third_party/WebKit/LayoutTests/TestExpectations:861: crbug.com/666133 ...
10 months ago (2017-02-16 22:11:13 UTC) #92
chengx
Hi aelias@, it turns out I don't need to modify file TestExpectations at all. The ...
10 months ago (2017-02-17 08:26:02 UTC) #126
sky
https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_event_details.h File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_event_details.h#newcode20 ui/events/gesture_event_details.h:20: enum ScrollUnits { PrecisePixels = 0, Pixels, Page }; ...
9 months, 4 weeks ago (2017-02-17 18:00:25 UTC) #127
chengx
Hi sky@, I have addressed your comments in the new patch set. PTAL~ https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_event_details.h File ...
9 months, 4 weeks ago (2017-02-17 20:52:46 UTC) #134
aelias_OOO_until_Jul13
lgtm
9 months, 4 weeks ago (2017-02-17 20:56:03 UTC) #135
Bret
lgtm otherwise https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_event_util.cc File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_event_util.cc#newcode759 ui/events/blink/blink_event_util.cc:759: if (!wheel_event->scrollByPage) { Test for this too? ...
9 months, 4 weeks ago (2017-02-17 21:06:03 UTC) #136
chengx
Updated unittests in the new patch set, as suggested by bsep@ https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_event_util.cc File ui/events/blink/blink_event_util.cc (right): ...
9 months, 4 weeks ago (2017-02-17 21:57:45 UTC) #141
sky
https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_event_details.h File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_event_details.h#newcode24 ui/events/gesture_event_details.h:24: PIXELS, // large pixel jump duration; should animate to ...
9 months, 4 weeks ago (2017-02-17 22:52:31 UTC) #146
chengx
Hi sky@, please see my reply below. https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_event_details.h File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_event_details.h#newcode24 ui/events/gesture_event_details.h:24: PIXELS, // ...
9 months, 4 weeks ago (2017-02-17 23:19:18 UTC) #147
sky
Ok, LGTM
9 months, 4 weeks ago (2017-02-17 23:40:20 UTC) #148
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2605193002/500001
9 months, 4 weeks ago (2017-02-18 06:56:58 UTC) #167
commit-bot: I haz the power
Committed patchset #7 (id:500001) as https://chromium.googlesource.com/chromium/src/+/9c53350ce5b4233fedd89b6593451df30539b920
9 months, 4 weeks ago (2017-02-18 07:03:48 UTC) #170
dtapuska
On 2017/02/18 07:03:48, commit-bot: I haz the power wrote: > Committed patchset #7 (id:500001) as ...
9 months, 4 weeks ago (2017-02-18 11:58:06 UTC) #171
chengx
9 months, 4 weeks ago (2017-02-18 17:38:59 UTC) #172
Message was sent while issue was closed.
On 2017/02/18 11:58:06, dtapuska wrote:
> On 2017/02/18 07:03:48, commit-bot: I haz the power wrote:
> > Committed patchset #7 (id:500001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/9c53350ce5b4233fedd89b659345...
> It isn't clear to me where this additional ctor parameters is passed in. It
> seems the delta units is always precise pixels is that true?

We didn't have ScrollUnits for class GestureEventDetails, so when
GestureEventDetails was used to create WebGestureEvent in blink_event_util.cc,
as in 
https://cs.chromium.org/chromium/src/ui/events/blink/blink_event_util.cc?l=596
WebGestureEvent used the default value for the scroll delta units. The default
value is PrecisePixels, as documented in
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebGe...
and
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebGe...

So I think using PRECISE_PIXELS (same as PrecisePixels in WebGestureEvent.h just
using ALL_CAPS) here is fine. Besides, I tried PIXELS as default for curiosity
and some try bots failed, which is expected because of inconsistency.

Powered by Google App Engine
This is Rietveld 0eb02b776