1503902 - RDM loses "rotate viewport" setting when closing

Status: VERIFIED FIXED

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

Description

[Martin Balfanz [:mbalfanz]]

RDM should store the rotation setting as well when closing and reopening.

STR:
- enter RDM
- rotate viewport
- close and reopen RDM

ER:
- RDM should restore all previous settings incl. rotation

AR:
- RDM does restore previous settings, but not rotation

Comment 1

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

Attachment: 1503902.patch [1.0]

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/969a48cb4f17
RDM should restore the previous state when reopening after rotating the viewport. r=rcaliman

Comment 3

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/969a48cb4f17

Comment 4

[Martin Balfanz [:mbalfanz]]

Attachment: rdm.mp4

This doesn't work for me. See the video attached from latest Nightly. Same happens for me on M-C

Comment 5

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

(In reply to Martin Balfanz [:mbalfanz] from comment #4)
> Created attachment 9024504 [details]
> rdm.mp4
> 
> This doesn't work for me. See the video attached from latest Nightly. Same
> happens for me on M-C

So, the problem is that you had a device selected, and rotating the viewport when a device is selected does not remove the device association.

Comment 6

[Patrick Brosset <:pbro>]

Yeah I see the problem too. When we rotate the viewport, we end up storing the new width/height in prefs. But when we select a device, we also set that device in a pref. So when closing/re-opening RDM, we restore the device (because one was stored) instead of the custom width/height.

I think what we should do here is when a device is selected, as soon as you click the rotate button, then we should forget that device and store the new width/height and restore that when reopening RDM.
That would make sense to me.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug 1503902 - Forget current device when rotating the viewport; r?gl!

Comment 8

[Patrick Brosset <:pbro>]

This is an attempt at fixing this. I have never worked with this part of the code before, so might be missing something important. But it seems to be working.

Comment 9

[Martin Balfanz [:mbalfanz]]

As a user, I don't think this makes sense to me. It implies that rotating changes the device, but I still want the same device. If I close and reopen devtools and loose my reference to the device it will be confusing, and eventually I will not be sure anymore what I'm looking at.

IMHO we should restore:
- selected device or custom
- dimensions based on ^, if custom
- rotation setting to up the viewport in the right way

Comment 10

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

(In reply to Martin Balfanz [:mbalfanz] from comment #9)
> As a user, I don't think this makes sense to me. It implies that rotating
> changes the device, but I still want the same device. If I close and reopen
> devtools and loose my reference to the device it will be confusing, and
> eventually I will not be sure anymore what I'm looking at.
> 
> IMHO we should restore:
> - selected device or custom
> - dimensions based on ^, if custom
> - rotation setting to up the viewport in the right way

Ya, I was thinking about it this morning, and I fell to the same conclusion that rotating a device should preserve the device settings. So, we should realistically restore the stored width/height when restoring the device instead of grabbing it from the device setting.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 9024660 [details]
Bug 1503902 - Forget current device when rotating the viewport; r?gl!

Ok that makes, thanks for the feedback on this attempt. Let me remove this attachment then.
I'll let Gabriel (or whoever wants to try this) take it from there as this was just a quick attempt on my part of what I thought was the right approach.

Comment 12

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

Attachment: Bug 1503902 - Avoid resizing RDM when restoring the device state and simply use the stored pref width and height [1.0]

In the previous patch, we already store the width/height in prefs. So, we simply avoid resizing the viewport to the device size (since this is not correct when we rotate the device) when we restore RDM and allow the Redux store of the viewport to restore its width/height on open.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/facf21932367
Avoid resizing RDM when restoring the device state and simply use the stored pref width and height. r=rcaliman

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/facf21932367

Comment 15

[Daniel Cicas [:dcicas], Release QA]

I reproduced this issue using Fx 65.0a1, build ID: 20181101220058, on Windows 10 x64.

I can confirm this issue is fixed, I verified using Fx 65.0b11, build ID: 20190114172331, on Windows 10 x64, macOS X 10.13 and Ubuntu 18.04.