1625925 - RDM touch simulation of a transformed stacking context incorrectly transforms the click location

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P3)

Description

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: filling form.MOV

[Affected versions]
Release 74.0 (64-bit)
Beta 75.0b11 (64-bit)
Firefox Nightly 76.0a1 (2020-03-31) (64-bit)

[Affected platforms]
All

[Steps to reproduce]

1- Launch FF and open a page that has login credential fields such as reddit.com
2- Input text in each text field displayed.
3- Log in and save the credentials
4- Log out and go back to the log in page.
5- Autofill both fields. (username and password)
6- Click in each field and notice that cursor goes from one field to the other one.
7- Activate touch simulation
8- Click in each field.

[Expected result]
User should be able to click any field without issue.

[Actual result]
cursor gets stuck in the username field, showing the saved logins dropdown.

Sometimes clicking wont' work at all, and tab key needs to be used to reach the text field.

Comment 1

[Clara Guerrero ( Need Info Brindusa Tot Please)]

This issue also happens in tribunnews.com in login page.

Comment 2

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: login.MOV

Comment 3

[Brad Werth [:bradwerth]]

I'm working to make a reduced testcase, which is not easy. I have been able to reduce the reddit page to the point where reproducing the bug is dependent on z-index properties. My guess is that the overlay login content is incorrectly passing through the click to something at a lower z-index.

Comment 4

[Brad Werth [:bradwerth]]

Additionally, my efforts at reduction have eliminated the input field as being relevant. In my still-too-complex reduced testcase, the error occurs with a click on an empty div above other content with a lower z-index.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: stacking_context_bad_event_transform.html

The testcase displays its own Steps to Reproduce.

Comment 6

[Brad Werth [:bradwerth]]

Botond, since this is dependent on a CSS transform, I'd appreciate hearing any insights you might have as to what is going on. I'll still debug from the RDM event trigger on down and see what I discover.

Comment 7

[Brad Werth [:bradwerth]]

What happens in this case with Touch Simulation ON is that the mouseup event is routed through nsContentUtils::SendMouseEvent, where it hits https://searchfox.org/mozilla-central/rev/54f965e51e4f77866bec42737978d40d4c1fdfc5/dom/base/nsContentUtils.cpp#7937 . That applies an unfortunate offset for the event that pushes it down (in a Y axis sense) considerably. The offset is calculated earlier in the function, through code that calls nsView::GetNearestWidget. I'm still working to understand the tree walk done in that code.

Interestingly, the offset is calculated differently for the mousedown -> mousemove -> mouseup cases. I'm not sure why the offset should be different for those 3 cases.

When Touch Simulation is OFF, the code path does not pass through nsContentUtils::SendMouseEvent, so it's difficult to directly compare anything like an offset calculation. In the OFF case, the event arrives via BrowserChild::DispatchWidgetEventViaAPZ and doesn't get an offset applied to the event.

Comment 8

[Brad Werth [:bradwerth]]

With patches for Bug 1556556 applied, which changes the behavior of the code path noted in comment 7, this bug is changed but not fixed. With those patches applied, the reddit case no longer passes through clicks to the lower stacking context, but does not register clicks on the input fields such that they can be edited. In the artificial testcase, the clicks don't pass through to the bottom stacking context, but a modified version with a click handler on the top content in the iframe shows that the offsets are being incorrectly calculated for that stacking context -- explaining the problem with the reddit case. I'll make Bug 1556556 a blocker and continue debugging. I'll also upload my modified testcase (2 files).

Comment 9

[Brad Werth [:bradwerth]]

Attachment: stacking_context_outer.html

Pair this with stacking_context_inner.html to see whether the top or bottom stacking context is getting the click event, and at which coordinates.

Comment 10

[Brad Werth [:bradwerth]]

Attachment: stacking_context_inner.html

Pair this with stacking_context_outer.html to see whether the top or bottom stacking context is getting the click event, and at which coordinates.

Comment 11

[Brad Werth [:bradwerth]]

Specifically, with the new testcase and with the patches for Bug 1556556 applied, clicking in the top-left quadrant triggers the onclick handler for the top stacking context, and shows that top left corner is being offset as if it is the center of the clickable region.

Comment 12

[Brad Werth [:bradwerth]]

With patches for Bug 1556556 applied, the contact point first appears with a bad offset already applied in BrowserParent::SendHandleTap in the parent process. There's no JS context on the stack, though the point of contact is generated by JS code, which I'll now go looking for.

Comment 13

[Brad Werth [:bradwerth]]

touch-simulator.js synthesizeNativeTouch is translating the window-relative CSS points into screen coordinates with its coordinatesRelativeToScreen function, which just isn't doing the correct math. We'll probably have to build something that correctly applies BrowserChild::GetChildToParentConversionMatrix() in this function.

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1625925 Part 1: Change touch-simulator.js synthesizeNative methods to use screen coordinates directly.

This change improves the math in coordinatesRelativeToScreen, to make it use
the same transforms calculated by the Platform code instead of trying to
recreate those transforms in JS code. It does this by relying on the
getBoxQuadsFromWindowOrigin method.

This change will likely not correctly handle the case of UI events triggered
in a cross-process iframe within a RDM document with a resolution other than 1.
The next part of the patch will introduce a test of this case, marked as TODO
if necessary.

Comment 15

[Brad Werth [:bradwerth]]

Attachment: Bug 1625925 Part 2: Make RDM UI functions that trigger reload await reload.

Depends on D74161

Comment 16

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 17

[Brad Werth [:bradwerth]]

The presentation of this bug has changed in the past few days; the testcases I added no longer demonstrate the problem well. The reddit login issue remains; I'll make a new testcase that makes it easy to reproduce.

Comment 18

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #17)

The presentation of this bug has changed in the past few days; the testcases I added no longer demonstrate the problem well. The reddit login issue remains; I'll make a new testcase that makes it easy to reproduce.

Seems likely that Bug 1637113 is responsible for the change / partial fix. The existing testcase can still demonstrate the problem, with new Steps to Reproduce:

1. Download locally attachment 9145917 [details] and attachment 9145918 [details]. Navigate to stacking_context_outer.html.
2. Open RDM, turn on Touch Simulation.
3. Click in either the right half or bottom half of the content.

Expected Results: a window alert will appear indicating that a click was received at x, y coordinates, where x and y correspond to the proportional position of the click relative to the RDM viewport size (which is also the layout viewport size due to the meta viewport tag).

Actual Results: Nothing will happen. If you click in the top-left quadrant of the content, you'll get the window alert. The reported coordinates will be translated 50% down and to the right.

Botond since you landed the partial fix in Bug 1637113, perhaps this test case is useful for making a more correct solution in the same code?

Comment 19

[Brad Werth [:bradwerth]]

Attachment: Bug 1625925 Part 2: Add tests of simulated touch events in documents with iframes.

Depends on D74161

Comment 20

[Botond Ballo [:botond]]

Sorry for the delay in responding here.

The problem is indeed that coordinatesRelativeToScreen() does not handle targets inside iframes correctly. This is also true of the version of this function in APZ test code (which the RDM version was based on), and we'll want to fix this eventually.

However, in RDM, I think we can deploy a simpler solution. Given that we need screen coordinates as input to nsIDOMWindowUtils.synthesizeNativeTouchPoint(), and we need those coordinates to correspond to the location of an original input event (the mouse event), it seems silly to use MouseEvent.clientX/Y, and then try to convert the result to screen coordinates. Instead, MouseEvent provides an API that gives us screen coordinates directly, namely, MouseEvent.screenX/Y, whose implementation does exactly the calculation we need.

Comment 21

[Botond Ballo [:botond]]

Attachment: Use Event.screenX/Y to get coordinates to pass into synthesizeNativeTouchPoint()

This patch fixes the STR described in comment 18 for me.

Comment 22

[Micah [:mtigley] (she/her)]

(In reply to Botond Ballo [:botond] from comment #20)

Instead, MouseEvent provides an API that gives us screen coordinates directly, namely, MouseEvent.screenX/Y, whose implementation does exactly the calculation we need.

If we decide to use screenX and screenY, would we still need to scale those values with the deviceScale calculated at https://searchfox.org/mozilla-central/source/devtools/server/actors/emulation/touch-simulator.js#359 ? I'm unsure if the calculations for screenX and screenY account for RDM's override DPR.

Comment 23

[Botond Ballo [:botond]]

(In reply to Micah Tigley [:mtigley] from comment #22)

If we decide to use screenX and screenY, would we still need to scale those values with the deviceScale calculated at https://searchfox.org/mozilla-central/source/devtools/server/actors/emulation/touch-simulator.js#359 ? I'm unsure if the calculations for screenX and screenY account for RDM's override DPR.

I believe the answer is no. The outcome we want is to ignore the override DPR (since it's not actually used for rendering), and most platform APIs, including the ones used by screenX/Y (e.g. nsPresContext::AppUnitsPerDevPixel()) do so. window.devicePixelRatio is one of the few places that respects the override DPR (which is why we had to replace it, in coordinatesRelativeToScreen(), with the new API utils.screenPixelsPerCSSPixelNoOverride which ignores it).

Comment 24

[Brad Werth [:bradwerth]]

(In reply to Botond Ballo [:botond] from comment #23)

I believe the answer is no. The outcome we want is to ignore the override DPR (since it's not actually used for rendering), and most platform APIs, including the ones used by screenX/Y (e.g. nsPresContext::AppUnitsPerDevPixel()) do so. window.devicePixelRatio is one of the few places that respects the override DPR (which is why we had to replace it, in coordinatesRelativeToScreen(), with the new API utils.screenPixelsPerCSSPixelNoOverride which ignores it).

In my testing, screenX and screenY are still off by a factor equivalent to the real DPR. I'll propose a modified version that includes the multiplication by DPR.

Comment 25

[Brad Werth [:bradwerth]]

This was generated from mach try auto. First time using it. We'll see...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf6d6ca5125fd9e1f0dd153a412774227983d92d

Comment 26

[Brad Werth [:bradwerth]]

New try push, hopefully more focused: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f823c8a1c5142f75be025fb63ad82926a9b61d

Comment 27

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi, i'm trying to reproduce this now in beta 79.0b4 (64-bit) without success, but it does happen in nightly 80.0a1 (2020-07-06) (64-bit).

Best regards,
Clara

Comment 28

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: click bank of america.mov

Hi again, i'm encountering this issue both nightly and beta for https://www.bankofamerica.com/ website. Im attaching video.

Comment 29

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: click2 bankofamerica .mov

Comment 30

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: no touch simulation.mov

Comment 31

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #26)

New try push, hopefully more focused: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f823c8a1c5142f75be025fb63ad82926a9b61d

Weirdly, the failures here on Linux show that the first, relatively-simple test passes, but then a later test that is nearly identical except the iframe is hosted from another domain fails. But then other tests hosted from that domain succeed. So there's no commonality to the failures, except perhaps that the click event has a timeout built-in. I'm extending the timeouts and will try again on Linux.

Comment 32

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffbb0eae1d2c2bc852a168246d633f57af85e972

Comment 33

[Brad Werth [:bradwerth]]

Hmm.. the test is just straight-up intermittent on Linux Fission. Duplicated try runs have very different failure logs. I've extended the timeouts so far that it's unlikely to be just a timeout issue. I'll see if there are other things I can do to make the test more consistent. Perhaps the tab is being re-used or the window is being moved or something. If I can't get it working on Linux, I'll disable the test on that platform and attempt to land the patches.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/102ed3aff763
Part 1: Change touch-simulator.js synthesizeNative methods to use screen coordinates directly. r=botond
https://hg.mozilla.org/integration/autoland/rev/1ac2f8c1bb93
Part 2: Add tests of simulated touch events in documents with iframes. r=mtigley

Comment 35

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/102ed3aff763
https://hg.mozilla.org/mozilla-central/rev/1ac2f8c1bb93