Apply touch simulation from selected device

VERIFIED FIXED in Firefox 51

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: jryans, Assigned: zer0)

Tracking

(Depends on 1 bug)

Trunk
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox51 verified)

Details

(Whiteboard: [multiviewport] [reserve-rdm])

Attachments

(1 attachment, 1 obsolete attachment)

Bug 1241714 adds a selected device.  We need to apply the touch simulation setting when it's selected.
Flags: qe-verify+
Priority: -- → P1
Whiteboard: [multiviewport][triage] → [multiviewport] [mvp-rdm]
QA Contact: mihai.boldan
Priority: P1 → P2
Priority: P2 → P1
Priority: P1 → P2
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [reserve-rdm]
Whiteboard: [multiviewport] [reserve-rdm] → [multiviewport] [mvp-rdm]
Assignee: nobody → gl
Posted patch 1254388.patch (obsolete) — Splinter Review
Attachment #8752697 - Flags: review?(jryans)
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!
Attachment #8752697 - Flags: review?(jryans)
Depends on: 1273333
Depends on: 970346
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?
Flags: needinfo?(mmucci)
Will do, thanks Ryan.
Assignee: gl → nobody
Flags: needinfo?(mmucci)
Priority: P2 → P3
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [reserve-rdm]
Priority: P3 → P2
Depends on: 1285566
No longer depends on: 970346
Priority: P2 → P3
Assignee: nobody → gl
Status: NEW → ASSIGNED
I am assuming for RDM planning that this is not being actively worked on.  Please reassign if that's incorrect.
Assignee: gl → nobody
Status: ASSIGNED → NEW
Assignee: nobody → zer0
Status: NEW → ASSIGNED
Iteration: --- → 51.3 - Sep 12
Priority: P3 → P1
Hey Tim, do you have time to review this patch? It's based on top of the work of bug 1254385.
Attachment #8752697 - Attachment is obsolete: true
Depends on: 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.
Attachment #8789863 - Flags: review?(ntim.bugs)
needinfo for the UX question in the previous comment.
Flags: needinfo?(zer0)
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.
Flags: needinfo?(zer0)
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 :)
Attachment #8789863 - Flags: review?(ntim.bugs) → review+
(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.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/d566e2962e1f
Apply touch simulation from selected device. r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d566e2962e1f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-rdm]
Flags: qe-verify+
Depends on: 1302879
No longer depends on: 1440678
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.