1609199 - RDM Touch simulator + Dev Tools element picker on link causes link to be followed when selected

Status: RESOLVED FIXED

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

Description

[repertor]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

When using the RDM built-in viewports for touch-based devices (e.g. iOS), touch simulation is turned on by default. When also using the Dev Tools element picker to select a link on the page, instead of just showing the <a> in the DOM and showing the styles, the link is instead followed.

Actual results:

Links that should be inspected only are loaded.

Expected results:

The link should only be selected, and not followed, when using the element picker.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Brad Werth [:bradwerth]]

When I reproduce the bug, clicking the link does both things. I tried a mailto: link. The Inspector does show the selected anchor element, and the link is followed. I'll check to see if this is a regression.

Comment 3

[Brad Werth [:bradwerth]]

This was regressed by the patch for Bug 1581799. I'm needinfo'ing the author of that patch, and also Julian in case this is an easy cleanup.

Comment 4

[Julian Descottes [:jdescottes]]

Thanks for the ping Brad! This issue seems non-trivial to fix.
The change that caused the issue is the following one:

--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js 
   _onPickerStarting: async function() {
     this.tellRDMAboutPickerState(true);
     this.pickerButton.isChecked = true;
     await this.selectTool("inspector", "inspect_dom");
+    // turn off color picker when node picker is starting
+    this.getPanel("inspector").hideEyeDropper();
     this.on("select", this.nodePicker.stop);
   },

Inspector::hideEyeDropper calls Inspector::stopEyeDropperListeners, which is:

  stopEyeDropperListeners: function() {
    this.toolbox.tellRDMAboutPickerState(false);
    this.inspectorFront.off("color-pick-canceled", this.onEyeDropperDone);
    this.inspectorFront.off("color-picked", this.onEyeDropperDone);
    this.walker.off("new-root", this.onEyeDropperDone);
  },

So going back to _onPickerStarting, we first call tellRDMAboutPickerState(true);, and then right after we call tellRDMAboutPickerState(false) (via stopEyeDropperListeners).
tellRDMAboutPickerState is used to disable touch simulation when a picker is used (color picker or element picker). And since we use a single flag for both, we get the bug described above. We need to be able to say "color picker is not used BUT element picker is used".

So the main issue is that we are using a single flag. We should change the implementation to let the TouchSimulator know if elementPicker is on and if colorPicker is on, independantly (in devtools/server/actors/emulation/touch-simulator.js). Another issue is that tellRDMAboutPickerState is async, but we never await on it anywhere. If we did, we could have worked around the issue by moving the call to this.getPanel("inspector").hideEyeDropper(); before the call to this.tellRDMAboutPickerState(true); in _onPickerStarting.

To make it easier for backward compat maybe we could simply add an optional type argument to setElementPickerState.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609199 - Remove Emulation Actor

We still need the front and the spec files to talk to older servers.
However we don't maintain forward compatibility, which means the server is not supposed to work with older clients.
So the emulation actor can be removed.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609199 - Track picker types independantly in devtools touch-simulator.js

Depends on D60294

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609199 - Add RDM test browser_picker_link.js

Depends on D60295

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/219e155d358e
Remove Emulation Actor r=mtigley
https://hg.mozilla.org/integration/autoland/rev/40e2997a5397
Track picker types independantly in devtools touch-simulator.js r=mtigley
https://hg.mozilla.org/integration/autoland/rev/d7e6814835fe
Add RDM test browser_picker_link.js r=mtigley

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/219e155d358e
https://hg.mozilla.org/mozilla-central/rev/40e2997a5397
https://hg.mozilla.org/mozilla-central/rev/d7e6814835fe