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

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[Tim Nguyen :ntim]

STR:
- Open RDM
- Enable touch
- Resize viewport

Actual result:
- touch gets disabled

ER:
- touch should stay enabled

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1319950 - DPR watching should move to actor.

Review commit: https://reviewboard.mozilla.org/r/106232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106232/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1319950 - Wrap dPR state in object for consistency.

Review commit: https://reviewboard.mozilla.org/r/106234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106234/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1319950 - Use consistent naming with viewport properties.

Use "change" instead of "update" terminology with viewport properties.
Remove redundant "viewport" word from viewport changes.

Review commit: https://reviewboard.mozilla.org/r/106236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106236/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1319950 - Only clear properties on resize if device is active.

If a device is active, remove it on resize.  If not, leave device properties
alone, so that things like touch simulation stay enabled when resizing without
a device.

Review commit: https://reviewboard.mozilla.org/r/106238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106238/

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 6

[Tim Nguyen :ntim]

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 7

[Tim Nguyen :ntim]

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 8

[Gabriel [:gl] (ΦωΦ)]

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.

Comment 9

[Tim Nguyen :ntim]

(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 10

[Gabriel [:gl] (ΦωΦ)]

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.

Comment 11

[Gabriel [:gl] (ΦωΦ)]

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 12

[Gabriel [:gl] (ΦωΦ)]

Comment on attachment 8829025 [details]
Bug 1319950 - Only clear properties on resize if device is active.

https://reviewboard.mozilla.org/r/106238/#review107414

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8829023 [details]
Bug 1319950 - Wrap dPR state in object for consistency.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106234/diff/1-2/

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106236/diff/1-2/

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8829025 [details]
Bug 1319950 - Only clear properties on resize if device is active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106238/diff/1-2/

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8829022 [details]
Bug 1319950 - DPR watching should move to actor.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f1cefc5b510
https://hg.mozilla.org/mozilla-central/rev/b6076330933b
https://hg.mozilla.org/mozilla-central/rev/a0c4fc1d8ae7
https://hg.mozilla.org/mozilla-central/rev/54350550ed2c

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

See comment 23.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8829025 [details]
Bug 1319950 - Only clear properties on resize if device is active.

See comment 23.

Comment 26

[Julien Cristau [:jcristau]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/227831ce1a7b
https://hg.mozilla.org/releases/mozilla-aurora/rev/b48c0364b756
https://hg.mozilla.org/releases/mozilla-aurora/rev/c933e719a567

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a6cb4268d7b4
https://hg.mozilla.org/releases/mozilla-beta/rev/7ea353eafc60
https://hg.mozilla.org/releases/mozilla-beta/rev/bf69066e6917

Comment 29

[Simona Badau]

Attachment: 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?

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 31

[Simona Badau]

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.

Comment 32

[Mihai Boldan, QA [:mboldan]]

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