New issue
Advanced search Search tips

Issue 920931 link

Starred by 1 user

SingleProcessMash: Web content hit testing broken with Screen Magnifier and iframes

Project Member Reported by rkalavakuntla@chromium.org, Jan 11

Issue description

Chrome 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

 
Actual.mp4
9.9 MB View Download
Expected.mp4
5.7 MB View Download
Cc: alemate@chromium.org aleventhal@chromium.org afakhry@chromium.org jdufault@chromium.org
Adding a few people to CC to help find an owner. Please help to provide an update as soon as possible as this is currently blocking the first M73 Dev.
Owner: msw@chromium.org
Status: Assigned (was: Untriaged)
msw@ could you please check to see if this is related to any of your recent changed? Thanks!
Components: UI>Input
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.)
Cc: jamescook@chromium.org
Labels: Proj-Mash-SingleProcess
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...
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)
Blocking: 918537
Cc: sky@chromium.org xiy...@chromium.org osh...@chromium.org
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);
  }
...
}
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?
I don't observe any defects with the fullscreen magnifier on maximized/fullscreen browser windows.
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.
Labels: -ReleaseBlock-Dev ReleaseBlock-Beta
Moving to RBB, can workaround this using the Accessibility settings in tray (sidenote, accessibility settings crashed while testing this in M72).
Status: Started (was: Assigned)
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.
Cc: sadrul@chromium.org riajiang@chromium.org
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
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.
Summary: SingleProcessMash: Web content hit testing broken with Screen Magnifier and iframes (was: Regression: Unable to operate any buttons in OOBE when Screen Magnifier is enabled)
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.
Cc: creis@chromium.org alex...@chromium.org
+alexmos@, +creis@
Cc: kenrb@chromium.org wjmaclean@chromium.org
Components: Internals>Sandbox>SiteIsolation
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.
Cc: -creis@chromium.org -alex...@chromium.org
The core issue is around coordinate conversion. So, removing alexmos and creis.

Comment 19 by wjmaclean@chromium.org, 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.

Comment 20 by msw@chromium.org, 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?
Project Member

Comment 21 by bugdroid1@chromium.org, 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

Comment 22 by msw@chromium.org, Today (14 hours ago)

Status: Fixed (was: Started)
Fixed, please help confirm.

Comment 23 by rkalavakuntla@chromium.org, 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