Chromium Code Reviews
Help | Chromium Project | Sign in
(19)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by chengx
Modified:
1 month 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 172 (137 generated)
chengx
This CL is ready for review. PTAL~
2 months, 2 weeks 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 ...
2 months, 2 weeks ago (2017-01-05 00:45:00 UTC) #47
Bret Sepulveda
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 ...
2 months, 2 weeks ago (2017-01-05 00:53:29 UTC) #48
aelias
On 2017/01/05 at 00:45:00, chengx wrote: > aelias@, sorry I just found that you are ...
2 months, 2 weeks ago (2017-01-05 00:55:37 UTC) #50
aelias
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 ...
2 months, 2 weeks 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 ...
2 months, 2 weeks ago (2017-01-05 04:50:13 UTC) #56
Bret Sepulveda
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: ...
2 months, 2 weeks ago (2017-01-05 21:42:44 UTC) #57
aelias
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): ...
2 months, 2 weeks 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 ...
2 months, 2 weeks 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 ...
2 months, 2 weeks 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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, > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-01-12 03:01:08 UTC) #79
aelias
On 2017/01/12 at 01:48:41, chengx wrote: > On 2017/01/12 01:29:17, skobes wrote: > > The ...
2 months, 1 week ago (2017-01-12 03:59:39 UTC) #80
aelias
Err, I didn't read that skobes@ already suggested the exact same thing, sorry. If it's ...
2 months, 1 week 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 ...
1 month ago (2017-02-16 19:07:37 UTC) #89
Bret Sepulveda
On 2017/02/16 19:07:37, chengx wrote: > Hi aelias@ bsep@ skobes@, it has been a little ...
1 month ago (2017-02-16 20:31:45 UTC) #91
aelias
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 ...
1 month 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 ...
1 month 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 }; ...
1 month 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 ...
1 month ago (2017-02-17 20:52:46 UTC) #134
aelias
lgtm
1 month ago (2017-02-17 20:56:03 UTC) #135
Bret Sepulveda
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? ...
1 month 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): ...
1 month 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 ...
1 month 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, // ...
1 month ago (2017-02-17 23:19:18 UTC) #147
sky
Ok, LGTM
1 month 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
1 month 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
1 month 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 ...
1 month ago (2017-02-18 11:58:06 UTC) #171
chengx
1 month 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62