New issue
Advanced search Search tips

Issue 894961 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

touch adjustment range is shrink due to zoom-for-dsf

Project Member Reported by eirage@chromium.org, Oct 12

Issue description

Touch adjustment range on android is suppose to be radius 16 in dip. [1]
After enabling zoom-for-dsf, the radius is 16 physical pixel. which makes it ~5 css pixel, so it feels like disabled on Android.


[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/touch_adjustment.cc?sq=package:chromium&l=47
 
Cc: rbyers@chromium.org nzolghadr@chromium.org
Maybe we need to fix drag DragThreshold too. although mouse is rarely used on Android, other platform might already be affected. 

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/input/mouse_event_manager.cc?sq=package:chromium&dr=CSs&g=0&l=76
Description: Show this description
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e08cca3a55e90a852284e196cb3b1212252d08f3

commit e08cca3a55e90a852284e196cb3b1212252d08f3
Author: Ella Ge <eirage@chromium.org>
Date: Tue Oct 16 21:28:07 2018

Make touch adjustment range upperbound respect zoom_factor

We used to have the touch adjustment range as 32dip in diameter (16dip
in radius) for Android. After the zoom-for-dsf change, the pointerevent
width & height became in physical pixel. It makes the actually range in
dip much smaller on Android.

This CL changes the adjustment range to be 32 CSS pixel, and apply the
zoom_factor before compare with the pointerevent width&height.

On Android, there is no ctrl +/- page zoom, so CSS pixel is dip. There
is no change in behavior.
On Desktop (aura), the adjustment upperbound is affected as it wasn't
consider zooming before but now it does. Therefore, the bound will be
affected by zoom_factor.
On aura, there wasn't an upper bound for adjustment range before the
unified touch adjustment change, which means the adjustment range is
always the pointerevent width & height (physical pixel not changed).
When we zoom in, the upper bound is increased, but the adjustment range
will follow the finger size. So I think it's ok to make the bound
changing with zooming as well on aura.

Bug:  894961 
Change-Id: I466d6b187359f13c82c62860086cb9eeef8cb2a5
Reviewed-on: https://chromium-review.googlesource.com/c/1279204
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600126}
[add] https://crrev.com/e08cca3a55e90a852284e196cb3b1212252d08f3/third_party/WebKit/LayoutTests/fast/hidpi/static/pointerevents/pointerevent_touch-adjustment_click_target.html
[modify] https://crrev.com/e08cca3a55e90a852284e196cb3b1212252d08f3/third_party/blink/renderer/core/input/event_handler.cc
[modify] https://crrev.com/e08cca3a55e90a852284e196cb3b1212252d08f3/third_party/blink/renderer/core/input/pointer_event_manager.cc
[modify] https://crrev.com/e08cca3a55e90a852284e196cb3b1212252d08f3/third_party/blink/renderer/core/page/touch_adjustment.cc
[modify] https://crrev.com/e08cca3a55e90a852284e196cb3b1212252d08f3/third_party/blink/renderer/core/page/touch_adjustment.h

Labels: Merge-Request-71 M-71
Status: Assigned (was: Untriaged)
Requesting merge c#4 to M71.

It's for fixing a regression in M71. The change is small and not risky.
Project Member

Comment 7 by sheriffbot@chromium.org, Oct 17

Labels: -Merge-Request-71 Hotlist-Merge-Approved Merge-Approved-71
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 18

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bff915bdfef66c46742cb1db2df17c59485ae17

commit 9bff915bdfef66c46742cb1db2df17c59485ae17
Author: Ella Ge <eirage@chromium.org>
Date: Thu Oct 18 21:22:49 2018

Make touch adjustment range upperbound respect zoom_factor

We used to have the touch adjustment range as 32dip in diameter (16dip
in radius) for Android. After the zoom-for-dsf change, the pointerevent
width & height became in physical pixel. It makes the actually range in
dip much smaller on Android.

This CL changes the adjustment range to be 32 CSS pixel, and apply the
zoom_factor before compare with the pointerevent width&height.

On Android, there is no ctrl +/- page zoom, so CSS pixel is dip. There
is no change in behavior.
On Desktop (aura), the adjustment upperbound is affected as it wasn't
consider zooming before but now it does. Therefore, the bound will be
affected by zoom_factor.
On aura, there wasn't an upper bound for adjustment range before the
unified touch adjustment change, which means the adjustment range is
always the pointerevent width & height (physical pixel not changed).
When we zoom in, the upper bound is increased, but the adjustment range
will follow the finger size. So I think it's ok to make the bound
changing with zooming as well on aura.

Bug:  894961 
Change-Id: I466d6b187359f13c82c62860086cb9eeef8cb2a5
Reviewed-on: https://chromium-review.googlesource.com/c/1279204
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#600126}(cherry picked from commit e08cca3a55e90a852284e196cb3b1212252d08f3)
Reviewed-on: https://chromium-review.googlesource.com/c/1289681
Reviewed-by: Ella Ge <eirage@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#134}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[add] https://crrev.com/9bff915bdfef66c46742cb1db2df17c59485ae17/third_party/WebKit/LayoutTests/fast/hidpi/static/pointerevents/pointerevent_touch-adjustment_click_target.html
[modify] https://crrev.com/9bff915bdfef66c46742cb1db2df17c59485ae17/third_party/blink/renderer/core/input/event_handler.cc
[modify] https://crrev.com/9bff915bdfef66c46742cb1db2df17c59485ae17/third_party/blink/renderer/core/input/pointer_event_manager.cc
[modify] https://crrev.com/9bff915bdfef66c46742cb1db2df17c59485ae17/third_party/blink/renderer/core/page/touch_adjustment.cc
[modify] https://crrev.com/9bff915bdfef66c46742cb1db2df17c59485ae17/third_party/blink/renderer/core/page/touch_adjustment.h

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1db3dc04678b2815a8e86612961385ac3814ad0a

commit 1db3dc04678b2815a8e86612961385ac3814ad0a
Author: Ella Ge <eirage@chromium.org>
Date: Fri Oct 19 17:00:56 2018

make touch adjustment bound in dip

Previous CL crrev.com/c/1279204 changed the touch adjustment bound to
CSS pixel; however, kMaxAdjustmentSize is a bound on the finger size
in case noisy drivers or errant touches causing degenerate hit-tests
that hit too many elements. The bound should be in dip so it's
corresponding to a physical size.
Therefore, kMaxAdjustmentSize should be in unscaled dip, and we need to
convert it to scaled physical pixel to compare with touch_area, which is
in root frame coordinates (scaled physical pixel).

Bug:  894961 
Change-Id: Ib515e55fd027ba7c17db7488d872be82b91f954a
Reviewed-on: https://chromium-review.googlesource.com/c/1287201
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601192}
[modify] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/input/event_handler.cc
[modify] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/input/pointer_event_manager.cc
[modify] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/layout/hit_test_location.h
[modify] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/page/touch_adjustment.cc
[modify] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/page/touch_adjustment.h
[add] https://crrev.com/1db3dc04678b2815a8e86612961385ac3814ad0a/third_party/blink/renderer/core/page/touch_adjustment_test.cc

Labels: Merge-Request-71
Requesting merge c#9 to M71.

It's a follow up for c#4(merged). It’s a small change with test, should not be risky.
Project Member

Comment 11 by sheriffbot@chromium.org, Oct 20

Labels: -Merge-Request-71 Merge-Approved-71
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 21

Labels: -merge-approved-71
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/faef8fb04d80de35f97990ad373f888896015ba0

commit faef8fb04d80de35f97990ad373f888896015ba0
Author: Ella Ge <eirage@chromium.org>
Date: Sun Oct 21 03:54:39 2018

make touch adjustment bound in dip

Previous CL crrev.com/c/1279204 changed the touch adjustment bound to
CSS pixel; however, kMaxAdjustmentSize is a bound on the finger size
in case noisy drivers or errant touches causing degenerate hit-tests
that hit too many elements. The bound should be in dip so it's
corresponding to a physical size.
Therefore, kMaxAdjustmentSize should be in unscaled dip, and we need to
convert it to scaled physical pixel to compare with touch_area, which is
in root frame coordinates (scaled physical pixel).

Bug:  894961 
Change-Id: Ib515e55fd027ba7c17db7488d872be82b91f954a
Reviewed-on: https://chromium-review.googlesource.com/c/1287201
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#601192}(cherry picked from commit 1db3dc04678b2815a8e86612961385ac3814ad0a)
Reviewed-on: https://chromium-review.googlesource.com/c/1293040
Reviewed-by: Ella Ge <eirage@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#188}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/input/event_handler.cc
[modify] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/input/pointer_event_manager.cc
[modify] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/layout/hit_test_location.h
[modify] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/page/touch_adjustment.cc
[modify] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/page/touch_adjustment.h
[add] https://crrev.com/faef8fb04d80de35f97990ad373f888896015ba0/third_party/blink/renderer/core/page/touch_adjustment_test.cc

Status: Fixed (was: Assigned)
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/9bff915bdfef66c46742cb1db2df17c59485ae17

Commit: 9bff915bdfef66c46742cb1db2df17c59485ae17
Author: eirage@chromium.org
Commiter: eirage@chromium.org
Date: 2018-10-18 21:22:49 +0000 UTC

Make touch adjustment range upperbound respect zoom_factor

We used to have the touch adjustment range as 32dip in diameter (16dip
in radius) for Android. After the zoom-for-dsf change, the pointerevent
width & height became in physical pixel. It makes the actually range in
dip much smaller on Android.

This CL changes the adjustment range to be 32 CSS pixel, and apply the
zoom_factor before compare with the pointerevent width&height.

On Android, there is no ctrl +/- page zoom, so CSS pixel is dip. There
is no change in behavior.
On Desktop (aura), the adjustment upperbound is affected as it wasn't
consider zooming before but now it does. Therefore, the bound will be
affected by zoom_factor.
On aura, there wasn't an upper bound for adjustment range before the
unified touch adjustment change, which means the adjustment range is
always the pointerevent width & height (physical pixel not changed).
When we zoom in, the upper bound is increased, but the adjustment range
will follow the finger size. So I think it's ok to make the bound
changing with zooming as well on aura.

Bug:  894961 
Change-Id: I466d6b187359f13c82c62860086cb9eeef8cb2a5
Reviewed-on: https://chromium-review.googlesource.com/c/1279204
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#600126}(cherry picked from commit e08cca3a55e90a852284e196cb3b1212252d08f3)
Reviewed-on: https://chromium-review.googlesource.com/c/1289681
Reviewed-by: Ella Ge <eirage@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#134}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/faef8fb04d80de35f97990ad373f888896015ba0

Commit: faef8fb04d80de35f97990ad373f888896015ba0
Author: eirage@chromium.org
Commiter: eirage@chromium.org
Date: 2018-10-21 03:54:39 +0000 UTC

make touch adjustment bound in dip

Previous CL crrev.com/c/1279204 changed the touch adjustment bound to
CSS pixel; however, kMaxAdjustmentSize is a bound on the finger size
in case noisy drivers or errant touches causing degenerate hit-tests
that hit too many elements. The bound should be in dip so it's
corresponding to a physical size.
Therefore, kMaxAdjustmentSize should be in unscaled dip, and we need to
convert it to scaled physical pixel to compare with touch_area, which is
in root frame coordinates (scaled physical pixel).

Bug:  894961 
Change-Id: Ib515e55fd027ba7c17db7488d872be82b91f954a
Reviewed-on: https://chromium-review.googlesource.com/c/1287201
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#601192}(cherry picked from commit 1db3dc04678b2815a8e86612961385ac3814ad0a)
Reviewed-on: https://chromium-review.googlesource.com/c/1293040
Reviewed-by: Ella Ge <eirage@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#188}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Sign in to add a comment