1555511 - MobileViewportManager incorrectly zooms content when shrinking display width

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Brad Werth [:bradwerth]]

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

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1555511 Part 4: Add a test of resizing a document with meta viewport minimum-scale.

Depends on D32909

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1555511 Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation.

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.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1555511 Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth.

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.

Comment 4

[Brad Werth [:bradwerth]]

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

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1555511 Part 3: Refactor viewport resize test helper functions.

Depends on D32909

Comment 6

[Brad Werth [:bradwerth]]

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

Comment 7

[Pulsebot]

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

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1069871269f7
https://hg.mozilla.org/mozilla-central/rev/999f3af17edf
https://hg.mozilla.org/mozilla-central/rev/aa1c32a38785
https://hg.mozilla.org/mozilla-central/rev/6988b151bbe7

Comment 9

[Raul Gurzau (:RaulG)]

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

Comment 10

[Brad Werth [:bradwerth]]

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

Comment 11

[Pulsebot]

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

Comment 12

[Alexandru Michis [:malexandru]]

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

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/986dc438181c
Backed out 4 changesets for devtools failures at browser_viewport_resizing_minimum_scale.js CLOSED TREE

Comment 14

[Brad Werth [:bradwerth]]

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

Comment 15

[Brad Werth [:bradwerth]]

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

Comment 16

[Pulsebot]

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

Comment 17

[Andreea Pavel [:apavel]]

Backed out for failing dt at browser_boxmodel_pseudo-element.js

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

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250098634&repo=autoland&lineNumber=7073
and
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250100564&repo=autoland&lineNumber=11532

Backout: https://hg.mozilla.org/integration/autoland/rev/618e44acc245c567be2f635b933f845cd27a8c35

Comment 18

[Pulsebot]

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

Comment 19

[Alexandru Michis [:malexandru]]

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

Comment 20

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/9c0c97f11fa4
https://hg.mozilla.org/mozilla-central/rev/872fc290dbdb
https://hg.mozilla.org/mozilla-central/rev/794ac0b82ba8
https://hg.mozilla.org/mozilla-central/rev/addbb08ba89d

Comment 21

[Botond Ballo [:botond]]

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?

Comment 22

[Brad Werth [:bradwerth]]

Attachment: Bug1555511-esr68-Part1.patch

Comment 23

[Brad Werth [:bradwerth]]

Attachment: Bug1555511-esr68-Part2.patch

Comment 24

[Brad Werth [:bradwerth]]

Attachment: Bug1555511-esr68-Part3.patch

Comment 25

[Brad Werth [:bradwerth]]

Attachment: Bug1555511-esr68-Part4.patch

Comment 26

[Brad Werth [:bradwerth]]

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:

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/4eb8bea60c4d
https://hg.mozilla.org/releases/mozilla-esr68/rev/305d795fb86e
https://hg.mozilla.org/releases/mozilla-esr68/rev/5eefd1eebf97
https://hg.mozilla.org/releases/mozilla-esr68/rev/a80a09e9c87c

Comment 30

[Andrei Jolțan [:Andi]]

Hi, verified as fixed on Firefox 68.1b8 using the following devices:
• Samsung Galaxy Note 9 (Android 9)
• Samsung Galaxy S9 (Android 8)
• Samsung Galaxy S7 (Android 7)
• Samsung Galaxy S6 (Android 6.0.1)

Comment 31

[Diana Rus]

Hi, verified as fixed on Firefox Beta 70.0b3 with devices:

• Google Pixel 3 XL (Android 9)
• Samsung Galaxy Note 9 (Android 8.1.0)
• Sony Xperia Z5 (Android 7.0)