Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605193002:
Fix mouse wheel over-scrolls when display is scaled and scroll is paginated (Closed)
DescriptionFix 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
Messages
Total messages: 172 (137 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== temp fix BUG= ========== to ========== Fix mouse wheel over-scrolls when display is scaled BUG=666133 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled In windows, when the windows are 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%. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled In windows, when the windows are 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%. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled In windows, when the windows are 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled In windows, when the windows are 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled In Windows, when the windows are 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled In Windows, when the windows are 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled 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% (display scaling * 100%), where approximately 100% is expected. Basically, the scrolling doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled 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 doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled 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 doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled 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 doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ==========
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chengx@chromium.org changed reviewers: + aelias@chromium.org, brucedawson@chromium.org, bsep@chromium.org
This CL is ready for review. PTAL~
chengx@chromium.org changed reviewers: + esprehn@chromium.org - aelias@chromium.org, brucedawson@chromium.org
aelias@, sorry I just found that you are the owner for Android as indicated in the OWNERS file. Please ignore this CL. Thanks! bsep@ for review please. esprehn@ for both owner and review please. Thank you!
https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:755: #if OS(WIN) Why is this Windows-only? It seems like this would be a problem across platforms. (if it is Windows-only use "#if defined(OS_WIN)" instead) https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:757: float scaleFactor = client()->GetOriginalDeviceScaleFactor(); Use client()->convertViewportToWindow() instead of doing the math here, which should simplify this CL. https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebViewClient.h (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/web/WebViewClient.h:274: float GetOriginalDeviceScaleFactor() const override { return 1.0f; } Be careful adding functions to the public interfaces. I don't think Blink should have access to the device scale factor directly.
aelias@chromium.org changed reviewers: + aelias@chromium.org, brucedawson@chromium.org - esprehn@chromium.org
On 2017/01/05 at 00:45:00, chengx wrote: > aelias@, sorry I just found that you are the owner for > Android as indicated in the OWNERS file. Please ignore this > CL. Thanks! > > bsep@ for review please. > > esprehn@ for both owner and review please. > > Thank you! Well, I'm also a content/renderer OWNER as well as knowledgeable about input and DPI issues, so I'm still better reviewer than esprehn@.
https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:756: if (event.type == WebInputEvent::GestureScrollUpdate) { CC impl thread also has the ability to handle scroll update events directly without ever making it to this method, so it doesn't seem that doing it here can be correct. I'd suggest adjusting the value on the browser process, is there any reason not to do it there? Secondly, this adjustment would apply to all kinds of scroll updates, not just wheel page scrolling. So A) please whether DPI affects non-paginated wheel scroll distance as well (either it's an unnoticed bug and you'd want to fix both, or you'd want to make your fix specific to paginated wheel scroll). And B) make sure that your change doesn't regress touchscreen scrolling.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aelias@ Thanks a lot for being the reviewer of this CL. I am still quite new to Google (2 months), and still sometimes have troubles finding the "right" reviewers. Sorry about this. Please see my inline replies to your comments. PTAL~ https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:755: #if OS(WIN) On 2017/01/05 00:53:29, Bret Sepulveda wrote: > Why is this Windows-only? It seems like this would be a problem across > platforms. > > (if it is Windows-only use "#if defined(OS_WIN)" instead) Okay, I will make it apply to all platforms. https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:756: if (event.type == WebInputEvent::GestureScrollUpdate) { On 2017/01/05 00:55:45, aelias wrote: > CC impl thread also has the ability to handle scroll update events directly > without ever making it to this method, so it doesn't seem that doing it here can > be correct. I'd suggest adjusting the value on the browser process, is there > any reason not to do it there? I have dig into both the compositor thread and the main thread, which turns out to be fairly tricky. For mouse wheel event, only the non-paginated ones are handled in compositor thread, while the paginated ones are handled here. Since this bug is related to paginated wheel scroll, I think this is the right place to make the change. > Secondly, this adjustment would apply to all kinds of scroll updates, not just > wheel page scrolling. So A) please whether DPI affects non-paginated wheel > scroll distance as well (either it's an unnoticed bug and you'd want to fix > both, or you'd want to make your fix specific to paginated wheel scroll). And > B) make sure that your change doesn't regress touchscreen scrolling. A) As mentioned above, since non-paginated wheel scroll is handled in compositor thread, this CL won't affect it at all. This addresses your concern A. B) The mouse wheel event is used to generate a web gesture event, which is handled here. The sourceDevice property of this event is set to "WebGestureDeviceTouchpad". I have added a "if" condition related to this in the new patch, so touchscreen scrolling won't be affected. I believe mouse wheel event is equivalent to scroll event from the touchpad, so it is unnecessary to differentiate these two as mentioned in your concern B. https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:757: float scaleFactor = client()->GetOriginalDeviceScaleFactor(); On 2017/01/05 00:53:29, Bret Sepulveda wrote: > Use client()->convertViewportToWindow() instead of doing the math here, which > should simplify this CL. convertViewportToWindow() takes WebRect as parameter, while WebFloatRec is needed here. Either I need to make a new function to put all the math here, or I will go with the current implementation. https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebViewClient.h (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/web/WebViewClient.h:274: float GetOriginalDeviceScaleFactor() const override { return 1.0f; } On 2017/01/05 00:53:29, Bret Sepulveda wrote: > Be careful adding functions to the public interfaces. I don't think Blink should > have access to the device scale factor directly. Acknowledged.
https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:757: float scaleFactor = client()->GetOriginalDeviceScaleFactor(); On 2017/01/05 04:50:13, chengx wrote: > On 2017/01/05 00:53:29, Bret Sepulveda wrote: > > Use client()->convertViewportToWindow() instead of doing the math here, which > > should simplify this CL. > > convertViewportToWindow() takes WebRect as parameter, while WebFloatRec is > needed here. Either I need to make a new function to put all the math here, or I > will go with the current implementation. I think it would be better to add a version that uses a float rect. The existing function can delegate to the new one and call gfx::ToEnclosedRect on the result.
Sure, no problem, it can pretty nonobvious which reviewer to pick. https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:756: if (event.type == WebInputEvent::GestureScrollUpdate) { >> I'd suggest adjusting the value on the browser process, is there >> any reason not to do it there? > I have dig into both the compositor thread and the main thread, which turns out to be fairly tricky. But I'm proposing you make the change in the browser process (in the environs of RenderWidgetHostImpl, or above), which is code that should be gone through irrespective of whether it's handled on the main or impl side in the renderer process. So you shouldn't have to make the change twice. > For mouse wheel event, only the non-paginated ones are handled in compositor thread I believe it should be the case that non-paginated ones are also handled on the main thread if a Javascript wheel listener is registered. Secondly, I would be curious to learn whether there actually is a bug with non-paginated ones already, since it's a bit suspicious.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Something went wrong with my repo, so I had to git pull, which messed up all the patch-wise comparison in this CL. I removed all old patches, and made a new base with all previous changes on the lasted repo. I pasted all your comments in the new base for record. Extra changes are put in the second patch. > But I'm proposing you make the change in the browser process (in the environs of > RenderWidgetHostImpl, or above), which is code that should be gone through > irrespective of whether it's handled on the main or impl side in the renderer > process. So you shouldn't have to make the change twice. I understand. However, the thing is that non-paginated scrolls don't need this conversion by scale factor. Non-paginated scroll is in units of pixels. For example, one scroll (1 line for example) will move the view by 100 pixels. So if the window is scaled up by 2, then we actually 200 pixels per scroll, which is still 1 line. This also means there is no bug with non-paginated scrolls now. If I make the change in the browser process before these two types of scrolls get handled differently, non-paginated ones will be incorrectly handled. > I believe it should be the case that non-paginated ones are also handled on the > main thread if a Javascript wheel listener is registered. Secondly, I would be > curious to learn whether there actually is a bug with non-paginated ones > already, since it's a bit suspicious. Thanks for the reminder. I didn't know that non-paginated ones are also handled here if a Javascript wheel listener is registered. As mentioned above, since non-paginated scroll doesn't need this conversion, it needs to be separated from paginated ones. In the second patch, I added a condition which basically says that the conversion only applies to the paginated scrolls.
Something went wrong with my repo, so I had to git pull, which messed up all the patch-wise comparison in this CL. I removed all old patches, and made a new base with all previous changes on the lasted repo. I pasted all your comments in the new base for record. Extra changes are put in the second patch. aelias@, I have replied to your comments in a previous email, not inline with the code. Sorry about the confusion. https://codereview.chromium.org/2605193002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2605193002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:754: float scaleFactor = client()->GetOriginalDeviceScaleFactor(); aelias 2017/01/05 00:55:45 CC impl thread also has the ability to handle scroll update events directly without ever making it to this method, so it doesn't seem that doing it here can be correct. I'd suggest adjusting the value on the browser process, is there any reason not to do it there? Secondly, this adjustment would apply to all kinds of scroll updates, not just wheel page scrolling. So A) please whether DPI affects non-paginated wheel scroll distance as well (either it's an unnoticed bug and you'd want to fix both, or you'd want to make your fix specific to paginated wheel scroll). And B) make sure that your change doesn't regress touchscreen scrolling. ===================== chengx: I have dig into both the compositor thread and the main thread, which turns out to be fairly tricky. For mouse wheel event, only the non-paginated ones are handled in compositor thread, while the paginated ones are handled here. Since this bug is related to paginated wheel scroll, I think this is the right place to make the change. A) As mentioned above, since non-paginated wheel scroll is handled in compositor thread, this CL won't affect it at all. This addresses your concern A. B) The mouse wheel event is used to generate a web gesture event, which is handled here. The sourceDevice property of this event is set to "WebGestureDeviceTouchpad". I have added a "if" condition related to this in the new patch, so touchscreen scrolling won't be affected. I believe mouse wheel event is equivalent to scroll event from the touchpad, so it is unnecessary to differentiate these two as mentioned in your concern B. ========== aelias@: But I'm proposing you make the change in the browser process (in the environs of RenderWidgetHostImpl, or above), which is code that should be gone through irrespective of whether it's handled on the main or impl side in the renderer process. So you shouldn't have to make the change twice. I believe it should be the case that non-paginated ones are also handled on the main thread if a Javascript wheel listener is registered. Secondly, I would be curious to learn whether there actually is a bug with non-paginated ones already, since it's a bit suspicious. https://codereview.chromium.org/2605193002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:756: scaledEvent.data.scrollUpdate.deltaY /= scaleFactor; Bret: Use client()->convertViewportToWindow() instead of doing the math here, which should simplify this CL. ========= chengx: convertViewportToWindow() takes WebRect as parameter, while WebFloatRec is needed here. Either I need to make a new function to put all the math here, or I will go with the current implementation. ========= Bret: I think it would be better to add a version that uses a float rect. The existing function can delegate to the new one and call gfx::ToEnclosedRect on the result. https://codereview.chromium.org/2605193002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebViewClient.h (right): https://codereview.chromium.org/2605193002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/web/WebViewClient.h:274: float GetOriginalDeviceScaleFactor() const override { return 1.0f; } Bret: Be careful adding functions to the public interfaces. I don't think Blink should have access to the device scale factor directly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aelias@, gentle ping, PTAL~ Below is what I have investigated to your suggestions. Also a new patch has been uploaded. > But I'm proposing you make the change in the browser process (in environs of > RenderWidgetHostImpl, or above), which is code that should be gone through > irrespective of whether it's handled on the main or impl side in the renderer > process. So you shouldn't have to make the change twice. I understand. However, the thing is that non-paginated scrolls don't need this conversion by scale factor. Non-paginated scroll is in units of pixels. For example, one scroll (1 line for example) will move the view by 100 pixels. So if the window is scaled up by 2, then we actually 200 pixels per scroll, which is still 1 line. This also means there is no bug with non-paginated scrolls now. If I make the change in the browser process before these two types of scrolls get handled differently, non-paginated ones will be incorrectly handled. > I believe it should be the case that non-paginated ones are also handled on > main thread if a Javascript wheel listener is registered.Secondly, I would be > curious to learn whether there actually is a bug with non-paginated ones > already, since it's a bit suspicious. Thanks for the reminder. As mentioned above, since non-paginated scroll doesn't need this conversion, it needs to be separated from paginated ones. In the second patch, I updated WebViewImpl.cpp which basically says that the conversion only applies to the paginated scrolls.
skobes@chromium.org changed reviewers: + skobes@chromium.org
The page scroll distance comes from ScrollableArea::pageStep, which should return physical pixels with --enable-use-zoom-for-dsf (or DIPs otherwise). Is it not doing that? If deltaUnits == Page it doesn't make sense to scale deltaX and deltaY inside the WebGestureEvent; those values should be 1 or -1.
On 2017/01/12 01:29:17, skobes wrote: > The page scroll distance comes from ScrollableArea::pageStep, which should > return physical pixels with --enable-use-zoom-for-dsf (or DIPs otherwise). Is > it not doing that? I would say for page scroll (at least generated from mouse wheel event), it is not in units of physical pixels, but in units of pages. The scroll event is generated in //src/content/browser/renderer_host/input/mouse_wheel_event_queue.cc, around line 90. > If deltaUnits == Page it doesn't make sense to scale deltaX and deltaY inside > the WebGestureEvent; those values should be 1 or -1. When WebGestureEvent is generated in mouse_wheel_event_queue.cc mentioned above, deltaX and deltaY are indeed 1 or -1. But they are no long 1/-1 where I made the if(deltaUnits == Page) change. They have been multiplied by the current scale factors, somewhere I don't know yet. For example, if it is zoomed in by 150%, the deltaX/deltaY values are 1.5/-1.5 now.
On 2017/01/12 01:48:41, chengx wrote: > On 2017/01/12 01:29:17, skobes wrote: > > The page scroll distance comes from ScrollableArea::pageStep, which should > > return physical pixels with --enable-use-zoom-for-dsf (or DIPs otherwise). Is > > it not doing that? > > I would say for page scroll (at least generated from mouse wheel event), it is > not in units of physical pixels, but in units of pages. I'm talking about the return value of ScrollableArea::pageStep, which is based on ScrollableArea::visibleContentRect. > When WebGestureEvent is generated in mouse_wheel_event_queue.cc mentioned above, > deltaX and deltaY are indeed 1 or -1. But they are no long 1/-1 where I made the > if(deltaUnits == Page) change. I understand, but I'm saying we shouldn't do it that way. Instead we should ensure that a WebGestureEvent with deltaY == 1 and deltaUnits == Page produces the correct behavior from ScrollManager::handleGestureScrollEvent.
On 2017/01/12 01:48:41, chengx wrote: > When WebGestureEvent is generated in mouse_wheel_event_queue.cc mentioned above, > deltaX and deltaY are indeed 1 or -1. But they are no long 1/-1 where I made the > if(deltaUnits == Page) change. They have been multiplied by the current scale > factors, somewhere I don't know yet. For example, if it is zoomed in by 150%, > the > deltaX/deltaY values are 1.5/-1.5 now. Oh I might have misread what you said here. Even before your change, the deltaX/deltaY values are 1.5/-1.5 in WebViewImpl::handleGestureEvent? In that case it seems like the bug is wherever they are getting scaled.
On 2017/01/12 02:03:12, skobes wrote: > On 2017/01/12 01:48:41, chengx wrote: > > When WebGestureEvent is generated in mouse_wheel_event_queue.cc mentioned > above, > > deltaX and deltaY are indeed 1 or -1. But they are no long 1/-1 where I made > the > > if(deltaUnits == Page) change. They have been multiplied by the current scale > > factors, somewhere I don't know yet. For example, if it is zoomed in by 150%, > > the > > deltaX/deltaY values are 1.5/-1.5 now. > > Oh I might have misread what you said here. Even before your change, the > deltaX/deltaY values are 1.5/-1.5 in WebViewImpl::handleGestureEvent? > > In that case it seems like the bug is wherever they are getting scaled. Yes, they just got scaled somewhere even before my change, which I feel more like a bug than intended.
On 2017/01/12 01:29:17, skobes wrote: > The page scroll distance comes from ScrollableArea::pageStep, which should > return physical pixels with --enable-use-zoom-for-dsf (or DIPs otherwise). Is > it not doing that? Also, I tested that ScrollableArea::pageStep() is executed after WebViewImpl::handleGestureEvent(), so the issue is simply deltaX/deltaY got scaled somewhere.
On 2017/01/12 at 01:48:41, chengx wrote: > On 2017/01/12 01:29:17, skobes wrote: > > The page scroll distance comes from ScrollableArea::pageStep, which should > > return physical pixels with --enable-use-zoom-for-dsf (or DIPs otherwise). Is > > it not doing that? > > I would say for page scroll (at least generated from mouse wheel event), it is > not in units of physical pixels, but in units of pages. The scroll event is > generated in //src/content/browser/renderer_host/input/mouse_wheel_event_queue.cc, > around line 90. OK, thanks for the additional detail. In that case, since we still want to scroll by exactly 1 page, it doesn't make sense to me that we would adjust the amount to a value other than 1. It looks like the final page step size in pixels is computed in the following code: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scrol... , how about applying the adjustment there instead? Could you add a unit test for this? I suggest adding a new C++ test in WebViewTest.cpp.
Err, I didn't read that skobes@ already suggested the exact same thing, sorry. If it's true that: > Yes, they just got scaled somewhere even before my change, which I feel more like a bug than intended. Then please figure out the place that scaling happens and remove it, instead of counter-adjusting later. (If that winds up outside of Blink, it doesn't necessarily need a unit test.)
Patchset #3 (id:260001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled 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 doesn't take display scaling into account and makes mouse wheel scrolling unusable. This CL fixes this bug via upgrading WebViewImpl::handleGestureEvent(), which takes display scaling into account for rendering. BUG=666133 ========== to ========== Fix mouse wheel over-scrolls when display is scaled and scroll is set to 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 ==========
chengx@chromium.org changed reviewers: - brucedawson@chromium.org
Hi aelias@ bsep@ skobes@, it has been a little while since the last update I made to this CL. I was busy with other stuff. Sorry about this. I figured out the place where the unnecessary scaling happens and removed it in the new patch. Also validated locally and the issue is fixed. PTAL~ Thanks! aelias@ for file owner please. On 2017/01/12 04:01:58, aelias wrote: > Err, I didn't read that skobes@ already suggested the exact same thing, sorry. > If it's true that: > > > Yes, they just got scaled somewhere even before my change, which I feel more > like a bug than intended. > > Then please figure out the place that scaling happens and remove it, instead of counter-adjusting later. (If that winds up outside of Blink, it doesn't necessarily need a unit test.)
Description was changed from ========== Fix mouse wheel over-scrolls when display is scaled and scroll is set to 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 ========== to ========== 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 ==========
On 2017/02/16 19:07:37, chengx wrote: > Hi aelias@ bsep@ skobes@, it has been a little while since the last update I > made to this CL. I was busy with other stuff. Sorry about this. > > I figured out the place where the unnecessary scaling happens and removed it in > the new patch. Also validated locally and the issue is fixed. > > PTAL~ Thanks! > > aelias@ for file owner please. Patchset 4 looks good, but please add a test to blink_event_util_unittest.cc.
Fixing it in that spot looks appropriate, thanks. https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:861: crbug.com/666133 virtual/scalefactor200withzoom/fast/hidpi/static/gesture-scroll-amount.html [ NeedsRebaseline ] It doesn't make sense that this test result would have changed. It's clearly intended to scroll by pixel amounts. It must be a preexisting problem with the test that deltaHintUnits is incorrect set to Page. Instead of changing the expectations to be wrong, please change the event sender framework to set deltaHintUnits by pixel. https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:862: crbug.com/666133 virtual/scalefactor200withzoom/fast/hidpi/static/mousewheel-scroll-amount.html [ NeedsRebaseline ] Since these don't have any image expectations, please directly include the rebaselined result in this patch instead of relying on rebaseline bot. This makes it easier to examine what changed and also simplifies cherry-picks/reverts. (Although, I suspect there shouldn't be any result changes needed at all if the test framework is changed to configure the events properly by pixel.O) https://codereview.chromium.org/2605193002/diff/300001/ui/events/blink/blink_... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2605193002/diff/300001/ui/events/blink/blink_... ui/events/blink/blink_event_util.cc:757: wheel_event->wheelTicksX *= scale; I think wheel tick count should be within the if block as well.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:340001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #5 (id:320001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:360001) has been deleted
Patchset #5 (id:380001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:400001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:420001) has been deleted
chengx@chromium.org changed reviewers: + sky@chromium.org
Hi aelias@, it turns out I don't need to modify file TestExpectations at all. The two lines added and marked as "NeedsRebaseline" in file TestExpectations are necessary for the first two patch sets. This may indicate the first two patch sets have flaws. I removed all the changes in TestExpectations in the new patch set. Hi bsep@, I have added the unit tests in the new patch set. Adding sky@ to cover the newly added files ui/events/gesture_event_details.*. PTAL~ https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:861: crbug.com/666133 virtual/scalefactor200withzoom/fast/hidpi/static/gesture-scroll-amount.html [ NeedsRebaseline ] On 2017/02/16 22:11:13, aelias wrote: > It doesn't make sense that this test result would have changed. It's clearly > intended to scroll by pixel amounts. It must be a preexisting problem with the > test that deltaHintUnits is incorrect set to Page. Instead of changing the > expectations to be wrong, please change the event sender framework to set > deltaHintUnits by pixel. Acknowledged. https://codereview.chromium.org/2605193002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:862: crbug.com/666133 virtual/scalefactor200withzoom/fast/hidpi/static/mousewheel-scroll-amount.html [ NeedsRebaseline ] On 2017/02/16 22:11:13, aelias wrote: > Since these don't have any image expectations, please directly include the > rebaselined result in this patch instead of relying on rebaseline bot. This > makes it easier to examine what changed and also simplifies > cherry-picks/reverts. (Although, I suspect there shouldn't be any result > changes needed at all if the test framework is changed to configure the events > properly by pixel.O) Acknowledged. https://codereview.chromium.org/2605193002/diff/300001/ui/events/blink/blink_... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2605193002/diff/300001/ui/events/blink/blink_... ui/events/blink/blink_event_util.cc:757: wheel_event->wheelTicksX *= scale; On 2017/02/16 22:11:13, aelias wrote: > I think wheel tick count should be within the if block as well. Done.
https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:20: enum ScrollUnits { PrecisePixels = 0, Pixels, Page }; chrome style is ALL_CAPS. Also, use enum class. Also, please document what these values mean. It isn't at all clear what differentiates 'precise' from non-precise. https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:65: ScrollUnits scroll_units_hint() const { scroll_begin_units? https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:80: ScrollUnits scroll_units() const { scroll_update_units? https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:179: ScrollUnits deltaHintUnits; delta_hint_units. https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:185: ScrollUnits deltaUnits; delta_units.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi sky@, I have addressed your comments in the new patch set. PTAL~ https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:20: enum ScrollUnits { PrecisePixels = 0, Pixels, Page }; On 2017/02/17 18:00:25, sky wrote: > chrome style is ALL_CAPS. Also, use enum class. > > Also, please document what these values mean. It isn't at all clear what > differentiates 'precise' from non-precise. I copied this enum definition from WebGestureEvent.h in webKit, because WebGestureEvent.h is not allowed to be included here. In more details, git cl upload cannot be completed due to this style issue. Anyway, I have added comments, switched to ALL_CAPS and enum class. https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:65: ScrollUnits scroll_units_hint() const { On 2017/02/17 18:00:25, sky wrote: > scroll_begin_units? Done. https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:80: ScrollUnits scroll_units() const { On 2017/02/17 18:00:25, sky wrote: > scroll_update_units? Done. https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:179: ScrollUnits deltaHintUnits; On 2017/02/17 18:00:25, sky wrote: > delta_hint_units. Done. https://codereview.chromium.org/2605193002/diff/440001/ui/events/gesture_even... ui/events/gesture_event_details.h:185: ScrollUnits deltaUnits; On 2017/02/17 18:00:25, sky wrote: > delta_units. Done.
lgtm
lgtm otherwise https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... ui/events/blink/blink_event_util.cc:759: if (!wheel_event->scrollByPage) { Test for this too? https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... File ui/events/blink/blink_event_util_unittest.cc (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... ui/events/blink/blink_event_util_unittest.cc:43: EXPECT_EQ(1, gestureEvent->data.scrollUpdate.deltaY); nit: it's probably worth adding the one line to check deltaX too (and below). https://codereview.chromium.org/2605193002/diff/460001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/gesture_even... ui/events/gesture_event_details.h:33: ScrollUnits units = ScrollUnits::PRECISE_PIXELS); nit: add a short comment saying this is the default in WebGestureEvent too
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated unittests in the new patch set, as suggested by bsep@ https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... ui/events/blink/blink_event_util.cc:759: if (!wheel_event->scrollByPage) { On 2017/02/17 21:06:03, Bret Sepulveda wrote: > Test for this too? Yep, added. https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... File ui/events/blink/blink_event_util_unittest.cc (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/blink/blink_... ui/events/blink/blink_event_util_unittest.cc:43: EXPECT_EQ(1, gestureEvent->data.scrollUpdate.deltaY); On 2017/02/17 21:06:03, Bret Sepulveda wrote: > nit: it's probably worth adding the one line to check deltaX too (and below). Added checks for deltaX. Also, I should use scrollBegin* here. It is fixed now. https://codereview.chromium.org/2605193002/diff/460001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/460001/ui/events/gesture_even... ui/events/gesture_event_details.h:33: ScrollUnits units = ScrollUnits::PRECISE_PIXELS); On 2017/02/17 21:06:03, Bret Sepulveda wrote: > nit: add a short comment saying this is the default in WebGestureEvent too Done.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #7 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_even... ui/events/gesture_event_details.h:24: PIXELS, // large pixel jump duration; should animate to delta. Are you sure this isn't DIPs and what you are calling PRECISE_PIXELS is pixels?
Hi sky@, please see my reply below. https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2605193002/diff/500001/ui/events/gesture_even... ui/events/gesture_event_details.h:24: PIXELS, // large pixel jump duration; should animate to delta. On 2017/02/17 22:52:31, sky wrote: > Are you sure this isn't DIPs and what you are calling PRECISE_PIXELS is pixels? I agree the names here, which originate from WebGestureEvent.h, is confusing. Here is the thing. We didn't have ScrollUnits here for 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?type... WebGestureEvent used the default value for the scroll 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 and some try bots failed, which is expected because of inconsistency. The names in the enum here are consistent with the ones in WebGestureEvent.h, which have been used in a lot of places throughout chrome. If you think we should rename them, probably we need to do it in another CL.
Ok, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, bsep@chromium.org Link to the patchset: https://codereview.chromium.org/2605193002/#ps500001 (title: "More unittests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1487400995046300,
"parent_rev": "f392d837d227d79a9b8dec1d298e9c562929c7cf", "commit_rev":
"9c53350ce5b4233fedd89b6593451df30539b920"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/9c53350ce5b4233fedd89b659345... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:500001) as https://chromium.googlesource.com/chromium/src/+/9c53350ce5b4233fedd89b659345...
Message was sent while issue was closed.
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?
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
