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

VERIFIED FIXED in Firefox 65

Status

defect
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

(Blocks 3 bugs, {regression})

64 Branch
Firefox 66
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 65+, firefox-esr60 unaffected, firefox65 verified, firefox66 verified)

Details

(Whiteboard: [qa-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 months ago

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.

Assignee

Comment 1

4 months ago

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.

Assignee

Updated

4 months ago
Summary: Put meta viewport handling behind a pref, default to false until RDM viewports are stable → Put meta viewport handling behind dom.meta-viewport.enabled pref, default to false on desktop until RDM viewports are stable
Assignee

Updated

4 months ago
Blocks: 1521934
Assignee

Updated

4 months ago
Assignee: nobody → bwerth
Assignee

Updated

4 months ago
Summary: Put meta viewport handling behind dom.meta-viewport.enabled pref, default to false on desktop until RDM viewports are stable → Make RDM override meta viewport only when a new pref is set (defaulting to false).
Attachment #9038359 - Attachment is obsolete: true

Comment 8

4 months ago
Pushed by bwerth@mozilla.com:
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

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee

Updated

4 months ago
Attachment #9038686 - Flags: approval-mozilla-beta?
Assignee

Comment 10

4 months ago

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

Attachment #9038686 - Flags: approval-mozilla-beta?
Assignee

Comment 12

4 months ago

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

Attachment #9038686 - Flags: approval-mozilla-release?
Assignee

Comment 13

4 months ago

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.

Keywords: regression
Version: unspecified → 64 Branch

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?

Flags: needinfo?(bwerth)
Assignee

Comment 15

4 months ago

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.

Flags: needinfo?(bwerth)

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.

Attachment #9038686 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: qe-verify+

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.

Flags: needinfo?(gl)
Flags: needinfo?(bwerth)
Assignee

Comment 19

3 months ago

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

Flags: needinfo?(bwerth)

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

Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Clearing my needinfo since Brad already responded.

Flags: needinfo?(gl)
You need to log in before you can comment on or make changes to this bug.