Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 248522 Touch handlers aren't always invoked for elements in scrolled or transformed composited layers
Starred by 16 users Project Member Reported by rbyers@chromium.org, Jun 11 2013 Back to list
Status: Verified
Owner: rbyers@chromium.org
Closed: Jul 2013
Cc: trchen@chromium.org, klo...@chromium.org, vollick@chromium.org, le...@chromium.org, yus...@chromium.org, sadrul@chromium.org, aelias@chromium.org, egraether@chromium.org, johnmccutchan@chromium.org
Components:
OS: Android, Chrome
Pri: 1
Type: Bug-Regression


Sign in to add a comment
Chrome Version       : 28.0.1500.35
OS Version: 4100.38.3

What steps will reproduce the problem?
1. Open http://jsbin.com/apudet/1
2. Touch on the 'touch me' box - verify the touch event handlers get invoked (changes color, prevents scrolling)
3. Now scroll the div up so that the 'touch me' box is now at the top of the visible area
4. Touch in the bottom half of the 'touch me' box and drag

What is the expected result?
Expect the touch handlers to get invoked and no scrolling to occur

What happens instead of that?
The div scrolls as if I was touching an area without touch handlers

When compositor thread hit testing was added, it was done using a list of rects in document co-ordinates that are updated only on layout (see http://www.chromium.org/developers/design-documents/compositor-hit-testing).  This means that when a div scrolls, the hit test rects are incorrect, and the impl thread may not send touch events onto the main thread when it should.

This probably regressed in Chrome desktop M26 (when hit testing was turned on for CrOS) and I think it was M25 for Android.  I'm actually shocked this hasn't shown up as a problem - I'd expect this not to be uncommon.  I guess touch handlers in scrollable elements must be pretty rare, or sites that have them tend to have touch handlers on the whole document.

See also issue 248255 which is a similar problem with the non-fast-scroll regions.

I think the right fix is to move the hit test rects to be per-layer (eg. on RenderLayerBacking instead of ScrollingCoordinator).  I'll start looking into that, but it's probably more work than can be done for M29.  

aelias/yusufo/leviw: the short term fix is to recompute the rects on every scroll.  Is that even practical from a perf perspective?  If users haven't been complaining about this then perhaps we should just solve it the right way?  Let me know what you think should be done for Chrome Android in the short term...
 
Comment 1 by rbyers@chromium.org, Jun 11 2013
Blocking: chromium:248255
Comment 2 by aelias@chromium.org, Jun 11 2013
The original plan was to put them on layers and I'm surprised they're not, we should definitely put them on layers.

For Android, we don't want to recompute them on every scroll.  That wouldn't be a correct fix anyway since they'd still be out of date while the main thread hasn't heard about the scroll yet.  I suggest deactivating the feature on ChromeOS for M29 if we can't get a fix in time.
Comment 3 by rbyers@chromium.org, Jun 11 2013
Labels: -M-29 M-30 Iteration-84
Owner: rbyers@chromium.org
Status: Started
Talked more with aelias.  We've both been shipping with this bug for quite awhile - since we haven't heard complaints it must not be that urgent (sites that have such touch handlers probably tend to have handlers on the document as well, effectively disabling compositor hit testing).

So I'll work (with vollick@) on getting the right long term fix, probably in M-30.
Comment 4 by rbyers@chromium.org, Jun 13 2013
Note that if layout changes, then the rects are recomputed taking the current scroll offsets into account.  So this might also explain why we don't see the problem in practice as much as I had expected.

There's another related bug with iframes - their scroll offset isn't taken into account when computing the absolute hit rect location.  So any rects inside of iframes will be off, even after a relayout.
Comment 5 by rbyers@chromium.org, Jun 21 2013
Initial in-progress CL is here: https://codereview.chromium.org/17471008
Comment 6 by rbyers@chromium.org, Jun 24 2013
Summary: Touch handlers aren't always invoked for elements in scrolled or transformed divs (was: Touch handlers aren't invoked for elements in scrolled divs)
Updating subject to reflect the larger scope (hit testing is broken in a variety of ways due to being absolute instead of per-layer).  

Note that in all cases you can work around the problem by either:
1. putting a touch event handler on the document (generally a bad idea because it breaks threading scrolling), or
2. forcing a relayout after any change in handler location that isn't accounted for (eg. when an element is transformed or scrolled).

Neither work-around is good for performance, so it's important we get this fixed in the browser asap. 
Comment 7 by rbyers@chromium.org, Jun 24 2013
Blocking: chromium:253456
Comment 8 by rbyers@chromium.org, Jun 25 2013
Labels: Iteration-85
Comment 9 by rbyers@chromium.org, Jun 25 2013
Blocking: chromium:178281
Blocking: chromium:241964
Cc: johnmccutchan@chromium.org klo...@chromium.org
Issue 180720 has been merged into this issue.
Labels: Iteration-86
Blocking: chromium:261307
Summary: Touch handlers aren't always invoked for elements in scrolled or transformed composited layers (was: Touch handlers aren't always invoked for elements in scrolled or transformed divs)
https://codereview.chromium.org/17471008/ handles all the scenarios involving composited layers (the top priority), while also handling non-composited scrolling/trainsforms expect when they update without relayout.  I filed issue 261307 to handle those separately.
Issue 253456 has been merged into this issue.
Blocking: -chromium:253456
Cc: egraether@chromium.org
Note that there is a new feature in canary that let's you visualize the touch hit rects - see issue 253552.  With this you can see when the touch hit rect isn't tracking with an element that has a touch handler on it.  Eg. see screenshot for the simple repro above (http://jsbin.com/apudet/1)
Screenshot 2013-07-19 at 2.59.42 PM.png
45.1 KB View Download
Project Member Comment 18 by bugdroid1@chromium.org, Jul 21 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=154611

------------------------------------------------------------------------
r154611 | chrome-bot@google.com | 2013-07-21T13:15:11.513682Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-mac/fast/events?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-win/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderText.h?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/LayerRect.h?r1=154611&r2=154610&pathrev=154611
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/touch-hit-rects-in-iframe-expected.txt?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBoxModelObject.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderObject.cpp?r1=154611&r2=154610&pathrev=154611
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/frame-with-touch-handler.html?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects-iframe-nested.html?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/linux/fast/events/touch/compositor-touch-hit-rects-expected.txt?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/svg/RenderSVGModelObject.h?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/LayerRectList.idl?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects-iframes-expected.txt?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-win/fast/events?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-linux/fast/events?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderLayer.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderInline.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderLayerModelObject.cpp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/LayerRectList.cpp?r1=154611&r2=154610&pathrev=154611
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/touch-hit-rects-in-iframe.html?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/page/scrolling/ScrollingCoordinator.h?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects-iframes.html?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/Internals.h?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-win/fast?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderView.cpp?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderObject.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/Internals.idl?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects-global-expected.txt?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-linux/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBox.cpp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-mac/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderLayerModelObject.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBlock.cpp?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/page/scrolling/ScrollingCoordinator.cpp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/LayerRectList.h?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-win/fast/events/touch?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/Internals.cpp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gypi?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-linux/fast/events/touch?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/fast/events/touch/compositor-touch-hit-rects-expected.txt?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects-global.html?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-mac?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderView.h?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/LayerRect.idl?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderText.cpp?r1=154611&r2=154610&pathrev=154611
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/frame-with-document-touch-handler.html?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gyp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects-iframe.html?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects-iframe-doc.html?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBox.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBlock.h?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBoxModelObject.cpp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-mac/fast/events/touch?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/svg/RenderSVGModelObject.cpp?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects-scroll.html?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-linux/fast?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-mac/fast?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.css?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/events/touch/compositor-touch-hit-rects-expected.txt?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-win?r1=154611&r2=154610&pathrev=154611
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/chromium-linux?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderInline.cpp?r1=154611&r2=154610&pathrev=154611
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/events/touch/compositor-touch-hit-rects.html?r1=154611&r2=154610&pathrev=154611

Rework compositor touch hit testing

Previously compositor thread hit-testing was done by sending a list of absolute rects to the compositor on layout (see www.chromium.org/developers/design-documents/compositor-hit-testing for details).  This is broken whenever the position of an element can change without causing layout (scrolling, transforms, etc.), and results in touch event handlers not getting invoked when they should.  To fix this we need to track the hit-test rects on a layer-by-layer basis, and this CL is the first big step in that direction.
 
I started with the per-layer approach LeviW@ first tried here: https://bugs.webkit.org/show_bug.cgi?id=103914, it was rejected from WebKit because it was too different from what iOS was doing, but is a perfectly reasonable approach for Blink.  I modified this original patch in a number of ways, including:
 - tracking rects per enclosing RenderLayer (instead of the enclosing composited layer), and then later projecting those rects onto the appropriate composited layer.
 - breaking up the rect accumulation into multiple methods to eliminate code duplication and simplify some things
 - disabling the mechanism when touch events aren't supported (doesn't affect devtools touch emulation) or when the page isn't composited
 - fixing a bunch of bugs (see new tests below)
 - short circuiting some common cases to make them faster
 - accumulate LayoutRects instead of just IntRects
 - exposing the per-layer details to the tests

This doesn't yet handle scrolling (or changing the transform) of non-composited layers, but that was already broken so this shouldn't make it any worse (and now handles the case when the layers are composited).  I'll work on this case in a follow-on CL - this CL is big enough already.

This change reworks the testing infrastructure to make it possible to test bugs with when hit test rect update happens.  Rather than freshly compute the hit test rects whenever the test asks for them, we instead register Internals to be notified by ScrollingCoordinator whenever the rects change. This allows us to verify that the rects are getting updated when they should, and also that we aren't getting unexpected updates. 

This change also adds several additional test cases, including: 
- touch handlers inside of scrolled divs (composited and non-composited)
- clipping behavior
- 2d (non-composited) transforms
- svg
- that changing style causes the rects to be updated
- global handlers on the html and body elements
- additional iframe test cases, including non-composited transforms of iframes

Also in order to make validating the test results easier, I've put a 'visualize' mode into the tests which will overlay the actual rects on top of the elements being tested. See http://goo.gl/ObBry for the current results (when running in content-shell interactively, which disables the use of the Ahem font to make the text readable). 

BUG= 248522 

Review URL: https://chromiumcodereview.appspot.com/17471008
------------------------------------------------------------------------
Status: Fixed
Status: Verified
ChromeOS: 4443.0.0 | 30.0.1573.2 | link
Comment 22 by hasat...@gmail.com, Jul 29 2013
Glad this is getting fixed! As noted in issue 180720, neither of the hacks work anymore. Is there some other workaround that can be applied in the meantime?
Sorry for the delay.  Forcing layout or adding a handler to the document definitely both still work as workarounds.  In issue 180720 there is discussion of iOS, is that where you are testing this?  Chrome on iOS doesn't use blink (or even chromium's old build of WebKit), but Apple's private iOS WebKit fork.  So anything around this area on iOS is a completely different story you'd have to talk to Apple about.
Labels: -Cr-UI-Input-Touch-Screen Cr-Internals-Input-Touch-Screen
Issue 253456 has been merged into this issue.
Sign in to add a comment