MobileViewportManager incorrectly zooms content when shrinking display width

RESOLVED FIXED in Firefox -esr68

Status

()

defect
P1
normal
RESOLVED FIXED
3 months ago
24 days ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

(Blocks 1 bug, Regression)

unspecified
mozilla69
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr6869+ fixed, firefox68 wontfix, firefox69 fixed)

Details

(Whiteboard: [rdm-mvp])

Attachments

(9 attachments)

Posted file minimum-scale.html

Steps to Reproduce:

  1. Set the pref 'devtools.responsive.metaViewport.enabled' to true.
  2. Load the attached minimum-scale.html.
  3. Open Responsive Design Mode.
  4. Resize the viewport to 300 x 600.
  5. Turn on Touch Simulation (which also turns on meta viewport support).
  6. Click the button to rotate the viewport.
  7. Again click the button to rotate the viewport.

Expected Results:
The content zoom level should match the initial zoom level (as seen after Step 5).

Actual Results:
The content zoom level is larger compared to the initial zoom level.

Assignee: nobody → bwerth

The existing math is attempting to adjust the display scale ratio to
prevent the new size from landing outside the min/max zoom bounds. This
introduces unwanted side effects that can be avoided by simply clamping
to min/max at the end. The remaining math correctly handles the case
where the zoom has ALREADY been clamped, which is the only important
case.

This is a drive-by fix to turn a division into a multiplication. It also
is more correct in that the existing code attempts a divide-by-zero if
aNewViewport.width is zero. The updated code will instead return a zoom
of zero in such a case.

QA Whiteboard: [rdm-mvp]
Priority: -- → P1
Whiteboard: [rdm-mvp]
Status: NEW → ASSIGNED
Attachment #9068572 - Attachment description: Bug 1555511 Part 3: Add a test of resizing a document with meta viewport minimum-scale. → Bug 1555511 Part 4: Add a test of resizing a document with meta viewport minimum-scale.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1069871269f7
Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation. r=botond
https://hg.mozilla.org/integration/autoland/rev/999f3af17edf
Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth. r=botond
https://hg.mozilla.org/integration/autoland/rev/aa1c32a38785
Part 3: Refactor viewport resize test helper functions. r=botond
https://hg.mozilla.org/integration/autoland/rev/6988b151bbe7
Part 4: Add a test of resizing a document with meta viewport minimum-scale. r=botond

Backed out 4 changesets (bug 1555511) for failing at browser_viewport_resizing_minimum_scale.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/3a1135e99b22392d170911105dbdc3f09c0c4085

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=6988b151bbe73aaa04e07590c45642a14f13bd46&selectedJob=249623065

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249623065&repo=autoland&lineNumber=1218

Log snippet:

16:45:21 INFO - --- Starting viewport test output ---
16:45:21 INFO - Meta Viewport ON setting meta viewport support.
16:45:21 INFO - Reload is needed -- waiting for it.
16:45:21 INFO - GECKO(1695) | console.log: "[DISPATCH] action type:" "CHANGE_VIEWPORT_ANGLE"
16:45:21 INFO - Current size: 320 x 480, set to: 300 x 600
16:45:21 INFO - Waiting for viewport-resize to 300 x 600
16:45:21 INFO - GECKO(1695) | console.log: "[DISPATCH] action type:" "RESIZE_VIEWPORT"
16:45:21 INFO - Got viewport-resize to 300 x 600
16:45:21 INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON before resize should have expected zoom. -
16:45:21 INFO - TEST-INFO | started process screencapture
16:45:21 INFO - TEST-INFO | screencapture: exit 0
16:45:21 INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON before resize should have expected layout width. - Got 320, expected 300
16:45:21 INFO - Stack trace:
16:45:21 INFO - chrome://mochikit/content/browser-test.js:test_is:1325
16:45:21 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/head.js:testViewportZoomWidthAndHeight:600
16:45:21 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js:null:57
16:45:21 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/head.js:addRDMTask/<:116
16:45:21 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
16:45:21 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
16:45:21 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1000
16:45:21 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
16:45:21 INFO - Not taking screenshot here: see the one that was previously logged

Failed on Tier1 job on mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&searchStr=macosx1010-64-shippable%2Copt%2Cmochitests%2Ctest-macosx1010-64-shippable%2Fopt-mochitest-devtools-chrome-e10s-1%2Cm%28dt1%29&group_state=expanded&selectedJob=249639307

Status: RESOLVED → REOPENED
Flags: needinfo?(bwerth)
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24e136f27469
Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation. r=botond
https://hg.mozilla.org/integration/autoland/rev/5e9e587d0622
Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth. r=botond
https://hg.mozilla.org/integration/autoland/rev/d9ce18a4d7cd
Part 3: Refactor viewport resize test helper functions. r=botond
https://hg.mozilla.org/integration/autoland/rev/80b9b4eb8305
Part 4: Add a test of resizing a document with meta viewport minimum-scale. r=botond

Backed out 4 changesets (Bug 1555511) for devtools failures at browser_viewport_resizing_minimum_scale.js

Backout link: Backed out 4 changesets (Bug 1555511) for devtools failures at browser_viewport_resizing_minimum_scale.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&group_state=expanded&revision=80b9b4eb8305f9dcaa35f0a355fbe673712e854d&searchStr=devtools&selectedJob=249883700

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249883700&repo=autoland&lineNumber=11755

Failure snippet:
[task 2019-06-04T09:15:01.704Z] 09:15:01 INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON before resize should have expected zoom. -
[task 2019-06-04T09:15:01.720Z] 09:15:01 INFO - TEST-INFO | started process screentopng
[task 2019-06-04T09:15:02.292Z] 09:15:02 INFO - TEST-INFO | screentopng: exit 0
[task 2019-06-04T09:15:02.294Z] 09:15:02 INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON before resize should have expected layout width. - Got 320, expected 300
[task 2019-06-04T09:15:02.295Z] 09:15:02 INFO - Stack trace:
[task 2019-06-04T09:15:02.295Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:test_is:1324
[task 2019-06-04T09:15:02.296Z] 09:15:02 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/head.js:testViewportZoomWidthAndHeight:600
[task 2019-06-04T09:15:02.296Z] 09:15:02 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js:null:57
[task 2019-06-04T09:15:02.297Z] 09:15:02 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/head.js:addRDMTask/<:116
[task 2019-06-04T09:15:02.298Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1115
[task 2019-06-04T09:15:02.298Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1143
[task 2019-06-04T09:15:02.299Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1004
[task 2019-06-04T09:15:02.299Z] 09:15:02 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-06-04T09:15:02.300Z] 09:15:02 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-06-04T09:15:02.301Z] 09:15:02 INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON before resize should have expected layout height. - Got 480, expected 600
[task 2019-06-04T09:15:02.302Z] 09:15:02 INFO - Stack trace:
[task 2019-06-04T09:15:02.302Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:test_is:1324
[task 2019-06-04T09:15:02.302Z] 09:15:02 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/head.js:testViewportZoomWidthAndHeight:603
[task 2019-06-04T09:15:02.303Z] 09:15:02 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js:null:57
[task 2019-06-04T09:15:02.303Z] 09:15:02 INFO - chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/head.js:addRDMTask/<:116
[task 2019-06-04T09:15:02.304Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1115
[task 2019-06-04T09:15:02.304Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1143
[task 2019-06-04T09:15:02.305Z] 09:15:02 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1004
[task 2019-06-04T09:15:02.306Z] 09:15:02 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-06-04T09:15:02.307Z] 09:15:02 INFO - Current size: 300 x 600, set to: 600 x 300
[task 2019-06-04T09:15:02.307Z] 09:15:02 INFO - Waiting for viewport-resize to 600 x 300
[task 2019-06-04T09:15:02.308Z] 09:15:02 INFO - GECKO(2777) | console.log: "[DISPATCH] action type:" "RESIZE_VIEWPORT"
[task 2019-06-04T09:15:02.311Z] 09:15:02 INFO - Got viewport-resize to 600 x 300
[task 2019-06-04T09:15:02.312Z] 09:15:02 INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON after resize should have expected zoom. -
[task 2019-06-04T09:15:02.313Z] 09:15:02 INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON after resize should have expected layout width. -
[task 2019-06-04T09:15:02.314Z] 09:15:02 INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_viewport_resizing_minimum_scale.js | Meta Viewport ON after resize should have expected layout height. -

Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/986dc438181c
Backed out 4 changesets for devtools failures at browser_viewport_resizing_minimum_scale.js CLOSED TREE
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/262d2cad6407
Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation. r=botond
https://hg.mozilla.org/integration/autoland/rev/2a85e3af81bb
Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth. r=botond
https://hg.mozilla.org/integration/autoland/rev/8c6e5561abed
Part 3: Refactor viewport resize test helper functions. r=botond
https://hg.mozilla.org/integration/autoland/rev/e0cd61fa79ed
Part 4: Add a test of resizing a document with meta viewport minimum-scale. r=botond
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c0c97f11fa4
Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation. r=botond
https://hg.mozilla.org/integration/autoland/rev/872fc290dbdb
Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth. r=botond
https://hg.mozilla.org/integration/autoland/rev/794ac0b82ba8
Part 3: Refactor viewport resize test helper functions. r=botond
https://hg.mozilla.org/integration/autoland/rev/addbb08ba89d
Part 4: Add a test of resizing a document with meta viewport minimum-scale. r=botond

I relanded the changesets as It was not the culprit for the dt failures at browser_boxmodel_pseudo-element.js

Flags: needinfo?(bwerth)

These patches fix a webcompat issue which affects Fennec users.

Could we request approval for 68.1esr uplift for the patches, so that Fennec users can benefit from the fix?

Flags: needinfo?(bwerth)

Comment on attachment 9074896 [details] [diff] [review]
Bug1555511-esr68-Part1.patch

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes layout during device orientation for several top 100 sites.
  • User impact if declined: Sites with meta viewport width="device-width" tags will zoom unexpectedly when rotating from landscape to portrait.
  • Fix Landed on Version: 69
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changes the math involved in viewport sizing, but doesn't invoke any new code paths in layout.
  • String or UUID changes made by this patch:
Flags: needinfo?(bwerth)
Attachment #9074896 - Flags: approval-mozilla-esr68?
Attachment #9074897 - Flags: approval-mozilla-esr68?
Attachment #9074898 - Flags: approval-mozilla-esr68?
Attachment #9074899 - Flags: approval-mozilla-esr68?
Regressed by: 1501665
Comment on attachment 9074896 [details] [diff] [review]
Bug1555511-esr68-Part1.patch

Fixes unexpected zooming on mobile devices when rotating from landscape to portrait. Approved for 68.1esr and Fennec 68.1b1.
Attachment #9074896 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9074897 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9074898 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9074899 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.