Bug 1241714 adds a selected device. We need to apply the touch simulation setting when it's selected.

Comment on attachment 8752697 [details] [diff] [review] 1254388.patch Review of attachment 8752697 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking into this so fast! Overall it looks good assuming we decide to move forward with this feature. For the moment though, since we'll eventually make touch enabling reload the page (bug 1269882 comment 4), I don't think we want to add this feature since it's not a direct user intent to have touch support when choosing a device, and we decided to avoid reloading for such cases. If we do find a way to get platform support for touch simulation without reloading, we can make use of your work here. Sorry for not updating the status of this bug more quickly!

Since this is blocked on platform of unknown complexity (bug 1273333), let's move it out of MVP to the reserve list. Marco, can you make this change?

Will do, thanks Ryan.

I am assuming for RDM planning that this is not being actively worked on. Please reassign if that's incorrect.

Hey Tim, do you have time to review this patch? It's based on top of the work of bug 1254385.

Comment on attachment 8789863 [details] Bug 1254388 - Apply touch simulation from selected device; https://reviewboard.mozilla.org/r/77920/#review76450 I like the consistency updates within the test. Thanks! So the main (and only) reason I'm not giving r+ is that: when you switch to a touch device, the touch button in the main RDM toolbar doesn't turn blue, which seems like confusing UX. If this is the expected UX, I'm happy to give r+ (although I do find the UX confusing). ::: devtools/client/responsive.html/test/browser/browser_device_change.js:47 (Diff revision 1) > + yield testTouchEventsOverride(ui, false); > testViewportSelectLabel(ui, "no device selected"); > > - let waitingPixelRatio = onceDevicePixelRatioChange(ui); > - > // Test device with custom UA nit: maybe update this comment to: // Test device with custom properties ::: devtools/client/responsive.html/test/browser/browser_device_change.js:62 (Diff revision 1) > + yield testDevicePixelRatio(ui, DEFAULT_DPPX); > + yield testTouchEventsOverride(ui, false); > testViewportSelectLabel(ui, "no device selected"); > - testDevicePixelRatio(yield waitingPixelRatio, DEFAULT_DPPX); > > // Test device where UA field is blank ...and this one to: // Test device with blank fields or // Test device with generic properties ::: devtools/client/responsive.html/test/browser/browser_device_change.js:66 (Diff revision 1) > > // Test device where UA field is blank > yield switchDevice(ui, "Laptop (1366 x 768)"); > yield waitForViewportResizeTo(ui, 1366, 768); > yield testUserAgent(ui, DEFAULT_UA); > + yield testTouchEventsOverride(ui, false); nit: might be worth checking the default devicePixelRatio as well for consistency with above.

needinfo for the UX question in the previous comment.

Comment on attachment 8789863 [details] Bug 1254388 - Apply touch simulation from selected device; https://reviewboard.mozilla.org/r/77920/#review76450 Thanks for the catch! It definitely wasn't intendend, I forgot to dispatch the proper action on react side. The new patch take care of that. On one side, currently if the user select a device with `touch` equals to `true`, the touch will be enabled and the button will be blue (active). However, if the user click on the button, it would disable the touch – as expected. We should probably check on UX if we want to have a different behavior – to me, it's fine having that, seems reasonable, since if the user disable the touch, even if a touch device is selected, we have a clear indicator (the state of the button) for that so it won't be misleading. If the UX wants to change this behavior, I would do that in a follow up bug.

Comment on attachment 8789863 [details] Bug 1254388 - Apply touch simulation from selected device; https://reviewboard.mozilla.org/r/77920/#review76932 r=me with nits addressed. Thanks! ::: devtools/client/responsive.html/app.js:51 (Diff revision 2) > window.postMessage({ > type: "change-viewport-device", > device, > }, "*"); > this.props.dispatch(changeDevice(id, device.name)); > + this.props.dispatch(updateTouchSimulationEnabled(device.touch || false)); nit: I'd prefer defining a default parameter here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/actions/touch-simulation.js#15 updateTouchSimulationEnabled(enabled = false) ::: devtools/client/responsive.html/test/browser/browser_device_change.js:109 (Diff revision 2) > + > + let flag = yield ui.emulationFront.getTouchEventsOverride(); > + is(flag === Ci.nsIDocShell.TOUCHEVENTS_OVERRIDE_ENABLED, expected, > + `Touch events override should be ${expected ? "enabled" : "disabled"}`); > + is(touchButton.classList.contains("active"), expected, > + `Touch simulation button is ${ expected ? "" : "not"} active.`); nit: I'd prefer using "should" in the wording instead of "is" because it's more consistent with the rest, and the tests are not guarenteed to always pass :)

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10) > Comment on attachment 8789863 [details] > Bug 1254388 - Apply touch simulation from selected device; > > https://reviewboard.mozilla.org/r/77920/#review76450 > On one side, currently if the user select a device with `touch` equals to > `true`, the touch will be enabled and the button will be blue (active). > However, if the user click on the button, it would disable the touch – as > expected. We should probably check on UX if we want to have a different > behavior – to me, it's fine having that, seems reasonable, since if the user > disable the touch, even if a touch device is selected, we have a clear > indicator (the state of the button) for that so it won't be misleading. Seems reasonable to me that the indicator is in sync with the actual state. > If the UX wants to change this behavior, I would do that in a follow up bug. Agreed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e4d19ccb10a&selectedJob=27381591 The oranges don't seem to be related.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/d566e2962e1f Apply touch simulation from selected device. r=ntim

I managed to reproduce this issue on Firefox 51.0a1 (2016-09-13) and on WIndows 10 x64. The issue is no longer reproducible on Firefox 51.0a1 (2016-09-15). The tests were performed under Windows 10x64, Mac OS X 10.11.1 and under Ubuntu 16.04 x64. I am marking this issue Verified Fixed.