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

When you first enable the touch simulator, the viewport needs to be reloaded in most cases for this to have an effect.  The reason is that touch event handlers are effectively ignored if they are added when the simulator is disabled.  

Since most event handlers are added on page load, the viewport needs to be reloaded with touch simulation enabled from the start so these handlers will be listening correctly.

The legacy RDM uses the Firefox notification bar to tell the user about this, and offers to reload or never tell you again:

"If the touch event listeners have been added earlier, the page needs to be reloaded. (reload) (never show again) (x)"

We should work out what UX we want to use in the new RDM.

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

We came up with some alternative approaches during the standup.  Here are all the options we've mentioned so far:

A. Show a reload notification like legacy RDM (see comment 0)
B. Make a platform change so that event handlers are still bound when touch events are disabled, which makes the reload unnecessary (unclear how difficult that is)
C. We could make an assumption that touch simulation is more likely to be used when selecting a device and auto-reload the page when this happens.  There are other features (applying a new user agent) as part of selecting a device that also need reload to be effective.  We could choose to pair this with a notification after the fact like "We reloaded the page to apply all device settings.  (Ok) (Don't do that again)".

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

Looking more closely at Chrome's behavior here, they appear to be able to enable touch simulation without reloading, so it seems they have something like the platform support we would need in option B.  

For UA changes, Chrome also does not appear to reload.  Since the server can deliver different content based on UA, they may be just avoiding the issue so that the page does not reload, as opposed to forcing a reload for correctness.

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

Since it seems Chrome's behavior is a bit different than we discussed at this morning's standup, let's discuss it again tomorrow before moving forward.

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

We discussed this again at today's standup.  We agreed it would be better to leave users in control of when reloads happen, unless there is clear user intent to use some feature that only needs to reload because a limitation in the tool.

For touch simulation, we would automatically reload when you click to enable touch simulation, since that is clear user intent to use the simulation feature.  (However, it would still be great if this could be avoided with the proper platform support to hold the touch listeners.)

For user agent changes, we would not reload automatically, as developers should understand that a reload is needed to fully apply a UA change.

For changes to the device selector, it's unclear if the user only wanted the size of the device, or also wanted touch simulation, when changing to a device.  Since there is an explicit button to control touch simulation, we would not set touch simulation automatically when choosing a device to avoid the reload.  Instead, it can be enabled separately using the simulation button.

We may revise this in the future if platform support for the touch listeners is added, since we would no longer have a "reload tax" to pay for touch simulation in that case.

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

More notes from research into Chrome's impl:

* When Chrome toggles touch emulation, they change platform settings and re-run layout (without reloading)[1]
* Chrome installs a script into the page[2] to be run on each load that fakes ontouch* on document and window, presumably to convince feature detection that the events exist (if your page uses this you **would** need to manually reload in Chrome first)

[1]: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/DevToolsEmulator.cpp&sq=package:chromium&type=cs&l=320&rcl=1463411924
[2]: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/devtools/front_end/emulation/TouchModel.js&q=setTouchEnabled%20file:%5Esrc/third_party/WebKit/Source/devtools/front_end/emulation/&sq=package:chromium&type=cs&l=64

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

I filed bug 1273333 to investigate touch simulation without reloading on the platform side.

	Benoit Chabod [:bigben] Assignee
	Comment 7 • 8 years ago

Is this enough?
I guess there are still issues with touch simulation state (see Bug 979324 and Bug 1188270), but I think this patch fixes this bug.

By the way, there was an issue with the button: the visual state didn't match the real state.
Indeed, in app.js, the simulator was started before the actual redux store boolean was updated [1].
This should be fixed now.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/app.js#85

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

Comment on attachment 8764565 [details] [diff] [review]
1st patch - reload viewport when enabling touch simulation

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

The looks like the right approach overall!  Just a few refactoring things I'd like to see as well.

::: devtools/client/responsive.html/app.js
@@ +82,4 @@
>      this.props.dispatch(updateDeviceModalOpen(isOpen));
>    },
>  
>    onUpdateTouchSimulationEnabled() {

I don't like that this function differs from its neighbors by assuming it should do the toggling itself.

Please change it to take an `enabled` argument that is the new value, and removing all the negation here.  Then in global-toolbar which calls this, change it to pass the negated value.

::: devtools/client/responsive.html/manager.js
@@ +399,5 @@
>    },
>  
>    updateTouchSimulation: Task.async(function* (enabled) {
>      if (enabled) {
> +      this.touchEventSimulator.start().then(reloadNeeded => {

Since you're in an async function, you can yield on the result and avoid the .then():

let reloadNeeded = yield this.touchEventSimulator.start();

@@ +401,5 @@
>    updateTouchSimulation: Task.async(function* (enabled) {
>      if (enabled) {
> +      this.touchEventSimulator.start().then(reloadNeeded => {
> +        if (reloadNeeded) {
> +          this.toolWindow.document.querySelector("iframe.browser").reload();

I'd prefer that we go through a common path to access the viewport browser from the manager.  This will help to highlight things that need attention when we change to multiviewport.

The tool window's index.js does expose getViewportBrowser, but it's an odd thing that just exposes the message manager for tests.  Let's try the following:

1. Change getViewportBrowser to pull the actual browser element (instead of the frameLoader within)
2. Have it add a `messageManager` getter if that property is not defined.
3. getViewportBrowser then returns the actual browser element (update the comments to make it not just for tests)
4. In manager.js, also update its getViewportBrowser so its not just for tests
5. Use this.getViewportBrowser() here and also in init where it returns toolViewportContentBrowser

	Benoit Chabod [:bigben] Assignee
	Comment 9 • 8 years ago

Here's a second patch that does the refactoring things you asked for :)
It should be easier to test the touch simulation mode once this lands!

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

Comment on attachment 8765446 [details] [diff] [review]
2nd patch - reload viewport when enabling touch simulation

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

Looks good overall, just a few tweaks left.

Thanks for working on it!

::: devtools/client/responsive.html/components/global-toolbar.js
@@ +49,5 @@
>        dom.button({
>          id: "global-touch-simulation-button",
>          className: touchButtonClass,
> +        onClick: () => {
> +          onUpdateTouchSimulation(!touchSimulation.enabled);

If it fits within 90 chars, put this on one line:

onClick: () => onUpdateTouchSimulation(!touchSimulation.enabled),

::: devtools/client/responsive.html/index.js
@@ +128,5 @@
>   */
>  window.getViewportBrowser = () => {
> +  let browser = document.querySelector("iframe.browser");
> +  if (!browser.messageManager) {
> +    browser.messageManager = browser.frameLoader.messageManager;

The messageManager is a complex platform object, and I would be worried about possible leaks by just copying the actual value like this.  Instead, let's add a getter function:

Object.defineProperty(browser, "messageManager", {
  get() {
    return this.frameLoader.messageManager;
  },
  configurable: true,
  enumerable: true,
});

	Benoit Chabod [:bigben] Assignee
	Comment 11 • 8 years ago

Thanks for the review! :)
Here's a new patch, onClick fits in one line and I now use defineProperty to create an access to messageManager in the browser object (as you suggested).

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

	Pulsebot
	Comment 12 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/17fdf66c7ed1
Reload viewport when enabling touch simulation, r=jryans

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

https://hg.mozilla.org/mozilla-central/rev/17fdf66c7ed1

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 14 • 8 years ago

I managed to reproduce this issue on Firefox 49.0a1 (2016-05-25) and on Windows 10 x64.
The issue is no longer reproducible on Firefox 50.0a1(2016-07-12).
Tests were performed under Mac OS X 10.11.1, Ubuntu 14.04 x64 and Windows 10 x64, using STR from Bug 1273333.
I am marking this issue Verified Fixed.