Issue metadata
Sign in to add a comment
|
SingleProcessMash: Web content hit testing broken with Screen Magnifier and iframes |
||||||||||||||||||||||||
Issue descriptionChrome Version:73.0.3667.0/11562.0.0 dev channel Daisy,Kip,Reks OS:Chrome OS What steps will reproduce the problem? (1)Recover build >> In OOBE, enable Screen Magnifier from Accessibility settings (2)Now try to click on any toggle buttons and observe(Please refer video) Actual: Unable to operate any buttons in OOBE when Screen Magnifier is enabled Expected: Should be able to operate any buttons when we enable screen Magnifier This is a Regression issue as same works fine in 73.0.3666.0/11555.0.0 dev
,
Jan 11
msw@ could you please check to see if this is related to any of your recent changed? Thanks!
,
Jan 11
Looking, here are my recent magnifier CLs for reference: https://chromium-review.googlesource.com/c/chromium/src/+/1387669 https://chromium-review.googlesource.com/c/chromium/src/+/1399285 This seems like an event coordinate translation/scaling issue. (video shows mismatching cursor and hover location, toggling a button not under the cursor, etc.)
,
Jan 11
This definitely doesn't repro with --disable-features=SingleProcessMash If this is indeed a dev blocker, we may need to disable SingleProcessMash by default for now...
,
Jan 11
Enabling the fullscreen magnifier in an existing session works fine in SingleProcessMash. If you manage to get through OOBE with the broken magnifier, it begins working correctly. Does anyone know what's special about the magnifier during OOBE? (I'm not actually a magnifier expert)
,
Jan 11
,
Jan 11
I haven't made significant progress, any advice/insight from other folks would be greatly appreciated.
I wonder if there's something broken about SingleProcessMash with RootWindowTransformer or the way we're using these pre-target handlers?
Applying the magnifier's transform to the MouseEvent::location(), *not root_location()*, in MagnificationController::OnMouseEvent (a pre-target handler) seems to fix some initial incorrect hover and click positioning for the accessibility menu itself (the toggles and [OK] button), but it breaks the [Shut Down] button and system tray button (likely an issue with Ash vs. Chrome windows?), it doesn't fix mouse interaction in the "Sign into your Chrome" dialog later on, and it offsets all mouse interaction once you've signed in...
void MagnificationController::OnMouseEvent(ui::MouseEvent* event) {
aura::Window* target = static_cast<aura::Window*>(event->target());
aura::Window* current_root = target->GetRootWindow();
gfx::Rect root_bounds = current_root->bounds();
if (IsMagnified()) { // This block is my hacky change...
auto location = event->location();
GetMagnifierTransform().TransformPoint(&location);
auto root = event->root_location();
GetMagnifierTransform().TransformPoint(&root);
LOG(ERROR) << "MSW MagnificationController::OnMouseEvent location:" << event->location().ToString() << " -> " << location.ToString() << ", " << event->root_location().ToString() << " -> " << root.ToString();
event->set_location(location);
// event->set_root_location(root);
}
...
}
,
Jan 11
One thing special about oobe vs in-session is that oobe is a fullscreen widget contains a webui. Would the problem happen in session with a maximized/fullscreen browser window?
,
Jan 11
I don't observe any defects with the fullscreen magnifier on maximized/fullscreen browser windows.
,
Jan 11
I suspect this is because WindowTree is using aura::Window::ConvertPointToTarget(), which doesn't consider the transform on the root. I think WindowTree needs to use something like:
client_root->window()->layer()->ConvertPointFromAncestor(nullptr,
&root2);
instead of :
aura::Window::ConvertPointToTarget(window->GetRootWindow(),
client_root->window(), &root_location);
ConvertPointFromAncestor() with null considers the transform on the root.
I did try this and it doesn't fix things, so there is more going on here.
,
Jan 14
Moving to RBB, can workaround this using the Accessibility settings in tray (sidenote, accessibility settings crashed while testing this in M72).
,
Jan 14
AFAICT, RenderWidgetHostViewAura::OnMouseEvent is getting the same coordinates when hovering the cursor over the same content, regardless of magnifier zoom and position. That appears to be the same behavior with and without SingleProcessMash. So my guess is that some handling within/after RWHVA is incorrectly adjusting the location, or Mash magnification affects some low level value used by that code. I'm going to dig in that direction to look for defects, but I'm not really familiar with content/blink event handling.
,
Jan 15
As Scott suspected, this issue is related to the transform on the root window, but the specific defect is still unclear. OOBE hits some bad point conversion code, but fullscreen web contents (a similar scenario) but doesn't hit that code and works as intended. Ria or Sadrul, can you spot anything obvious? I'll keep digging to understand, but advice from folks more familiar with this area is appreciated. The issue seems to be in RenderWidgetHostInputEventRouter::FindViewAtLocation's coordinate conversion: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_input_event_router.cc?rcl=d81c6c45c37b90f3576b6ab3ac4d6963b894d036&l=484 That calls through to RenderWidgetHostViewAura::TransformPointToRootSurface, which doesn't convert the location correctly: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_aura.cc?rcl=5968f8fc2c5c9e3d830fe48d5a9bbd1b3ac5489f&l=934 Classic Ash OOBE: render_widget_host_view_aura.cc(1694)] MSW RenderWidgetHostViewAura::OnMouseEvent location: 500,600 render_widget_host_view_aura.cc(954)] MSW RenderWidgetHostViewAura::TransformPointToRootSurface point: 500,600 render_widget_host_input_event_router.cc(529)] MSW RenderWidgetHostInputEventRouter::FindViewAtLocation point: 500,601; point_in_root_in_pixels: 500,600; transformed_point: 687,661 render_widget_host_input_event_router.cc(434)] MSW RenderWidgetHostInputEventRouter::FindMouseEventTarget transformed_point: 687,661 Single Process Mash OOBE: render_widget_host_view_aura.cc(1694)] MSW RenderWidgetHostViewAura::OnMouseEvent location: 500,600 ERROR:render_widget_host_view_aura.cc(954)] MSW RenderWidgetHostViewAura::TransformPointToRootSurface point: 500,600 ERROR:render_widget_host_input_event_router.cc(529)] MSW RenderWidgetHostInputEventRouter::FindViewAtLocation point: 500,600; point_in_root_in_pixels: 500,600; transformed_point: 250,684 render_widget_host_input_event_router.cc(434)] MSW RenderWidgetHostInputEventRouter::FindMouseEventTarget transformed_point: 250,684 Classic Ash fullscreen web contents in active session: render_widget_host_view_aura.cc(1694)] MSW RenderWidgetHostViewAura::OnMouseEvent location: 500,600 render_widget_host_input_event_router.cc(434)] MSW RenderWidgetHostInputEventRouter::FindMouseEventTarget transformed_point: 500,600 Single Process Mash fullscreen web contents in active session: render_widget_host_view_aura.cc(1694)] MSW RenderWidgetHostViewAura::OnMouseEvent location: 500,600 render_widget_host_input_event_router.cc(434)] MSW RenderWidgetHostInputEventRouter::FindMouseEventTarget transformed_point: 500,600
,
Jan 15
I poked at this some last night. The event (WebMouseEvent) that is sent to renderers has a location in two coordinate spaces as well. One (position-in-widget) comes directly from the location of the Event (location, not root location). The other coordinate space is screen comes from GetScreenLocationFromEvent() in ui/events/blink/web_input_event.cc. If the event has a target, it uses target->GetScreenLocationF(), otherwise it uses the root location. I didn't have a chance to see if the mash/non-mash cases line up.
,
Jan 15
This is not specific to OOBE or fullscreen, it repros on any site with iframes, perhaps more. The key is whether or not it triggers the RenderWidgetHostInputEventRouter::FindViewAtLocation codepath.
,
Jan 15
+alexmos@, +creis@
,
Jan 15
Comment 16: What's your question for us? If this is a Site Isolation or OOPIF coordinate issue, the SiteIsolation component is worth adding, and we can ask wjmaclean@ or kenrb@ to take a look or offer some suggestions.
,
Jan 15
The core issue is around coordinate conversion. So, removing alexmos and creis.
,
Jan 16
(6 days ago)
Re C#15: what is the predominant codepath when RenderWidgetHostInputEventRouter::FindViewAtLocation() is *not* being called? This might be helpful for comparison purposes.
,
Jan 16
(6 days ago)
Scott authored a fix back in October that I revived here (I'm working on fixing content_unittests): https://chromium-review.googlesource.com/1411650 Sadrul and Ria, do you have any new thoughts on that approach?
,
Yesterday
(31 hours ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b4f6394dcd59d8697e154c2e2c74eb85284f2e4b commit b4f6394dcd59d8697e154c2e2c74eb85284f2e4b Author: Mike Wasserman <msw@chromium.org> Date: Mon Jan 21 22:38:35 2019 mash: content: changes RenderWidgetHostInputEventRouter hit test querying Rebase of sky@'s < http://crrev.com/c/1239454 >. Also, make FindTargetForLocationStartingFromImpl virtual for testing. Original description: ===================================================================== Specifically this converts from starting the hit testing from the root to hit testing from the frame-sink of the RenderWidgetHost. As the event is already received by the RenderWidgetHost hit testing can start from there, rather than the root. This matters in the mash case because of hot hit testing works. ===================================================================== TODO: After this change, we may be able to revert http://crrev.com/c/1249152 (sky@ suggested this should be possible, but it breaks SPM iframe targeting afaict) Bug: 879791, 920931 Test: SingleProcessMash web content (w/iframe) hit testing works with a11y magnifier. Change-Id: Ie6c53ecfdba9444c62a9b3e3a7f573f0ec27ce6b Reviewed-on: https://chromium-review.googlesource.com/c/1411650 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Ria Jiang <riajiang@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#624687} [modify] https://crrev.com/b4f6394dcd59d8697e154c2e2c74eb85284f2e4b/components/viz/host/hit_test/hit_test_query.cc [modify] https://crrev.com/b4f6394dcd59d8697e154c2e2c74eb85284f2e4b/components/viz/host/hit_test/hit_test_query.h [modify] https://crrev.com/b4f6394dcd59d8697e154c2e2c74eb85284f2e4b/components/viz/host/hit_test/hit_test_query_unittest.cc [modify] https://crrev.com/b4f6394dcd59d8697e154c2e2c74eb85284f2e4b/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/b4f6394dcd59d8697e154c2e2c74eb85284f2e4b/content/browser/renderer_host/render_widget_host_input_event_router_unittest.cc
,
Today
(14 hours ago)
Fixed, please help confirm.
,
Today
(2 hours ago)
C # 22 > Verified the issue on 73.0.3673.0/11627.0.0 dev and unable to reproduce the issue.Hence,issue seems fixed. Thanks! |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by dgagnon@chromium.org
, Jan 11