Touch events button to turn on simulation of mobile touch events

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.

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.

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

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 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.

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

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

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

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