1521814 - Make RDM override meta viewport only when a new pref is set (defaulting to false).

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect)

Description

[Brad Werth [:bradwerth]]

Bug 1290420 turned on our handling of the meta viewport whenever touch simulation was on in Responsive Design Mode. This created many UX problems that we didn't catch before the feature landed in release.

Let's retroactively put meta viewport handling behind a pref, default to false. Any tests that are requiring meta viewport handling can have the pref set to true.

Comment 1

[Brad Werth [:bradwerth]]

Whoa, there already is a pref, dom.meta-viewport.enabled that seems to be named appropriately. It may be sufficient to apply this pref to the relevant code. If not, and we do need to make a new pref, we'll have to name the new pref even more precisely.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1521814 Part 1: Make the existing dom.meta-viewport-enabled pref act as an off-switch for desktop meta viewport.

Comment 3

[Brad Werth [:bradwerth]]

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

Comment 4

[Brad Werth [:bradwerth]]

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

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1521814 Part 1: Make RDM meta viewport override dependent on a new pref, set to false for now. r=gl!

Comment 6

[Brad Werth [:bradwerth]]

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

Comment 7

[Brad Werth [:bradwerth]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/864bf1afa755
Part 1: Make RDM meta viewport override dependent on a new pref, set to false for now. r=gl

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/864bf1afa755

Comment 10

[Brad Werth [:bradwerth]]

I can't see the details for the beta uplift approval, so I'm repeating them (imperfectly) here:

Feature/Bug causing the regression: 1290420
User impact if declined: Responsive Design Mode will have various UX problems when displaying documents with a meta viewport tag.
Is this code covered by automated tests? Yes.
Has the fix been verified in Nightly? No.
Needs manual test from QE? No.
If yes, steps to reproduce:
List of other uplifts needed: None.
Risk to taking this patch: Medium.
Why is the change risky/not risky?
(and alternatives if risky): This patch puts the work of Bug 1290420 behind a pref and defaults that pref to false. If Nightly users have adjusted their content or workflow to compensate, those users will be surprised by this change. They will have to set the new pref to true to restore current behavior.
String changes made/needed: None.

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9038686 [details]
Bug 1521814 Part 1: Make RDM meta viewport override dependent on a new pref, set to false for now. r=gl!

changes landing on central this week get merged automatically to beta for 66, with no manual uplifts.

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 9038686 [details]
Bug 1521814 Part 1: Make RDM meta viewport override dependent on a new pref, set to false for now. r=gl!

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1290420

User impact if declined

Responsive Design Mode will have various UX problems when displaying documents with a meta viewport tag.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Medium

Why is the change risky/not risky? (and alternatives if risky)

This patch puts the work of Bug 1290420 behind a pref and defaults that pref to false. If Nightly or Beta users have adjusted their content or workflow to compensate, those users will be surprised by this change. They will have to set the new pref to true to restore current behavior. However, there have been no release notes detailing the existence of this pref, so they won't know how to restore the current (problematic) behavior.

String changes made/needed

Comment 13

[Brad Werth [:bradwerth]]

If possible, we want this in Release 65 to circumvent the problems we've seen in Release 64. In my uplift request notes above, I misspoke. If RELEASE 64 users have adjusted to compensate for the problems with meta viewport handling in RDM, they will be surprised by this change.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I'm a bit nervous about preffing off a feature we've already been shipping since 64. Do we have any idea of # of affected users? Is that something we should be relnoting?

Comment 15

[Brad Werth [:bradwerth]]

I agree that this is unusual and Not a Good Precedent. It may be worth doing anyway, since the behavior with the feature on is pretty bad (Bug 1501665). If we do decide to uplift the change, then we should add a release note. Here's a proposal:

Added pref "devtools.responsive.metaViewport.enabled", defaulting to false, turning off support for <meta> viewport tags in Responsive Design Mode. This pref controls whether Firefox Responsive Design Mode will attempt to respect the size constraints in a <meta> viewport tag, when RDM Touch Simluation is toggled on. Our current implementation of meta viewports in RDM has problems with sizing the viewport correctly. If you want to restore the previous behavior, set this pref to true.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9038686 [details]
Bug 1521814 Part 1: Make RDM meta viewport override dependent on a new pref, set to false for now. r=gl!

[Triage Comment]
Not crazy about turning off an already-shipped feature in a dot release, but it sounds like the alternative is pretty bad from a UX standpoint too. Approved for 65.0.1.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d98c887912e4

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi guys, I noticed that since this functionality is something that can be controlled via a pref flip. Should we consider shipping this when ready via a staged rollout (slowly turn it on for release population) rather than on-by-default? https://wiki.mozilla.org/Release_Management/Process_coordination_for_handling_off-train_releases#Process:_Feature_rollout_via_Normandy

This is a good strategy to rollout slowly and cautiously versus on-by-default on release launch day.

Comment 19

[Brad Werth [:bradwerth]]

(In reply to Ritu Kothari (:ritu) from comment #18)

Hi guys, I noticed that since this functionality is something that can be controlled via a pref flip. Should we consider shipping this when ready via a staged rollout (slowly turn it on for release population) rather than on-by-default? https://wiki.mozilla.org/Release_Management/Process_coordination_for_handling_off-train_releases#Process:_Feature_rollout_via_Normandy

This is a good strategy to rollout slowly and cautiously versus on-by-default on release launch day.

When we roll it out (again), we can certainly do it in a staged fashion.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Brad. Happy to help discuss the plan when you guys reach that point.

Comment 21

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 66.0a1 (build ID:20190122215349) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 65.0.1 and Fx 66.0b7 on Windows 10 x64, Ubuntu 16.04 LTS and macOs 10.13.

Comment 22

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

Clearing my needinfo since Brad already responded.