Closed Bug 1523844 Opened 5 years ago Closed 5 years ago

Pages don't fit the screen after rotating from landscape to portrait

Categories

(Core :: Panning and Zooming, defect, P1)

65 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: csheany, Assigned: bradwerth)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [rdm-mvp])

Attachments

(8 files, 13 obsolete files)

205.65 KB, image/jpeg
Details
216.45 KB, image/jpeg
Details
5.15 MB, video/mp4
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

+++ This bug was initially created as a clone of Bug #1509552 +++

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:65.0) Gecko/65.0 Firefox/65.0

Steps to reproduce:

  1. Open https://github.com/mozilla-mobile
  2. Request desktop site
  3. Rotate to landscape

Actual results:

The page is smaller than the screen. Zooming in even slightly fixes the issue but then in portrait the page is larger than screen.

Expected results:

The page should adjust correctly.

If I understand correctly, is it because the layout viewport doesn't match the visual viewport?

Flags: needinfo?(botond)

To clarify: is the issue still with the same website (https://github.com/mozilla-mobile)?

Flags: needinfo?(botond)

Yes.

Looking at the APZ Mini Map :) upon loading the red (layout) is inside the blue (visual)

Are you testing with a recent Nightly?

I tried this with a recent Nightly on both a phone and a tablet, and in both cases the page zooms in correctly to match the screen width after a rotation.

Yes.

Nightly 67.0a1 (2019-01-30)

Are you switching to landscape and back to portrait?

(In reply to csheany from comment #6)

Are you switching to landscape and back to portrait?

Just to landscape.

That hasn't regressed but portait is still larger than the screen afterward.

Ok, so if I understand correctly, the issue is that after switching to landscape and back to portrait, the page is zoomed in.

(I hope you can understand my confusion, as that's not what the STR in comment 0 say. By the way, I dislike Bugzilla's "clone bug" feature for this reason: you often end up carrying over information / steps that are no longer relevant from a previous bug.)

I can reproduce this, on a tablet only.

Reviewing the discussion in bug 1509552, I was expecting bug 1510029 to fix this, but it didn't. This will therefore need some further investigation.

Summary: Pages don't fit the screen after rotating from portrait to landscape → Page is zoomed in after rotating to landscape and back

I should have edited it.

Am I right about the map?

Should the blue ever be outside the red?

Maybe fixing that will help.

(In reply to csheany from comment #10)

Am I right about the map?

Should the blue ever be outside the red?

I want to say "no", as of bug 1423013. However, there are outstanding follow-up pieces to that work, such as bug 1520077, bug 1519013, and bug 1516377. Once those pieces are in place, I should be able to say more confidently that no, the layout viewport (red) should never be smaller than the visual viewport (blue).

Maybe fixing that will help.

Indeed, it might. (Which suggests that one of the three mentioned bugs might fix this as well.)

I think the page viewports were the same way even before Bug 1423013.

(In reply to csheany from comment #12)

I think the page viewports were the same way even before Bug 1423013.

Right; before bug 1423013, that was expected.

Bug 1423013 and its followups are intended to increase the size of the layout viewport, so it's always at least as large as the visual viewport.

Also, the non-scrollable element in landscape was present as well.

I think the GitHub desktop site isn't mobile friendly which makes it such a good test case.

Summary: Page is zoomed in after rotating to landscape and back → Pages don't fit the screen after rotating from landscape to portrait

The layout viewport now seems too large.

Swiping down causes it to toggle.

Is this an issue with map itself?

Hiro, would you mind taking a look?

Do you know what's going on with Comment 16?

Flags: needinfo?(botond)

I don't.

I do agree that it's weird and should be investigated.

Flags: needinfo?(botond)

Hi!
Tested this on Nexus 9 (Android 7.1.1, Tablet) on Release version 65.0.1, latest Beta build 66.0b7, latest Nightly build 67.0a1 and I was able to reproduce following the steps provided in Description.
I will mark this bug as NEW.

Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → ARM
See Also: → 1510029

(In reply to Botond Ballo [:botond] from comment #19)

I don't.

I do agree that it's weird and should be investigated.

I expect the weirdness discussed in comments 16 and 17 was fixed by bug 1525948.

Yes I believe it was.

Good thing I filed it :)

Attached video android 9 pixel3.mp4

I got the issue on Firefox 68(build number is 20190323094805.

Spec
Pixel 3
Android 9

Assignee: nobody → bwerth
Blocks: 1538681

My patches superficially resolve the problem, but cause failures in tests that would be fixed by Bug 1519013 being resolved. Since that bug is actively being worked on, and will likely change the same code that my patches change, I'm making this bug blocked by Bug 1519013. It may end up being a duplicate.

Depends on: 1519013

Are the tests still failing?

Flags: needinfo?(bwerth)
Priority: P3 → P1

It looks like this was not fixed by bug 1519013.

Attachment #9053446 - Attachment description: Bug 1523844 Part 3: Add a test of meta viewport width device-width with tall content. → Bug 1523844 Part 2: Add a test of meta viewport width device-width with tall content.
Attachment #9053444 - Attachment is obsolete: true
Attachment #9053355 - Attachment is obsolete: true
Attachment #9054953 - Attachment description: Bug 1523844 Part 1: Make MobileViewportManager use intrinsic scale even when restore resolution is set. → Bug 1523844 Part 1: Make MobileViewportManager clear out restore resolution after completing a reflow.

Hi!

I can reproduce this on the latest version of Nightly 68.0a1 (2019-04-07) with Nexus 9 (Android 7.1.1) on ebay.com and cnn.com following the steps:

  1. Go to ebay.com
  2. Switch to landscape mode;
  3. Switch to portrait mode;
  4. Observe the behavior.

The page remains zoomed in, as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1536755#c1

I can't reproduce this anymore in Responsive Design Mode. I'll attempt to test on a GeckoView build soon.

I'm not confident in this mozregression, but here's a pushlog that may contain a fix -- if this is indeed fixed:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50d64901b71fe8e6ed7d9730467bf4b4d6060f01&tochange=aa4c97d22712b946f70a4b88f9ea927ce969d611

I can still reproduce this in today's nightly.

I'm happy to take over investigation if this no longer reproduces in RDM.

However, why is this a P1? It seems like the symptoms here are fairly mild.

(In reply to Botond Ballo [:botond] from comment #38)

However, why is this a P1? It seems like the symptoms here are fairly mild.

I guess the reason it's a P1 is that it's blocking bug 1538681, which has more severe symptoms.

However, the reason for this dependency is not entirely clear to me.

Bug 1545150 isn't great either.

It would be nice to kill two birds with one stone.

No animals were hurt in the patching of this bug. :)

Bug 1538681 was fixed without this needing to be fixed, so I'm removing the dependency and restoring this to a P3.

No longer blocks: 1538681
Priority: P1 → P3

Should this block Bug 1524170

(In reply to csheany from comment #43)

Should this block Bug 1524170

I don't think they're related. Bug 1524170 seems to be an issue with scroll anchoring. This is an issue with choosing a zoom level after a rotation.

Attached file test.html (obsolete) —

This seems to also be affecting namu.wiki, which has this meta-viewport tag:

<meta name="viewport" content="user-scalable=no, initial-scale=1.0, maximum-scale=5.0, minimum-scale=1.0, width=device-width">

Naively, it seems that Firefox doesn't "zoom back out" because of the max-scale, while Chrome does. I've attached a minimal test-case just in case it helps.

(In reply to Thomas Wisniewski [:twisniewski] from comment #46)

This seems to also be affecting namu.wiki, which has this meta-viewport tag:

<meta name="viewport" content="user-scalable=no, initial-scale=1.0, maximum-scale=5.0, minimum-scale=1.0, width=device-width">

I think this is a different problem that can and should be fixed in this bug. This testcase demonstrates that MobileViewportManager::UpdateResolution is choosing the wrong resolution when shrinking the viewport horizontally. I'll add another part to the patch to address this issue.

Attachment #9054953 - Attachment description: Bug 1523844 Part 1: Make MobileViewportManager clear out restore resolution after completing a reflow. → Bug 1523844 Part 2: Make MobileViewportManager clear out restore resolution after completing a reflow.
Attachment #9053446 - Attachment description: Bug 1523844 Part 2: Add a test of meta viewport width device-width with tall content. → Bug 1523844 Part 4: Add a test of meta viewport width device-width with tall content.
Flags: needinfo?(bwerth)
Whiteboard: [rdm-mvp]
Status: NEW → ASSIGNED
Priority: P3 → P1
Attachment #9053446 - Attachment description: Bug 1523844 Part 4: Add a test of meta viewport width device-width with tall content. → Bug 1523844 Part 3: Add a test of meta viewport width device-width with tall content.
Attachment #9065581 - Attachment is obsolete: true

(In reply to Thomas Wisniewski [:twisniewski] from comment #46)

Created attachment 9064509 [details]
This seems to also be affecting namu.wiki, which has this meta-viewport tag:
<meta name="viewport" content="user-scalable=no, initial-scale=1.0, maximum-scale=5.0, minimum-scale=1.0, width=device-width">

This is occurring because the logic at https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/layout/base/MobileViewportManager.cpp#309 is using the current content size to determine the min zoom and max zoom clamping points. The current content size is the only size it has access to until the reflow occurs. After reflow, we then call MobileViewportManager::UpdateResolution again with UpdateType::ContentSize, but then it's too late to do scaling of the zoom based on the change in the viewport size. Very tricky.

Attachment 9065579 [details] attempts to address this but is making the assumption that the viewport size and the content size are the same, which is not a safe assumption. I'll need to figure out a better solution.

The previous math was attempting to adjust the display scale ratio to
prevent the new size from landing outside the min/max zoom bounds. This
introduced 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 attempta a divide-by-zero if
aNewViewport.width is zero. The updated code will instead return a zoom
of zero in such a case.

Depends on D32908

Attachment #9054953 - Attachment description: Bug 1523844 Part 2: Make MobileViewportManager clear out restore resolution after completing a reflow. → Bug 1523844 Part 3: Make MobileViewportManager clear out restore resolution after completing a reflow.
Attachment #9053446 - Attachment description: Bug 1523844 Part 3: Add a test of meta viewport width device-width with tall content. → Bug 1523844 Part 4: Add tests of meta viewport width device-width with and without fixed zoom.

This bug is highlighting the usability issue for our resolution-restoring code. The code path is designed to bring a page back at the resolution where it was last viewed, which makes sense. But we also apply it during device orientation change, which is less clear. We also apply it when the tab is closed in one orientation and restored when in another orientation, which is even less clear. These proposed changes increase the cases where we ignore the restored resolution.

Here's a list of actions with current and changed behavior (with these patches)

Action #1: Rotate from portrait to landscape to portrait (the original steps to reproduce).
CURRENT: Keep landscape resolution.
CHANGED: Go back to the portrait resolution.

Action #2: Rotate from portrait to landscape, then adjust the zoom manually, then rotate back to portrait.
CURRENT: Keep the landscape resolution.
CHANGED: No change.

Action #3: Rotate from portrait to landscape, close the tab, then rotate back to portrait and restore the tab.
CURRENT: Keep the landscape resolution.
CHANGED: Go back to the portrait resolution.

To support Action #3, another part of this patch will have to be added to adjust the expectations for the mobile/android/tests/browser/chrome/test_session_scroll_position.html test.

Attachment #9065579 - Attachment is obsolete: true
Attachment #9064509 - Attachment is obsolete: true

The issue in comment 46 has been split into its own Bug 1555511.

Attachment #9068203 - Attachment description: Bug 1523844 Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation. → Bug 1555511 Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation.
Attachment #9068204 - Attachment description: Bug 1523844 Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth. → Bug 1555511 Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth.

Comment on attachment 9068203 [details]
Bug 1555511 Part 1: Make MVM::UpdateResolution clamp viewport zoom for the new display size later in the calculation.

Revision D32908 was moved to bug 1555511. Setting attachment 9068203 [details] to obsolete.

Attachment #9068203 - Attachment is obsolete: true

Comment on attachment 9068204 [details]
Bug 1555511 Part 2: Remove a float division in MVM::ScaleZoomWithDisplayWidth.

Revision D32909 was moved to bug 1555511. Setting attachment 9068204 [details] to obsolete.

Attachment #9068204 - Attachment is obsolete: true
Attachment #9054953 - Attachment description: Bug 1523844 Part 3: Make MobileViewportManager clear out restore resolution after completing a reflow. → Bug 1523844 Part 1: Make MobileViewportManager clear out restore resolution after completing a reflow.
Attachment #9053446 - Attachment description: Bug 1523844 Part 4: Add tests of meta viewport width device-width with and without fixed zoom. → Bug 1523844 Part 2: Add a test of meta viewport width device-width with tall content.

(In reply to Brad Werth [:bradwerth] from comment #56)

This bug is highlighting the usability issue for our resolution-restoring code. The code path is designed to bring a page back at the resolution where it was last viewed, which makes sense. But we also apply it during device orientation change, which is less clear. We also apply it when the tab is closed in one orientation and restored when in another orientation, which is even less clear. These proposed changes increase the cases where we ignore the restored resolution.

Here's a list of actions with current and changed behavior (with these patches)

Action #1: Rotate from portrait to landscape to portrait (the original steps to reproduce).
CURRENT: Keep landscape resolution.
CHANGED: Go back to the portrait resolution.

Action #2: Rotate from portrait to landscape, then adjust the zoom manually, then rotate back to portrait.
CURRENT: Keep the landscape resolution.
CHANGED: No change.

Action #3: Rotate from portrait to landscape, close the tab, then rotate back to portrait and restore the tab.
CURRENT: Keep the landscape resolution.
CHANGED: Go back to the portrait resolution.

Botond, fixing this bug will meaningfully change our resolution restore behavior. Do you feel that the above proposed changes are improvements. If not, what behavior should we adopt?

Flags: needinfo?(botond)

If I'm understanding correctly, of the 3 scenarios you listed:

  • #1 is what we're looking to fix here
  • #2 is not a change (maintains the current behaviour)
  • #3 is a side effect of the change needed to fix #1

If so, that sounds reasonable to me. In particular, #3 seems like enough of an edge case (requiring as it does a very specific sequence of operations) that I think either behaviour there would be acceptable.

Flags: needinfo?(botond)
Attachment #9068582 - Attachment description: Bug 1523844 Part 3: Change test expectations that we ignore saved zoom levels when restoring tabs. → Bug 1523844 Part 4: Change test expectations that we ignore saved zoom levels when restoring tabs.
Attachment #9054953 - Attachment description: Bug 1523844 Part 1: Make MobileViewportManager clear out restore resolution after completing a reflow. → Bug 1523844 Part 1: Make MobileViewportManager shrink to fit content even when restoring resolution.
Attachment #9069112 - Attachment description: Bug 1523844 Part 3: Improve the test error messages in the scroll test helper. → Bug 1523844 Part 2: Improve the test error messages in the scroll test helper.
Attachment #9068582 - Attachment description: Bug 1523844 Part 4: Change test expectations that we ignore saved zoom levels when restoring tabs. → Bug 1523844 Part 3: Change test expectations that we ignore saved zoom levels when restoring tabs.

Brad, Does your patch fixes the issue in https://github.com/webcompat/web-bugs/issues/31792 ?
Thanks.

Flags: needinfo?(bwerth)

As per this comment, it was in fact bug 1555511 which fixed the mentioned issue.

Flags: needinfo?(bwerth)
Attachment #9054953 - Attachment description: Bug 1523844 Part 1: Make MobileViewportManager shrink to fit content even when restoring resolution. → Bug 1523844 Part 1: Make MobileViewportManager ignore restored resolution if display size has changed.
Attachment #9053446 - Attachment is obsolete: true

Finally starting to get some better insight into this. Notes to self:

The problem with the patch's proposed approach of clearing mRestoreResolution at the end of RefreshViewportSize -- though conceptually clean -- is that is confuses the tests' ability to wait on a resolution update event. This is the problematic calling pattern:

  1. When tab is restored, some dom-meta-added events are also fired, which calls MVM::RefreshViewportSize. Since the viewport size hasn't been stored yet, that reflows. So far, so good. The patch adds a step at the end that clears out mRestoreResolution.
  2. Later, ScrollFrameHelper::ReflowFinished -> MVM::ShrinkToDisplaySizeIfNeeded -> MVM::UpdateResolution, and now that mRestoreResolution is cleared, that sets the resolution to the intrinsic size. This sets mRestoreResolution again! Probably bad.
  3. Later, a before-first-paint event is fired, which calls MVM::SetInitialViewport -> MVM::RefreshViewportSize. Since this is a first paint, this attempts to restore the resolution. But the restored resolution is the last-set and still-current resolution, so PresShell::SetResolutionAndScaleTo ignores the request and doesn't fire the "mozvisualscroll" event that the tests are waiting for.

So, possible fixes:
a) Move the clearing of mRestoreResolution to happen instead in ScrollFrameHelper::ReflowFinished since that is conceptually still part of the same reflow triggered by MVM::RefreshViewportSize. Is that always called? Dunno. This is probably what I'll do.
b) Add some new kind of reflow observer that happens after all of the other things, and clear mRestoreResolution there.
c) Force the resolution to be set and events to be fired when restoring resolution. Sounds wasteful.

I'll pursue option a) and see if it solves the issue.

(In reply to Brad Werth [:bradwerth] from comment #86)

a) Move the clearing of mRestoreResolution to happen instead in ScrollFrameHelper::ReflowFinished since that is conceptually still part of the same reflow triggered by MVM::RefreshViewportSize. Is that always called? Dunno.

Yes, I think ReflowFinished() will always be called when the reflow completes, whether it completes synchronously or is interrupted and completes in a later refresh driver tick.

In terms of the general plan -- I don't recall what the motivation is for clearing mRestoreResolution, but assuming that's something we want to do after a reflow, then I agree that doing it in ReflowFinished() makes more sense than right after the Reflow() call.

setResolutionAndScaleTo has a side effect of setting mRestoreResolution if
mPainted is false. This is designed to fix cases where resolution is set
from outside the MobileViewportManager before first paint. Unfortunately,
when MVM sets its own resolution, this side effect causes the resolution
to become somewhat permanent and affect future resizing of the viewport.
By clearing mRestoreResolution after MVM sets resolution, we ensure that
the MVM will only respect restore resolutions coming from "outside".

Depends on D40673

Attachment #9054953 - Attachment is obsolete: true
Attachment #9069112 - Attachment is obsolete: true
Attachment #9068582 - Attachment is obsolete: true
Blocks: 1561227
Attachment #9083080 - Attachment is obsolete: true
Attachment #9083083 - Attachment description: Bug 1523844 Part 2: Undo the side effect after calling setResolutionAndScaleTo. → Bug 1523844 Part 1: Undo the side effect after calling setResolutionAndScaleTo.
Attachment #9084435 - Attachment description: Bug 1523844 - Add a test case to TestMobileViewportManager. r=bradwerth → Bug 1523844 Part 3: Add a test case to TestMobileViewportManager. r=bradwerth
Attachment #9083083 - Attachment is obsolete: true
Attachment #9083837 - Attachment description: Bug 1523844 Part 2: Add a test that meta viewport page resizes correctly after reload. → Bug 1523844 Part 4: Add a test that meta viewport page resizes correctly after reload.
Attachment #9084435 - Attachment description: Bug 1523844 Part 3: Add a test case to TestMobileViewportManager. r=bradwerth → Bug 1523844 Part 5: Add a test case to TestMobileViewportManager. r=bradwerth
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a41fe6ffee8a
Part 1: Rename ResolutionChangeOrigin::MainThreadRestore in anticipation of adding a related enum. r=botond
https://hg.mozilla.org/integration/autoland/rev/c28d03b42e7e
Part 2: Make MVMContext::SetResolutionAndScaleTo accept a ResolutionChangeOrigin. r=botond
https://hg.mozilla.org/integration/autoland/rev/564953d25158
Part 3: Make the MVM set resolution only as an adjustment, not a restore resolution. r=botond
https://hg.mozilla.org/integration/autoland/rev/78fda23b5ee4
Part 4: Add a test that meta viewport page resizes correctly after reload. r=botond
https://hg.mozilla.org/integration/autoland/rev/cc691c0e8775
Part 5: Add a test case to TestMobileViewportManager. r=bradwerth

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Regressions: 1598145

Thank you Brad!

First paint timing is everything :)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: