1239459 - Toggle touch event simulation

Status: RESOLVED FIXED

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

Description

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

Touch events button to turn on simulation of mobile touch events

Comment 1

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: touch-events.svg

Comment 2

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

This bug depends on bug 1240896 because you need a browser with a message manager before you can do much here.  It might be better to choose a different bug until that one is resolved.

Comment 3

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

One piece of this involves "enabling" touch events at all.  Assuming this bug copies the approach of legacy RDM, access to touch events can only be changed for the entire Firefox session (since it's a pref), but we've previously agreed it's important for RDM features to only affect the current tab.

Bug 970346 discusses adding a docshell flag instead so it would only affect one tab.  I'll put this bug up for triage.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1239459.patch

Looking for some general feedback. I don't think this patch needs much more beyond unit tests and maybe a check to reload, but I don't think the reload is actually necessary(?).

Comment 5

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

Comment on attachment 8747286 [details] [diff] [review]
1239459.patch

Review of attachment 8747286 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsive.html/actions/index.js
@@ +44,5 @@
>    // Update the device modal open state.
>    "UPDATE_DEVICE_MODAL_OPEN",
>  
> +  // Update the touch simulation enabled state.
> +  "UPDATE_TOUCH_SIMULATION_ENABLED",

I am not sure what to think about this UPDATE_* naming for boolean actions.  I sent a mail to the team list to get some more feedback.  When I read "UPDATE_*", it feels wrong to me at first since it's named in terms of "change state to X" instead of "user requested action Y" like some of the earlier actions.  It seems like the UPDATE_* naming (taken to the the extreme) could lead to every action starting with UPDATE_* since each action wants to change some kind of state, which is a form of update, but by then the word "update" doesn't really mean anything anymore.

I'll keep thinking about this and hopefully finish feedback tomorrow.

Comment 6

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

Comment on attachment 8747286 [details] [diff] [review]
1239459.patch

Review of attachment 8747286 [details] [diff] [review]:
-----------------------------------------------------------------

We do indeed need to reload for the event handlers to be bound properly, but we can work that part out in a follow up.  I filed bug 1269882 for this.

Overall it seems reasonable!  I think some simple unit tests from the new UI things would be good.  As discussed at the standup, we can leave coverage of actual touch simulation to the existing legacy RDM test that we'll eventually port over.

Also needs ui-review.

::: devtools/client/responsive.html/actions/index.js
@@ +44,5 @@
>    // Update the device modal open state.
>    "UPDATE_DEVICE_MODAL_OPEN",
>  
> +  // Update the touch simulation enabled state.
> +  "UPDATE_TOUCH_SIMULATION_ENABLED",

Sounds like most people like this naming style, so let's keep it as-is.

::: devtools/client/responsive.html/app.js
@@ +85,5 @@
> +  onUpdateTouchSimulationEnabled() {
> +    let { isTouchEnabled } = this.props.touchSimulation;
> +    this.props.dispatch(updateTouchSimulationEnabled(!isTouchEnabled));
> +
> +    window.postMessage({

Since we're sending this change through Redux and it's natural for Redux action creators to contain the logic to make the action happen, I think the `postMessage` should move to the action creator.  (See screenshot as an example, or other complex things in Memory tool.)

@@ +86,5 @@
> +    let { isTouchEnabled } = this.props.touchSimulation;
> +    this.props.dispatch(updateTouchSimulationEnabled(!isTouchEnabled));
> +
> +    window.postMessage({
> +      type: "touch-simulation-toggle",

Might as well make this sound active (since it's a request to make a change) and use the same name as in Redux, so "update-touch-simulation".

::: devtools/client/responsive.html/manager.js
@@ +225,5 @@
>      toolWindow.addEventListener("message", this);
>      yield waitForMessage(toolWindow, "init");
>      toolWindow.addInitialViewport(contentURI);
>      yield waitForMessage(toolWindow, "browser-mounted");
> +    let browser = toolWindow.document.querySelector("iframe.browser");

To ease working with multiple viewports, we should track an array of simulators, since we'll need one for each viewport.

When you get a message to start simulation, use `querySelectorAll` and make a simulator for each browser found since the button is currently in the global toolbar.  I guess we'll leave issues about adding a new viewport while simulation is already enabled for some future day. :)

@@ +236,5 @@
>      this.browserWindow = null;
>      this.tab = null;
>      this.inited = null;
>      this.toolWindow = null;
> +    this.touchEventSimulator = null;

It is important to stop the simulators on close.  They manipulate browser state that we need to reset on exit.

::: devtools/client/responsive.html/types.js
@@ +89,5 @@
> + * Touch simulation.
> + */
> +exports.touchSimulation = {
> +
> +  isTouchEnabled: PropTypes.bool.isRequired,

I think this can be shortened to `isEnabled` or `enabled`, since the touch part seems redundant.  (We already have `displayed` as a boolean state name...  Would prefer to have one style for such things!)

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1239459.patch WIP

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1239459.patch [1.0]

I left the window.postMessage in app.js since it causes problems with xpcshell tests with an undefined window if it is moved to actions/

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/try/pushloghtml?changeset=7b15b8265b66

Comment 11

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

Comment on attachment 8750055 [details] [diff] [review]
1239459.patch [1.0]

Review of attachment 8750055 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, sorry there was so much trouble with the multiple simulator approach last week.  We can investigate more in the future.

::: devtools/client/responsive.html/manager.js
@@ +243,5 @@
>      let loaded = waitForDocLoadComplete(browserWindow.gBrowser);
>      tabBrowser.goBack();
>      yield loaded;
> +
> +    yield this.touchEventSimulator.stop();

Move this before the previous step that navigates the tab back.  We want to have everything back to normal before navigating the tab back.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1239459.patch [2.0]

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/027b0144a1a6c2d62dfb1656e41597acfe6a22ef
Bug 1239459 - Toggle touch event simulation r=jryans

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c8241f3e1f53 for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9275122&repo=fx-team

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/c3ff04109a8a6e439be36ae44133b2ba68fc79f6
Bug 1239459 - Toggle touch event simulation r=jryans

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3ff04109a8a

Comment 19

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

Since we aren't reloading yet, this is pretty hard to test manually.  Let's do testing once reload is in place.