New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 671232 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocked on:
issue 644318



Sign in to add a comment

Touch rects aren't updated when touch event feature detection is disabled

Project Member Reported by mustaq@chromium.org, Dec 5 2016

Issue description

ScrollingCoordinator::updateTouchEventTargetRectsIfNeeded() currently doesn't update the touch rects when TouchEvent feature detection is disabled. This could bad scrolling with a touch-action:none region.

Always updating the rects should correct the problem but could be bad for scrolling performance.
 
Blockedon: 644318
Cc: sunyunjia@chromium.org aelias@chromium.org
Summary: Touch rects aren't updated when touch event feature detection is disabled (was: Update touch rects regardless of the touch event flag)
Updating summary to reflect the problem.  In particular:
1. Start Chrome on a touchscreen device with --touch-events=disabled (or auto without a touchscreen attached on startup)
2. Try scrolling over a touch-action:none region or div with a touchstart handler that always calls preventDefault

The page should not scroll but will.  Related to the work in  issue 644318 .

Fundamentally the touch rects are a performance tradeoff - doing more work at layout time in order to allow scrolling to start quickly more often.  For details on the touch rect design see: https://www.chromium.org/developers/design-documents/compositor-hit-testing

I think there are two options here:

1) Always update touch rects regardless of whether a touchscreen is present or not as Mustaq suggests.  This will fix the above issue but incur a cost on every layout which will almost never have any benefit.

2) Disable the touch rect scrolling optimization whenever touch event API feature detection is disabled.  This will probably be a better overall performance tradeoff (since it's unlikely that touch scrolling will occur when the touch feature detect is disabled) but may make it harder for developers to reason about performance and the behavior of TouchEvent.cancelable.

I _think_ I prefer option 1 since it's more predictable - if the overhead at layout time is too much then we should be fixing that anyway (we're already paying the cost where it hurts most - on Android).  But it's worth being aware of the perf tradeoff here and keeping our eyes out for any reports of regressions.

dtapuska/aelias any thoughts?
I personally would go with 1. I prefer consistency and yes we are debating about doing this on *desktop* platforms and we already are doing it on mobile platforms.

Granted the page set could be different but my guess is that we likely won't notice it.
Yeah, I favor option 1 as well.
Owner: sunyunjia@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 15 2016

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

commit 4436064358a762936f8da853fc1271e036b96491
Author: sunyunjia <sunyunjia@chromium.org>
Date: Thu Dec 15 18:15:56 2016

Remove all refs to touchEventFeatureDetection in ScrollingCoordinator.

ScrollingCoordinator::updateTouchEventTargetRectsIfNeeded() currently doesn't
update the touch rects when TouchEvent feature detection is disabled. This
could be bad when scrolling with a touch-action:none region. Since we are
always firing touch events, we can remove all refs to
touchEventFeatureDetection in ScrollingCoordinator.

BUG= 671232 

Review-Url: https://codereview.chromium.org/2582473003
Cr-Commit-Position: refs/heads/master@{#438875}

[modify] https://crrev.com/4436064358a762936f8da853fc1271e036b96491/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment