1409085 - Responsive design mode triggers touch events when inspecting

Status: VERIFIED FIXED

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

Description

[Martin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171002220106

Steps to reproduce:

1. Create a div with touch listeners. (Like in this fiddle: https://jsfiddle.net/qyx5qccm/)
2. Start the responsive design mode and turn on touch simulation.
3. Click on the "select element" button and then select the div we created in step 1.

(I am using e10s so this is related to the "new" responsive design mode. I cannot tell if it happened in the old one as well.)


Actual results:

The touch listeners get fired.


Expected results:

The touch listeners shouldn't get fired. If the touch listener fires a function modifying the element, this element becomes hard to debug. 
This doesn't seem to happen when inspecting elements with click listeners (and touch simulation disabled).

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for filing and sharing the fiddle!

This seems similar but not identical to bug 1386796.

Comment 2

[Abhinav Koppula]

Hi Ryan,
I'm interested in working on this issue. 
Can you provide me some pointers on how I can approach this?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Sure, I'll try to sketch the issue here.

Effectively, we have two components conflicting with each other:

* The touch simulator[1] (which is only active when touch is enabled in RDM), which listens for mouseup, mousedown, etc. and creates fake touch events
* The inspector's "pick" mode[2] for selecting elements, which listens for click, etc. to set the node

At a high level, I think most users would agree (like the reporter of this bug) it's confusing for the touch simulator to do things when pick mode is enabled.

The code about picking sets `_isPicking`[3] in the highlighter actor to true when active.  I think one approach would be for the touch simulator to check if there is a highlighter actor, and if so, see if it is picking.  If it is, it would ignore events.

So, as a rough sketch:

1. The emulation actor should provide[4] the `tabActor` to the `TouchSimulator` so it can find other actors.
2. The touch simulator can use code such as:

```
  get _highlighterActor() {
    if (this.tabActor.exited) {
      return Promise.resolve();
    }
    let form = this.tabActor.form();
    let inspector = this.conn.getActor(form.inspectorActor);
    if (!inspector) {
      return Promise.resolve();
    }
    return inspector.getHighlighter();
  },
```

This returns a promise, so you can use yield or await to wait for the highlighter.  We'll probably want to cache the highlighter actor if we do get one, so we don't have to repeat this check for every event.

3. When the touch simulator handles event, it can use the highlighter actor from above to check the `_isPicking` state.

We'll also want to add a test.  Hopefully that helps!

[1]: http://searchfox.org/mozilla-central/source/devtools/server/actors/emulation/touch-simulator.js
[2]: http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/devtools/server/actors/highlighters.js#386
[3]: http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/devtools/server/actors/highlighters.js#230
[4]: http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/devtools/server/actors/emulation.js#31

Comment 4

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 5

[Martin]

Does the status change indicate that this bug probably won't be fixed? (Sorry - I am new to Firefox Bugzilla) If so: Is it possible to stop the responsive design mode from re-loading when clicking on the button for touch enabling/disabling? In this way, you could simply click the button every time you want to inspect an element.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Martin from comment #5)
> Does the status change indicate that this bug probably won't be fixed?

No, it just means it won't be fixed for version 58.  We'd still like to fix it.

> (Sorry - I am new to Firefox Bugzilla) If so: Is it possible to stop the
> responsive design mode from re-loading when clicking on the button for touch
> enabling/disabling? In this way, you could simply click the button every
> time you want to inspect an element.

I'd like introduce explicit reload control for RDM, which should help with what you're describing here.  I hope to work on this in bug 1428816.

Comment 7

[Martin]

Sounds good - thanks for the explanation!

Comment 9

[Patrick Brosset <:pbro>]

Comment 3 sounds like the general way to go, but there is a complication that might make this harder:
RDM has its own DebuggerServer (that's because it can run without DevTools being open).

So when you open both RDM and DevTools, you really end up with 2 different servers.
When you try to access the inspector actor from RDM's target, you do get one, because the inspector actor is a top-level actor that always exists. But it's just not the same instance as the one the DevTools toolbox also uses.
So there's no way for RDM to know whether we are currently picking an element or not.

I suggest another way:
in DevTools, when the picker is started, get the RDM UI instance via the RDM manager (this is a singleton that's guaranteed to be the right one).
Once we have the object, send a message to its server saying that the picker is started for that particular tab.
So that means introducing new actor methods, but I don't see any other way.

I'll give it a try.

Comment 11

[Patrick Brosset <:pbro>]

My suggestion in comment 9 should work, but it requires accessing the RDM manager from the node picker, and that might be made easier with bug 1539764.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: Bug 1409085 - Tell RDM when picking to stop simulating touch; r?mtigley!,gl!

Comment 13

[Daniel Holbert [:dholbert]]

(transferring regression info from dupe bug 1539636 comment 6)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b860d8c3379
Tell RDM when picking to stop simulating touch; r=mtigley,gl

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/5b860d8c3379

Comment 16

[Rares Doghi, Desktop QA]

This issue is verified as Fixed in Nightly 69.0a1 (2019-06-03).

Comment 17

[Rares Doghi, Desktop QA]

Hi Patrick, we didnt notice this issue before but it seems after we refresh the page and we click the red Div from https://jsfiddle.net/qyx5qccm/ we are actually triggering some events in console. Should we add a new bug for it ? should we reopen this one ? this only happens after we refresh the page.

Comment 18

[Martin Balfanz [:mbalfanz]]

Rares, I think you are experiencing a different issue. This bug refers to using "Inspector element" only, which did trigger events before. I cannot reproduce this on the link you provided.

However, I do see that something is wrong. Please file a new bug :)

Comment 19

[Alin Ilea, Desktop QA]

(In reply to Martin Balfanz [:mbalfanz] from comment #18)

Rares, I think you are experiencing a different issue. This bug refers to using "Inspector element" only, which did trigger events before. I cannot reproduce this on the link you provided.

However, I do see that something is wrong. Please file a new bug :)

Here is the new bug logged for the issue that Rares asked in comment 17:
https://bugzilla.mozilla.org/show_bug.cgi?id=1565160