The event path through Node::dispatchMouseEvent bypasses all Blink state updates |
||||||||||
Issue descriptionThe event path through Node::dispatchMouseEvent, used for direct event dispatch for PointerLock and plugins (and more?), bypasses all Blink state updates around EventHandler. This looks bad. For example: - It originally had no PointerEvent firing on <object> ( crbug.com/659670 ). We added an urgent fix in M55 which bypasses PointerEvents state updates. - Should have other problems with mouse & pointer interactions (specially when interleaved between <object> and non-<object>). I haven't tested yet, but non-<object> never losing focus, PointerEvent on <object> being upcapturable etc comes to me off the top of my head. The real solution is to remove direct dispatches for PointerLock and plugins, instead pass all PlatformMouseEvents through EventHandler after consolidating all mouse states (from WebViewImpl etc) to EventHandler/MouseEventManager.
,
Nov 18 2016
,
Jan 20 2017
Wow, I agree this looks rather bad. It means, for example, that setPointerCapture will not work in such cases (because it doesn't believe the pointer to be in the active buttons state). Presumably it can also mess up over/out/enter/leave event sequences. There's probably a lot of other subtle bugs here.
This is marked Pri-2 ("want for current milestone") without a milestone listed. Please choose a target milestone or set it to Pri3.
,
Jan 23 2017
M58 seems a reasonable goal, at least for the two cases of direct dispatches mentioned above.
,
May 18 2017
Q2 triage checking - Mustaq, do we have a plan for addressing this?
,
May 18 2017
I am a bit hesitant to downgrade this to p3. Let's bump it back to p2 with a milestone that gives us some time to look into this.
,
May 18 2017
+Lan as it is somewhat related to the pointerlock change you are working on.
,
May 18 2017
Yes, good point: a fix for Issue 697581 should nuke the direct dispatch path for PointerLock (but the <object> path is independent).
,
May 18 2017
We haven't thought about doing the refactoring in that code right now. We are just going to clear the path and add a few checks here and there to tackle that specific problem. Here is the code review for that change in case you would like to chime in: https://codereview.chromium.org/2807433003/
,
Sep 6
#c3: eirage@ noted that we are dropping over/enter/out/leave events, at least in some cases. https://chromium-review.googlesource.com/c/chromium/src/+/1134219/11/third_party/blink/renderer/core/input/pointer_event_manager.cc#551
,
Sep 6
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98fe895533a521d1ac7f38627b86bb2dc718e3f7 commit 98fe895533a521d1ac7f38627b86bb2dc718e3f7 Author: Ella Ge <eirage@chromium.org> Date: Mon Sep 17 21:54:31 2018 Direct dispatch mouse pointer event by MEM and PEM We used to direct dispatch mouse pointer events by a different code path in node.cc. Unlike normal events, these events are created separately and didn't went through EventHandler. This causes extra complexity. This CL changes the direct dispatch code path to dispatch events through PointerEventManager and MouseEventManager, so we can have only one place to create events and further merge the event code path in the future. This patch should cause no change for direct dispatched events other then the click count. The new direct dispatch code path will also update click_count stored in MEM by the WebMouseEvent.click_count. This CL also fixes two issue caused by different code path, 1. direct dispatched pointermove events, (eg. when pointerlocked), didn't havbe getCoalescedEvents. 2. pointer move on chorded mouse button when pointer is locked. Bug: 859132 , 665924 Change-Id: Iead9c8135ef58c2bf8e45bd7787cf14334c2e139 Reviewed-on: https://chromium-review.googlesource.com/1134219 Commit-Queue: Ella Ge <eirage@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Cr-Commit-Position: refs/heads/master@{#591834} [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt/pointerevents/extension/pointerevent_getCoalescedEvents_when_pointerlocked-manual.html [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt/pointerevents/extension/pointerevent_pointerrawmove_in_pointerlock-manual.html [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointermove_in_pointerlock-manual.html [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointermove_on_chorded_mouse_button_when_locked-manual.html [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/resources/pointerevent_pointermove_in_pointerlock-iframe.html [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/extension/pointerevent_getCoalescedEvents_when_pointerlocked-manual-automation.js [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerevent_pointermove_in_pointerlock-manual-automation.js [add] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerevent_pointermove_on_chorded_mouse_button_when_locked-manual-automation.js [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/dom/node.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/dom/node.h [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/exported/web_view_impl.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/exported/web_view_impl.h [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/frame/web_frame_widget_base.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/html/forms/internal_popup_menu.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/input/event_handler.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/input/event_handler.h [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/input/pointer_event_manager.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/input/pointer_event_manager.h [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/page/pointer_lock_controller.cc [modify] https://crrev.com/98fe895533a521d1ac7f38627b86bb2dc718e3f7/third_party/blink/renderer/core/page/pointer_lock_controller.h |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by bokan@chromium.org
, Nov 17 2016