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

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:
6 months ago by chengx (OOO June 26-28)
Modified:
4 months, 1 week ago
Reviewers:
aelias, Bret, skobes, sky OOO
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 (OOO June 26-28)
This CL is ready for review. PTAL~
5 months, 3 weeks ago (2017-01-05 00:34:15 UTC) #45
chengx (OOO June 26-28)
aelias@, sorry I just found that you are the owner for Android as indicated in ...
5 months, 3 weeks 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 ...
5 months, 3 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 ...
5 months, 3 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 ...
5 months, 3 weeks ago (2017-01-05 00:55:45 UTC) #51
chengx (OOO June 26-28)
aelias@ Thanks a lot for being the reviewer of this CL. I am still quite ...
5 months, 3 weeks 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: ...
5 months, 3 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): ...
5 months, 3 weeks ago (2017-01-06 02:05:46 UTC) #58
chengx (OOO June 26-28)
Something went wrong with my repo, so I had to git pull, which messed up ...
5 months, 3 weeks ago (2017-01-06 23:44:09 UTC) #68
chengx (OOO June 26-28)
Something went wrong with my repo, so I had to git pull, which messed up ...
5 months, 3 weeks ago (2017-01-06 23:46:37 UTC) #69
chengx (OOO June 26-28)
aelias@, gentle ping, PTAL~ Below is what I have investigated to your suggestions. Also a ...
5 months, 2 weeks 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 ...
5 months, 2 weeks ago (2017-01-12 01:29:17 UTC) #74
chengx (OOO June 26-28)
On 2017/01/12 01:29:17, skobes wrote: > The page scroll distance comes from ScrollableArea::pageStep, which should ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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, > ...
5 months, 2 weeks ago (2017-01-12 02:03:12 UTC) #77
chengx (OOO June 26-28)
On 2017/01/12 02:03:12, skobes wrote: > On 2017/01/12 01:48:41, chengx wrote: > > When WebGestureEvent ...
5 months, 2 weeks ago (2017-01-12 02:08:19 UTC) #78
chengx (OOO June 26-28)
On 2017/01/12 01:29:17, skobes wrote: > The page scroll distance comes from ScrollableArea::pageStep, which should ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 2 weeks ago (2017-01-12 04:01:58 UTC) #81
chengx (OOO June 26-28)
Hi aelias@ bsep@ skobes@, it has been a little while since the last update I ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-16 22:11:13 UTC) #92
chengx (OOO June 26-28)
Hi aelias@, it turns out I don't need to modify file TestExpectations at all. The ...
4 months, 1 week ago (2017-02-17 08:26:02 UTC) #126
sky OOO
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 }; ...
4 months, 1 week ago (2017-02-17 18:00:25 UTC) #127
chengx (OOO June 26-28)
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 ...
4 months, 1 week ago (2017-02-17 20:52:46 UTC) #134
aelias
lgtm
4 months, 1 week 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? ...
4 months, 1 week ago (2017-02-17 21:06:03 UTC) #136
chengx (OOO June 26-28)
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): ...
4 months, 1 week ago (2017-02-17 21:57:45 UTC) #141
sky OOO
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 ...
4 months, 1 week ago (2017-02-17 22:52:31 UTC) #146
chengx (OOO June 26-28)
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, // ...
4 months, 1 week ago (2017-02-17 23:19:18 UTC) #147
sky OOO
Ok, LGTM
4 months, 1 week 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
4 months, 1 week 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
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-18 11:58:06 UTC) #171
chengx (OOO June 26-28)
4 months, 1 week 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 23e94e589