Fix injected touch coordinates for OOPIF touch emulation [chromium/src : main]
赵裕 (Gerrit)
赵裕 added 4 comments![]( "Open in Gerrit")
last_emulated_event_root_view_ = target->GetRootView();
if (!last_emulated_event_root_view_) {
last_emulated_event_root_view_ = target;
}赵裕Nit: Modify the logic to:
```
RenderWidgetHostViewInput* root_view = target->GetRootView();
last_emulated_event_root_view_ = root_view ? root_view : target;
``````
if (!last_emulated_event_root_view_) {
last_emulated_event_root_view_ = target;
}
```
This was slightly counter-intuitive, given what we really want to check was if `target->GetRootView()` is null or not.
Done in Patchset 10. I now store target->GetRootView() in a local root_view and assign last_emulated_event_root_view_ to root_view ? root_view : target, making the fallback explicit.
last_mouse_move_root_view_ ? last_mouse_move_root_view_.get() : target;赵裕Would just doing `target->GetRootView()` be sufficient here?
And can you please provide more context on why is this bit of code modification needed?
Kartar Singhtarget->GetRootView() is the intended value for the normal case. The fallback is retained because a child frame's GetRootView() can return null during teardown; using target preserves the router's prior fallback behavior for that case. The key change is to stop reusing last_mouse_move_root_view_, which is historical state and can refer to a different root than the injected touch's target. This keeps the touch sequence and synthesized gestures associated with the target's root view, as covered by the added regression test.
赵裕Thanks for the explanation. Makes sense.
Can we please add this to commit description to call out we are moving away from using `last_mouse_move_root_view_` as the root view for emulated touch events.
Done in Patchset 10. The commit description now calls out that injected touch events no longer use last_mouse_move_root_view_; the router uses the injected target's root view instead, which is correct when there was no preceding mouse move.
EXPECT_EQ(kPointInRoot,
child.view->TransformPointToRootCoordSpaceF(kPointInChild));赵裕Nit: Let's reverse the check here and in other places.
`Prefer (foo == 0) to (0 == foo).`
https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#code-formatting
Done in Patchset 10. Reversed the comparison operands in the regression test as suggested.
Related details
- Jonathan Ross
- Kartar Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh voted and added 6 comments![]( "Open in Gerrit")
Votes added by Kartar Singh
| Code-Review | +1 |
6 comments
Kartar SinghI am defering to @karta...@google.com: Please close this once he approved.
Done
last_emulated_event_root_view_ = target->GetRootView();
if (!last_emulated_event_root_view_) {
last_emulated_event_root_view_ = target;
}赵裕Nit: Modify the logic to:
```
RenderWidgetHostViewInput* root_view = target->GetRootView();
last_emulated_event_root_view_ = root_view ? root_view : target;
``````
if (!last_emulated_event_root_view_) {
last_emulated_event_root_view_ = target;
}
```
This was slightly counter-intuitive, given what we really want to check was if `target->GetRootView()` is null or not.
Done in Patchset 10. I now store target->GetRootView() in a local root_view and assign last_emulated_event_root_view_ to root_view ? root_view : target, making the fallback explicit.
Acknowledged. Feel free to generally mark comments `Resolved`, once you have addressed them.
last_mouse_move_root_view_ ? last_mouse_move_root_view_.get() : target;赵裕Would just doing `target->GetRootView()` be sufficient here?
And can you please provide more context on why is this bit of code modification needed?
Kartar Singhtarget->GetRootView() is the intended value for the normal case. The fallback is retained because a child frame's GetRootView() can return null during teardown; using target preserves the router's prior fallback behavior for that case. The key change is to stop reusing last_mouse_move_root_view_, which is historical state and can refer to a different root than the injected touch's target. This keeps the touch sequence and synthesized gestures associated with the target's root view, as covered by the added regression test.
赵裕Thanks for the explanation. Makes sense.
Can we please add this to commit description to call out we are moving away from using `last_mouse_move_root_view_` as the root view for emulated touch events.
Done in Patchset 10. The commit description now calls out that injected touch events no longer use last_mouse_move_root_view_; the router uses the injected target's root view instead, which is correct when there was no preceding mouse move.
Acknowledged
EXPECT_EQ(kPointInRoot,
child.view->TransformPointToRootCoordSpaceF(kPointInChild));赵裕Nit: Let's reverse the check here and in other places.
`Prefer (foo == 0) to (0 == foo).`
https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#code-formatting
Done in Patchset 10. Reversed the comparison operands in the regression test as suggested.
- Arthur Sonzogni
- Jonathan Ross
- 赵裕
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Thanks!
STAMP (defered to input owners)
Related details
- Jonathan Ross
- 赵裕
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix injected touch coordinates for OOPIF touch emulation
DevTools injected touch points are specified in root-view coordinates.
After OOPIF hit testing, the legacy injection path converted them to the
target view's coordinates, and TouchEmulatorImpl then converted them back
to root coordinates for gesture synthesis. This round trip could apply an
OOPIF offset twice to synthesized compatibility clicks and gestures.
Keep injected touch points in root-view coordinates through the touch
emulator and emulated event router. The input event router performs the
single root-to-target conversion only when dispatching to the renderer.
Compute screen coordinates from the root view as well.
Also stop using last_mouse_move_root_view_ for injected touch events.
Instead, route the emulated touch sequence through the root view of the
injected target, which remains correct when no prior mouse move exists.
- M AUTHORS
- M components/input/render_widget_host_input_event_router.cc
- M content/browser/devtools/protocol/input_handler.cc
- M content/browser/renderer_host/input/touch_emulator_impl.cc
- M content/browser/renderer_host/render_widget_host_input_event_router_unittest.cc
Code-Review: +1 by Kartar Singh, +1 by Arthur Sonzogni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/4684306447335424
Sample build with failed test: https://ci.chromium.org/b/8675393698746547873
Affected test(s):
[://\:chrome_wpt_tests!webtest::external/wpt/event-timing#event-click-visibilitychange.html](https://ci.chromium.org/ui/test/chromium/:%2F%2F%5C:chrome_wpt_tests%21webtest::external%2Fwpt%2Fevent-timing%23event-click-visibilitychange.html?q=VHash%3A6129bda16b001a29)
[://\:chrome_wpt_tests!webtest::external/wpt/pointerevents#pointerevent_capture_touch_and_release_at_got_capture.html](https://ci.chromium.org/ui/test/chromium/:%2F%2F%5C:chrome_wpt_tests%21webtest::external%2Fwpt%2Fpointerevents%23pointerevent_capture_touch_and_release_at_got_capture.html?q=VHash%3A6129bda16b001a29)
[://\:chrome_wpt_tests!webtest::external/wpt/pointerevents#pointerevent_click_during_parent_capture.html?pointerType=touch&preventDefault=touchstart](https://ci.chromium.org/ui/test/chromium/:%2F%2F%5C:chrome_wpt_tests%21webtest::external%2Fwpt%2Fpointerevents%23pointerevent_click_during_parent_capture.html%3FpointerType=touch&preventDefault=touchstart?q=VHash%3A6129bda16b001a29)
[://\:chrome_wpt_tests!webtest::external/wpt/pointerevents#pointerevent_disabled_form_control.html?touch](https://ci.chromium.org/ui/test/chromium/:%2F%2F%5C:chrome_wpt_tests%21webtest::external%2Fwpt%2Fpointerevents%23pointerevent_disabled_form_control.html%3Ftouch?q=VHash%3A6129bda16b001a29)
[://\:chrome_wpt_tests!webtest::external/wpt/pointerevents#pointerevent_element_haspointercapture.html?touch](https://ci.chromium.org/ui/test/chromium/:%2F%2F%5C:chrome_wpt_tests%21webtest::external%2Fwpt%2Fpointerevents%23pointerevent_element_haspointercapture.html%3Ftouch?q=VHash%3A6129bda16b001a29)
and 7 more ...
A revert for this change was not created because the builder that this CL broke is not watched by gardeners, therefore less important. You can consider revert this CL, fix forward or let builder owners resolve it themselves.
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F4684306447335424&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F8128317&type=BUG
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |