Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(68)

Issue 17471008: Rework compositor touch hit testing (Closed)

Created:
7 years, 6 months ago by Rick Byers
Modified:
7 years, 4 months ago
CC:
blink-reviews, kenneth.christiansen, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, trchen, Yusuf, Ian Vollick
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154611

Patch Set 1 #

Patch Set 2 : Clean up a few things, fix continuations and SVG #

Patch Set 3 : Fix iframes and add some polish #

Patch Set 4 : Retry borked upload #

Patch Set 5 : Add dummy expectations for other platforms #

Patch Set 6 : Various fixes and test additions #

Total comments: 3

Patch Set 7 : Tweak comments based on CR from leviw #

Patch Set 8 : Tweak comment again #

Patch Set 9 : Support non-composited transforms #

Patch Set 10 : A few more tweaks #

Patch Set 11 : Small simplificaiton in document traversal logic #

Patch Set 12 : Tweak comment and remove unnecessary include #

Patch Set 13 : Fix 0-height body case #

Patch Set 14 : Fix 0-height body case #

Patch Set 15 : Fix lifetime issue with Internals::m_currentTouchEventRects #

Total comments: 16

Patch Set 16 : Leviw CR feedback and license header updates #

Total comments: 1

Patch Set 17 : CR feedback - accumulate LayoutRects instead of IntRects, disable when page isn't composited. Also… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1284 lines, -352 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/fast/events/touch/compositor-touch-hit-rects.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +117 lines, -97 lines 0 comments Download
A LayoutTests/fast/events/touch/compositor-touch-hit-rects-global.html View 1 2 3 4 5 6 7 8 1 chunk +44 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/compositor-touch-hit-rects-global-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/compositor-touch-hit-rects-iframes.html View 1 2 3 4 5 6 7 8 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/compositor-touch-hit-rects-iframes-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/compositor-touch-hit-rects-scroll.html View 1 2 3 4 5 1 chunk +97 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.css View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js View 1 2 3 4 5 1 chunk +129 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects-iframe.html View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects-iframe-doc.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +35 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects-iframe-nested.html View 1 2 1 chunk +36 lines, -0 lines 0 comments Download
D LayoutTests/fast/events/touch/resources/frame-with-document-touch-handler.html View 1 2 1 chunk +0 lines, -7 lines 0 comments Download
D LayoutTests/fast/events/touch/resources/frame-with-touch-handler.html View 1 2 1 chunk +0 lines, -8 lines 0 comments Download
D LayoutTests/fast/events/touch/touch-hit-rects-in-iframe.html View 1 2 1 chunk +0 lines, -58 lines 0 comments Download
D LayoutTests/fast/events/touch/touch-hit-rects-in-iframe-expected.txt View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
A LayoutTests/platform/chromium-linux/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/platform/chromium-mac/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/platform/chromium-win/fast/events/touch/compositor-touch-hit-rects-scroll-expected.txt View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M LayoutTests/platform/linux/fast/events/touch/compositor-touch-hit-rects-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +51 lines, -15 lines 0 comments Download
M LayoutTests/platform/mac/fast/events/touch/compositor-touch-hit-rects-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +49 lines, -15 lines 0 comments Download
M LayoutTests/platform/win/fast/events/touch/compositor-touch-hit-rects-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +49 lines, -15 lines 0 comments Download
M Source/core/core.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -0 lines 0 comments Download
Source/core/page/scrolling/ScrollingCoordinator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +13 lines, -3 lines 0 comments Download
M Source/core/page/scrolling/ScrollingCoordinator.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +129 lines, -62 lines 0 comments Download
M Source/core/rendering/RenderBlock.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
Source/core/rendering/RenderBlock.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +16 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBox.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +4 lines, -1 line 0 comments Download
M Source/core/rendering/RenderBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +12 lines, -0 lines 0 comments Download
Source/core/rendering/RenderBoxModelObject.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
Source/core/rendering/RenderBoxModelObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +12 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderInline.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
Source/core/rendering/RenderInline.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +27 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderLayer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
Source/core/rendering/RenderLayerModelObject.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderLayerModelObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +12 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderObject.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +17 lines, -1 line 0 comments Download
Source/core/rendering/RenderObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +54 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderText.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderText.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
Source/core/rendering/RenderView.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderView.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +25 lines, -3 lines 0 comments Download
M Source/core/rendering/svg/RenderSVGModelObject.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/rendering/svg/RenderSVGModelObject.cpp View 1 2 3 4 5 6 7 8 2 chunks +12 lines, -0 lines 0 comments Download
M Source/core/testing/Internals.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +11 lines, -2 lines 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +40 lines, -11 lines 0 comments Download
M Source/core/testing/Internals.idl View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments Download
A + Source/core/testing/LayerRect.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +19 lines, -13 lines 0 comments Download
A + Source/core/testing/LayerRect.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -6 lines 0 comments Download
A + Source/core/testing/LayerRectList.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +15 lines, -13 lines 0 comments Download
A + Source/core/testing/LayerRectList.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +20 lines, -9 lines 0 comments Download
A + Source/core/testing/LayerRectList.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -6 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
Rick Byers
Hey Levi, I've finally got an initial crack at per-layer hit testing done, can you ...
7 years, 5 months ago (2013-06-28 03:03:00 UTC) #1
leviw_travelin_and_unemployed
I think you've got a good start. We do need to think of a good ...
7 years, 5 months ago (2013-07-10 01:51:16 UTC) #2
Rick Byers
On 2013/07/10 01:51:16, Levi wrote: > I think you've got a good start. We do ...
7 years, 5 months ago (2013-07-10 14:53:12 UTC) #3
Rick Byers
I've got non-compostied transforms (and even non-composited transformed iframes, which was a real pain) working ...
7 years, 5 months ago (2013-07-16 19:40:46 UTC) #4
Rick Byers
On 2013/07/10 14:53:12, Rick Byers wrote: > On 2013/07/10 01:51:16, Levi wrote: > https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/RenderView.cpp#newcode355 > ...
7 years, 5 months ago (2013-07-18 01:36:48 UTC) #5
leviw_travelin_and_unemployed
https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrolling/ScrollingCoordinator.cpp File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode325 Source/core/page/scrolling/ScrollingCoordinator.cpp:325: continue; Does this happen? When? https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode520 Source/core/page/scrolling/ScrollingCoordinator.cpp:520: // Examine ...
7 years, 5 months ago (2013-07-18 18:08:38 UTC) #6
Rick Byers
Thanks Levi. https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrolling/ScrollingCoordinator.cpp File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode520 Source/core/page/scrolling/ScrollingCoordinator.cpp:520: // Examine every target in detail. On ...
7 years, 5 months ago (2013-07-18 19:58:51 UTC) #7
leviw_travelin_and_unemployed
https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/RenderBlock.h File Source/core/rendering/RenderBlock.h (right): https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/RenderBlock.h#newcode571 Source/core/rendering/RenderBlock.h:571: virtual void computeOwnHitTestRects(Vector<IntRect>&, const LayoutPoint& layerOffset) const OVERRIDE; On ...
7 years, 5 months ago (2013-07-18 20:21:48 UTC) #8
leviw_travelin_and_unemployed
https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrolling/ScrollingCoordinator.cpp File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode324 Source/core/page/scrolling/ScrollingCoordinator.cpp:324: // This page may not have compositing enabled at ...
7 years, 5 months ago (2013-07-18 20:23:03 UTC) #9
Rick Byers
On 2013/07/18 20:23:03, Levi wrote: > https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrolling/ScrollingCoordinator.cpp > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode324 > ...
7 years, 5 months ago (2013-07-18 20:55:49 UTC) #10
leviw_travelin_and_unemployed
On 2013/07/18 20:55:49, Rick Byers wrote: > On 2013/07/18 20:23:03, Levi wrote: > > > ...
7 years, 5 months ago (2013-07-18 20:57:44 UTC) #11
Rick Byers
Thanks Levi. CL updated (you can just review the diff from the previous patchset). > ...
7 years, 5 months ago (2013-07-18 22:20:32 UTC) #12
leviw_travelin_and_unemployed
Now with unnamed namespaces! if you add a goto I think we've nearly covered everything ...
7 years, 5 months ago (2013-07-19 17:02:49 UTC) #13
Rick Byers
On 2013/07/19 17:02:49, Levi wrote: > Now with unnamed namespaces! if you add a goto ...
7 years, 5 months ago (2013-07-19 17:37:48 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108
7 years, 5 months ago (2013-07-19 18:05:58 UTC) #15
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=193
7 years, 5 months ago (2013-07-20 00:54:13 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108
7 years, 5 months ago (2013-07-20 00:59:49 UTC) #17
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=264
7 years, 5 months ago (2013-07-20 09:07:19 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108
7 years, 5 months ago (2013-07-20 16:58:55 UTC) #19
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=333
7 years, 5 months ago (2013-07-20 19:37:58 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108
7 years, 5 months ago (2013-07-21 12:37:12 UTC) #21
commit-bot: I haz the power
Change committed as 154611
7 years, 5 months ago (2013-07-21 13:14:04 UTC) #22
cbiesinger
On 2013/07/21 13:14:04, I haz the power (commit-bot) wrote: > Change committed as 154611 Is ...
7 years, 5 months ago (2013-07-22 17:53:48 UTC) #23
Rick Byers
On 2013/07/22 17:53:48, cbiesinger wrote: > On 2013/07/21 13:14:04, I haz the power (commit-bot) wrote: ...
7 years, 5 months ago (2013-07-22 19:28:18 UTC) #24
Rick Byers
7 years, 5 months ago (2013-07-22 20:06:09 UTC) #25
Message was sent while issue was closed.
On 2013/07/22 19:28:18, Rick Byers wrote:
> On 2013/07/22 17:53:48, cbiesinger wrote:
> > On 2013/07/21 13:14:04, I haz the power (commit-bot) wrote:
> > > Change committed as 154611
> > 
> > Is it possible that this made
compositing/iframes/iframe-content-flipping.html
> > consistently fail on Mac dbg?
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=com...
> 
> I suppose it's possible (although I didn't see such failures in my CQ / try
job
> runs).  I'm looking into the details of this failure now.

It looks like this test is flaky in the same way (border being the wrong color
or something) on linux and windows too - just not as consistently (so there's
definitely some sort of non-determinism issue here).  I can't see any way my CL
could have caused that (since this page has no touch event handlers, my change
_should_ have been almost entirely a no-op.  EXCEPT that it could have affected
timing slightly (basically on each layout I'd quickly see there are no touch
handlers and tell Internals that there are no rects, for which it'll allocate an
empty list in case the test asks for it).

I'll try running the test in a loop on my Linux desktop to see if I can get it
to fail with and without my change.

Powered by Google App Engine
This is Rietveld 408576698