Enabling touch simulation then resizing the viewport should not disable touch simulation

VERIFIED FIXED in Firefox 52

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: ntim, Assigned: jryans)

Tracking

unspecified
Firefox 54
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 unaffected, firefox52 verified, firefox53 verified, firefox54 verified)

Details

(Whiteboard: [rdm-v2])

Attachments

(5 attachments)

STR:
- Open RDM
- Enable touch
- Resize viewport

Actual result:
- touch gets disabled

ER:
- touch should stay enabled
Priority: -- → P2
Whiteboard: [triage][rdm-v2] → [rdm-v2]
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Priority: P2 → P1
Blocks: 1313140
Comment on attachment 8829022 [details]
Bug 1319950 - DPR watching should move to actor.

https://reviewboard.mozilla.org/r/106232/#review107252

::: devtools/client/responsive.html/index.js:104
(Diff revision 1)
>  // Dispatch a `changeDisplayPixelRatio` action when the browser's pixel ratio is changing.
>  // This is usually triggered when the user changes the monitor resolution, or when the
>  // browser's window is dragged to a different display with a different pixel ratio.
> +// TODO: It would be better to move this watching into the actor, so that it can be
> +// better synchronized with any overrides that might be applied.  Also, reading a single
> +// value like this makes less sense with multiple viewports.

nit: reading a value
Comment on attachment 8829024 [details]
Bug 1319950 - Use consistent naming with viewport properties.

https://reviewboard.mozilla.org/r/106236/#review107254

Is "viewport" really redundant in the names? I expect we'll likely re-introduce it with multiple viewports, so I see no point of removing viewport now.
Comment on attachment 8829023 [details]
Bug 1319950 - Wrap dPR state in object for consistency.

https://reviewboard.mozilla.org/r/106234/#review107288

::: devtools/client/responsive.html/types.js:100
(Diff revision 1)
> - * The location of the document displayed in the viewport(s).
> - */
> -exports.location = PropTypes.string;
>  
>  /**
> - * The progression of the screenshot
> + * devicePixelRatio for a given viewport.

s/devicePixelRatio/Device Pixel Ratio

::: devtools/client/responsive.html/types.js:102
(Diff revision 1)
> -exports.location = PropTypes.string;
>  
>  /**
> - * The progression of the screenshot
> + * devicePixelRatio for a given viewport.
>   */
> -exports.screenshot = {
> +const pixelRatio = exports.pixelRatio = {

Let's move pixelRatio below networkThrottling to maintain an alphabetical order for the VIEWPORT types.

::: devtools/client/responsive.html/types.js:104
(Diff revision 1)
>  /**
> - * The progression of the screenshot
> + * devicePixelRatio for a given viewport.
>   */
> -exports.screenshot = {
> +const pixelRatio = exports.pixelRatio = {
>  
> -  isCapturing: PropTypes.bool,
> +  // The devicePixelRatio value

s/devicePixelRatio/Device Pixel Ratio

::: devtools/client/responsive.html/types.js:154
(Diff revision 1)
> +  // The devicePixelRatio of the viewport
> +  pixelRatio: PropTypes.shape(pixelRatio),
> +
> +};
> +
> +/* ACTION */

I like to keep things sorted in alphabetical order. Perhaps consider moving these ACTION types above GLOBAL.
Attachment #8829023 - Flags: review?(gl) → review+
(In reply to Tim Nguyen :ntim from comment #6)
> Comment on attachment 8829022 [details]
> Bug 1319950 - DPR watching should move to actor.
> 
> https://reviewboard.mozilla.org/r/106232/#review107252
> 
> ::: devtools/client/responsive.html/index.js:104
> (Diff revision 1)
> >  // Dispatch a `changeDisplayPixelRatio` action when the browser's pixel ratio is changing.
> >  // This is usually triggered when the user changes the monitor resolution, or when the
> >  // browser's window is dragged to a different display with a different pixel ratio.
> > +// TODO: It would be better to move this watching into the actor, so that it can be
> > +// better synchronized with any overrides that might be applied.  Also, reading a single
> > +// value like this makes less sense with multiple viewports.
> 
> nit: reading a value

Hmm, Mozreview cut off "a single" form the viewport. I guess the comment is fine then.
Comment on attachment 8829024 [details]
Bug 1319950 - Use consistent naming with viewport properties.

https://reviewboard.mozilla.org/r/106236/#review107398

::: devtools/client/responsive.html/app.js:51
(Diff revision 1)
>    onBrowserMounted() {
>      window.postMessage({ type: "browser-mounted" }, "*");
>    },
>  
> +  onChangeDevice(id, device) {
> +    // XXX: Get rid of this extra message?

Do we want to keep these comments?

::: devtools/client/responsive.html/manager.js:455
(Diff revision 1)
>  
>      switch (event.data.type) {
> +      case "change-device":
> +        this.onChangeDevice(event);
> +        break;
> +      case "change-pixel-ratio":

Move "change-pixel-ratio" below "change-network-throtting" case to keep the switch block alphabetically ordered.
Attachment #8829024 - Flags: review?(gl) → review+
Comment on attachment 8829024 [details]
Bug 1319950 - Use consistent naming with viewport properties.

https://reviewboard.mozilla.org/r/106236/#review107398

> Do we want to keep these comments?

Dropping this issue since it is removed in the next patch
Comment on attachment 8829025 [details]
Bug 1319950 - Only clear properties on resize if device is active.

https://reviewboard.mozilla.org/r/106238/#review107414
Attachment #8829025 - Flags: review?(gl) → review+
Comment on attachment 8829024 [details]
Bug 1319950 - Use consistent naming with viewport properties.

https://reviewboard.mozilla.org/r/106236/#review107398

> Dropping this issue since it is removed in the next patch

I removed them anyway, there were WIP notes not worth keeping.

> Move "change-pixel-ratio" below "change-network-throtting" case to keep the switch block alphabetically ordered.

Fixed.
Comment on attachment 8829024 [details]
Bug 1319950 - Use consistent naming with viewport properties.

https://reviewboard.mozilla.org/r/106236/#review107254

In my view, it will remain redundant even with multiple viewports.  Nearly everything in RDM is about a viewport, so it seems unnecessary to include that word all over the place.
Comment on attachment 8829023 [details]
Bug 1319950 - Wrap dPR state in object for consistency.

https://reviewboard.mozilla.org/r/106234/#review107288

> s/devicePixelRatio/Device Pixel Ratio

I don't think "Device Pixel Ratio" is a good spelling of the name, since it's not really a proper noun...  I would think "devicePixelRatio" (the name of the window property for this value) or "device pixel ratio" are good spellings.  We're not very consistent right now, so I filed bug 1333254 about this.

In this case, I guess I'll use "Device pixel ratio".

> Let's move pixelRatio below networkThrottling to maintain an alphabetical order for the VIEWPORT types.

Seems reasonable, fixed.

> s/devicePixelRatio/Device Pixel Ratio

Fixed as above.

> I like to keep things sorted in alphabetical order. Perhaps consider moving these ACTION types above GLOBAL.

Yeah...  I am not sure how to balance the order of this file yet...  I have a mental model of "overall app state" that is something like "global, then viewports, then actions", but it's pretty nebulous.

I think I'll drop this one for now and keep thinking about it as I continue to make changes.

I want something richer like several levels of an outline, or maybe some way to list out the full app state, but I don't see clean way to do it yet.
Comment on attachment 8829022 [details]
Bug 1319950 - DPR watching should move to actor.

https://reviewboard.mozilla.org/r/106232/#review107948
Attachment #8829022 - Flags: review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6f1cefc5b510
DPR watching should move to actor. r=jryans
https://hg.mozilla.org/integration/autoland/rev/b6076330933b
Wrap dPR state in object for consistency. r=gl
https://hg.mozilla.org/integration/autoland/rev/a0c4fc1d8ae7
Use consistent naming with viewport properties. r=gl
https://hg.mozilla.org/integration/autoland/rev/54350550ed2c
Only clear properties on resize if device is active. r=gl
Flags: qe-verify+
Comment on attachment 8829023 [details]
Bug 1319950 - Wrap dPR state in object for consistency.

Approval Request Comment
[Feature/Bug causing the regression]: Touch simulation in DevTools RDM
[User impact if declined]: Touch simulation becomes disabled if you resize the viewport, which is expected to be a frequent activity.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: QE is flagged for testing, but I don't think it needs to block uplift.
[List of other uplifts needed for the feature/fix]: 3 patches in this bug are needed (another patch in this bug only changes comments and can be ignored).
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects touch in DevTools RDM.
[String changes made/needed]: None
Attachment #8829023 - Flags: approval-mozilla-beta?
Attachment #8829023 - Flags: approval-mozilla-aurora?
Comment on attachment 8829024 [details]
Bug 1319950 - Use consistent naming with viewport properties.

See comment 23.
Attachment #8829024 - Flags: approval-mozilla-beta?
Attachment #8829024 - Flags: approval-mozilla-aurora?
Comment on attachment 8829025 [details]
Bug 1319950 - Only clear properties on resize if device is active.

See comment 23.
Attachment #8829025 - Flags: approval-mozilla-beta?
Attachment #8829025 - Flags: approval-mozilla-aurora?
Comment on attachment 8829023 [details]
Bug 1319950 - Wrap dPR state in object for consistency.

devtools RDM fix for aurora53 and beta52, should be in b2
Attachment #8829023 - Flags: approval-mozilla-beta?
Attachment #8829023 - Flags: approval-mozilla-beta+
Attachment #8829023 - Flags: approval-mozilla-aurora?
Attachment #8829023 - Flags: approval-mozilla-aurora+
Attachment #8829024 - Flags: approval-mozilla-beta?
Attachment #8829024 - Flags: approval-mozilla-beta+
Attachment #8829024 - Flags: approval-mozilla-aurora?
Attachment #8829024 - Flags: approval-mozilla-aurora+
Attachment #8829025 - Flags: approval-mozilla-beta?
Attachment #8829025 - Flags: approval-mozilla-beta+
Attachment #8829025 - Flags: approval-mozilla-aurora?
Attachment #8829025 - Flags: approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170201030207

When trying to verify this, I noticed that the touch simulation is still lost when resizing the viewport if the touch simulation is enabled by selecting the device. 

Please see the screen cast for more detail. 

Ryan, is this expected in any way?
Flags: needinfo?(jryans)
(In reply to Simona B [:simonab ] from comment #29)
> Created attachment 8832836 [details]
> LostTouchSimulation.mp4
> 
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101
> Firefox/54.0
> Build ID: 20170201030207
> 
> When trying to verify this, I noticed that the touch simulation is still
> lost when resizing the viewport if the touch simulation is enabled by
> selecting the device. 
> 
> Please see the screen cast for more detail. 
> 
> Ryan, is this expected in any way?

Yes, that's "expected", at least from the way the code is written today.  It's quite hard to guess what would be most obvious to users...

But for the moment anyway, if you select a device and then resize manually, all device parameters are cleared including touch.  If however no device is selected and you enable touch, resizing should keep it enabled (that's what this bug has fixed).

We have some ideas to improve the UX of changing away from a device like you mention in bug 1329843, so hopefully it can be improved.
Flags: needinfo?(jryans)
Thank you Ryan for all the additional information. 

Verified as fixed on the latest Nightly 54.0a1 (Build ID: 20170201030207) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
I managed to reproduce this issue on Firefox 52.0a1 (2016-11-09), under Windows 10x64.
The issue is no longer reproducible on Firefox 52.0b2 (64-bit), or on Firefox 53.0a2 (2017-02-03).
Tests were performed under Windows 10x64, Mac OS X 10.12.1, Ubuntu 16.04x64
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.