J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Description • 9 years ago

Touch events button to turn on simulation of mobile touch events

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 2 • 8 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 3 • 8 years ago

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.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 4 • 8 years ago

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(?).

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 5 • 8 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 6 • 8 years ago

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!)

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 8 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 9 • 8 years ago

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/

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 10 • 8 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 11 • 8 years ago

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.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 13 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 14 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 15 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 16 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 17 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 19 • 8 years ago

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