Pointer events regresses touch events for site-isolation (oopif) |
||||||||||
Issue descriptionTurning on PointerEvents in Chrome has regressed touch event handling in OOPIF sub-frames. 1) Run chrome with --site-per-process 2) Load the following data URI: data:text/html, <body><iframe src="https://rbyers.github.io/paint.html" style="width: 1000px; height: 800px"></iframe></body> Finger painting in this page works for the first part of the movement, and then stalls. Running chrome with --disable-features=PointerEvents causes the page to operate properly. The short term problem seems to be this: PointerEventManager::handleTouchEvents() receives TouchScrollStart, causing it to call blockTouchPointers(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/PointerEventManager.cpp?rcl=0&l=276 As a side-effect, after blockTouchPointers() has been called, moving a mouse pointer over the iframe causes painting, even though the mouse has not been clicked - this seems to be due to the frame not receiving the corresponding unblock() when the touch is lifted? Note that removing the call to blockTouchPointers() alleviates the symptoms. It also helps to disable the slop-suppression filtering at https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/touch_event_queue.cc?rcl=1484137392&l=881 helps, but is not central to the problem. The larger question seems to be Why are we getting TouchScrollStarted in the first place. This event is sent from TouchEventQueue when GestureEvents start being sent to the frame. In discussion with dtapuska@, it seems this is related to the fact that ScrollingCoordinator doesn't work properly for oopif frames, in that touch handler region rects don't seem to be generated properly for the frames. Dave, could you elaborate on how the touch handler rects are involved? ScrollingCoordinators are page-level objects. They have a function called updateTouchEventTargetRects() which notifies the compositor about where the touch event regions are in a page. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp?rcl=1484137392&l=665 However, the issue of touch event rects may be one level removed here: When the paint page is loaded in the main frame, InputHandlerProxy::HandleTouchStart() returns DID_NOT_HANDLE. However, this function is never called when the oopif iframe has the touch handler, meaning the suppression of GestureScroll events cannot happen. I suspect this is because the touch events are never sent to the (correct?) compositor in the OOPIF case, because the subframes are using a different layer tree than the RenderView which receives the events. It may be my efforts to move the InputHandler from the RenderView to the RenderWidget may help here, but may not be sufficient. Thoughts?
,
Jan 11 2017
Yes, this affects M56. I'll try and see if landing https://codereview.chromium.org/2479663002/ gets us close to a solution or not. I don't think this issue overlaps issue 675695 , although they both lead back to ScrollingCoordinator, so eventually something needs to be done there as well.
,
Jan 11 2017
I agree with comment #2, they are related in that they both are linked to ScrollingCoordinator not properly dealing with OOPIFs, but past that there is no similarity. The fixes for these bugs are going to be different. James, can you own this one?
,
Jan 11 2017
Sure, I'll take over as owner until (at least) we decide what the best way forward is.
,
Jan 11 2017
The touch-action is relevant on this paint page. What should really happen is the layout adjusts the event handler registry via: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutObject.cpp?sq=package:chromium&rcl=1484137392&l=1820 when it detects a layout object has a touch-action. The scrolling coordinator will then update the touch rects: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutObject.cpp?sq=package:chromium&rcl=1484137392&l=1820 And then when the touch-start is received it should know to send it to the main thread: https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?q=handleTouchStart&sq=package:chromium&l=946 I presume that isn't executing correctly and is saying there are no touchstart listeners which then allows scrolling to occur.
,
Jan 11 2017
Pointer events enabled here: https://codereview.chromium.org/2126663002
,
Jan 11 2017
Re comment 5: At present *all* touch events go to the main thread, butInputHandlerProxy::HandleTouchStart never executes for an OOPIF since the OOPIF's compositor doesn't even have one (yet). It seems the suppression of the GestureScrollEvents in the browser requires the response from InputHandlerProxy?
,
Jan 11 2017
Have you verified this method is executing correctly? https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?q=setTouchAction&sq=package:chromium&l=2189
,
Jan 11 2017
No, I'll verify that next ...
,
Jan 11 2017
RenderWidget::setTouchAction() is *not* working correctly for OOPIFs. Background: note that the OOPIF renderer in the test case mentioned in this bug will have a RenderView (which is a RenderWidget), and a pure RenderWidget. All compositing (and presumably event handling) are associated with the latter. But both will have a compositor/layer-tree. RenderWidget::setTouchAction() *is* getting called, but it never sends InputHostMsg_SetTouchAction since the function is called on the RenderView, and not on the RenderWidget associated with the OOPIF [1]. So when we get to https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&l=2194 the input handler has never seen an event. It seems like the correct thing to do here is have the ChromeClientImpl::setTouchAction() call reference the WebWidgetClient that is the OOPIF's RenderWidget, instead of the RenderView. That is very similar to the changes already in https://codereview.chromium.org/2479663002/, but I'll see if we can make this change separately (assuming it works).
,
Jan 11 2017
Ok, my idea from #c10 seems to work. I'm compiling against all the tests right now, and will have a CL up (hopefully) shortly.
,
Jan 11 2017
Candidate cl for a fix uploaded to https://codereview.chromium.org/2626133002
,
Jan 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c15a423974bf654c237bc27a2ac12cca22cccd2 commit 8c15a423974bf654c237bc27a2ac12cca22cccd2 Author: wjmaclean <wjmaclean@chromium.org> Date: Thu Jan 12 18:45:29 2017 Candidate fix for PointerEvent-OOPIF combination not working. Prior to enabling PointerEvents, OOPIFs with TouchEvent handlers worked. But with PointerEvents turned on, they fail. This happens because the InputRouterImpl sending the touch events to the OOPIF never gets notified of the current TouchAction, in turn because ChromeClientImpl sends the TouchAction notification via RenderView and not the RenderWidget serving the OOPIF. Prior to PointerEvents this was not an issue, as the touch events would continue to flow regardlesss. But with PointerEvents, touch events are blocked after a TouchScrollStart is issued. This CL plumbs a LocalFrame* parameter into ChromeClient::setTouchAction so the notification can be sent via the correct channel. BUG= 680158 Review-Url: https://codereview.chromium.org/2626133002 Cr-Commit-Position: refs/heads/master@{#443302} [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/core/input/TouchEventManager.cpp [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/core/loader/EmptyClients.h [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/web/ChromeClientImpl.h [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/web/WebPagePopupImpl.cpp [modify] https://crrev.com/8c15a423974bf654c237bc27a2ac12cca22cccd2/third_party/WebKit/Source/web/tests/TouchActionTest.cpp
,
Jan 12 2017
I'll let this bake a day or two, then if all is ok we can evaluate it for merge back to M56.
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af8f03fcb37a1bb4bf1935c060880d82297350e7 commit af8f03fcb37a1bb4bf1935c060880d82297350e7 Author: wjmaclean <wjmaclean@chromium.org> Date: Fri Jan 13 22:06:56 2017 Add OOPIF-specific test for setting TOUCH_ACTION in PointerEvents. This CL adds another test for the changes made in https://codereview.chromium.org/2626133002/. Specifically, it modifies an existing OOPIF test for touch events sent to OOPIF subframes, making sure that the target sub-frame's InputRouterImpl receives the proper TouchAction information from the renderer. BUG= 680158 Review-Url: https://codereview.chromium.org/2633723002 Cr-Commit-Position: refs/heads/master@{#443692} [modify] https://crrev.com/af8f03fcb37a1bb4bf1935c060880d82297350e7/content/browser/renderer_host/input/input_router_impl.h [modify] https://crrev.com/af8f03fcb37a1bb4bf1935c060880d82297350e7/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/af8f03fcb37a1bb4bf1935c060880d82297350e7/content/test/data/page_with_touch_handler.html
,
Jan 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e899da13d3c288c9081bb85e372ebbadb53f51ce commit e899da13d3c288c9081bb85e372ebbadb53f51ce Author: wjmaclean <wjmaclean@chromium.org> Date: Mon Jan 16 17:01:05 2017 Address creis@'s suggestions for issue 2633723002. This patch was meant to be included in issue 2633723002 before landing it, but I managed to CQ the issue without uploading this. Landing it now, as it is an improvement TBR=creis@chromium.org BUG= 680158 Review-Url: https://codereview.chromium.org/2632263002 Cr-Commit-Position: refs/heads/master@{#443909} [modify] https://crrev.com/e899da13d3c288c9081bb85e372ebbadb53f51ce/content/browser/renderer_host/input/input_router_impl.h [modify] https://crrev.com/e899da13d3c288c9081bb85e372ebbadb53f51ce/content/browser/site_per_process_browsertest.cc
,
Jan 17 2017
I've done a manual inspection of the M56 branch, and it looks like this CL should apply cleanly. This is a high-priority fix, as touch events are currently broken for OOPIFs on M56.
,
Jan 17 2017
Fixed in ToT (M57 dev).
,
Jan 17 2017
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/545d14233e8e78d8420a9be6233efeef0777847f commit 545d14233e8e78d8420a9be6233efeef0777847f Author: Alex Mineer <amineer@chromium.org> Date: Wed Jan 18 02:02:58 2017 Candidate fix for PointerEvent-OOPIF combination not working. Prior to enabling PointerEvents, OOPIFs with TouchEvent handlers worked. But with PointerEvents turned on, they fail. This happens because the InputRouterImpl sending the touch events to the OOPIF never gets notified of the current TouchAction, in turn because ChromeClientImpl sends the TouchAction notification via RenderView and not the RenderWidget serving the OOPIF. Prior to PointerEvents this was not an issue, as the touch events would continue to flow regardlesss. But with PointerEvents, touch events are blocked after a TouchScrollStart is issued. This CL plumbs a LocalFrame* parameter into ChromeClient::setTouchAction so the notification can be sent via the correct channel. BUG= 680158 (cherry picked from commit 8c15a423974bf654c237bc27a2ac12cca22cccd2) Review-Url: https://codereview.chromium.org/2626133002 Cr-Original-Commit-Position: refs/heads/master@{#443302} Cr-Commit-Position: refs/branch-heads/2924@{#789} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/core/input/TouchEventManager.cpp [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/core/loader/EmptyClients.h [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/web/ChromeClientImpl.h [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/web/WebPagePopupImpl.cpp [modify] https://crrev.com/545d14233e8e78d8420a9be6233efeef0777847f/third_party/WebKit/Source/web/tests/TouchActionTest.cpp
,
Jan 18 2017
Merge approved by myself, CPed to make tonight's M56 build.
,
Jan 25 2017
Tested the issue on windows 10 using chrome version 56.0.2924.76 with the below steps 1) Run chrome with --site-per-process 2) Load the following data URI: data:text/html, <body><iframe src="https://rbyers.github.io/paint.html" style="width: 1000px; height: 800px"></iframe></body> 3)Finger painting worked properly without any issues. Please find the attached screen cast for the same. Adding TE-Verified labels. Thanks,
,
Apr 4 2017
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by creis@chromium.org
, Jan 11 2017