Pages don't fit the screen after rotating from landscape to portrait
Categories
(Core :: Panning and Zooming, defect, P1)
Tracking
()
People
(Reporter: csheany, Assigned: bradwerth)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [rdm-mvp])
Attachments
(8 files, 13 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
video/mp4
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
+++ 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:
- Open https://github.com/mozilla-mobile
- Request desktop site
- 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?
Comment 2•6 years ago
|
||
To clarify: is the issue still with the same website (https://github.com/mozilla-mobile)?
Yes.
Looking at the APZ Mini Map :) upon loading the red (layout) is inside the blue (visual)
Comment 4•6 years ago
|
||
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.
Comment 7•6 years ago
|
||
(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.
Comment 9•6 years ago
|
||
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.
Reporter | ||
Comment 10•6 years ago
|
||
I should have edited it.
Am I right about the map?
Should the blue ever be outside the red?
Maybe fixing that will help.
Comment 11•6 years ago
|
||
(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.)
Reporter | ||
Comment 12•6 years ago
|
||
I think the page viewports were the same way even before Bug 1423013.
Comment 13•6 years ago
|
||
(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.
Reporter | ||
Comment 14•6 years ago
|
||
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.
Reporter | ||
Comment 15•6 years ago
|
||
Reporter | ||
Comment 16•6 years ago
|
||
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?
Reporter | ||
Comment 17•6 years ago
|
||
Reporter | ||
Comment 18•6 years ago
|
||
Do you know what's going on with Comment 16?
Comment 19•6 years ago
|
||
I don't.
I do agree that it's weird and should be investigated.
Comment 20•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 21•6 years ago
|
||
(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.
Reporter | ||
Comment 22•6 years ago
|
||
Yes I believe it was.
Good thing I filed it :)
Comment 23•6 years ago
|
||
I got the issue on Firefox 68(build number is 20190323094805.
Spec
Pixel 3
Android 9
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
Depends on D24757
Assignee | ||
Comment 26•6 years ago
|
||
Depends on D24816
Assignee | ||
Comment 27•6 years ago
|
||
Assignee | ||
Comment 28•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Comment 30•6 years ago
|
||
It looks like this was not fixed by bug 1519013.
Assignee | ||
Comment 31•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 32•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 33•6 years ago
|
||
Comment 34•6 years ago
|
||
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:
- Go to ebay.com
- Switch to landscape mode;
- Switch to portrait mode;
- Observe the behavior.
The page remains zoomed in, as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1536755#c1
Assignee | ||
Comment 35•6 years ago
|
||
Assignee | ||
Comment 36•6 years ago
|
||
I can't reproduce this anymore in Responsive Design Mode. I'll attempt to test on a GeckoView build soon.
Assignee | ||
Comment 37•6 years ago
|
||
I'm not confident in this mozregression, but here's a pushlog that may contain a fix -- if this is indeed fixed:
Comment 38•6 years ago
|
||
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.
Comment 39•6 years ago
|
||
(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.
Reporter | ||
Comment 40•6 years ago
|
||
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. :)
Assignee | ||
Comment 41•6 years ago
|
||
Comment 42•6 years ago
|
||
Bug 1538681 was fixed without this needing to be fixed, so I'm removing the dependency and restoring this to a P3.
Reporter | ||
Comment 43•6 years ago
|
||
Should this block Bug 1524170
Comment 44•6 years ago
|
||
(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.
Assignee | ||
Comment 45•6 years ago
|
||
Comment 46•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Comment 47•6 years ago
|
||
(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.
Assignee | ||
Comment 48•6 years ago
|
||
Assignee | ||
Comment 49•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 50•6 years ago
|
||
Depends on D25656
Updated•6 years ago
|
Assignee | ||
Comment 51•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 52•5 years ago
|
||
Assignee | ||
Comment 53•5 years ago
|
||
(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.
Assignee | ||
Comment 54•5 years ago
|
||
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.
Assignee | ||
Comment 55•5 years ago
|
||
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
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 56•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 57•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 58•5 years ago
|
||
The issue in comment 46 has been split into its own Bug 1555511.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 59•5 years ago
|
||
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.
Comment 60•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 61•5 years ago
|
||
Depends on D24817
Assignee | ||
Comment 62•5 years ago
|
||
(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?
Comment 63•5 years ago
|
||
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.
Assignee | ||
Comment 64•5 years ago
|
||
Assignee | ||
Comment 65•5 years ago
|
||
Assignee | ||
Comment 66•5 years ago
|
||
Depends on D24817
Updated•5 years ago
|
Assignee | ||
Comment 67•5 years ago
|
||
Assignee | ||
Comment 68•5 years ago
|
||
Assignee | ||
Comment 69•5 years ago
|
||
Assignee | ||
Comment 70•5 years ago
|
||
Assignee | ||
Comment 71•5 years ago
|
||
Assignee | ||
Comment 72•5 years ago
|
||
Assignee | ||
Comment 73•5 years ago
|
||
Assignee | ||
Comment 74•5 years ago
|
||
Assignee | ||
Comment 75•5 years ago
|
||
Assignee | ||
Comment 76•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 77•5 years ago
|
||
Assignee | ||
Comment 78•5 years ago
|
||
Comment 79•5 years ago
|
||
Brad, Does your patch fixes the issue in https://github.com/webcompat/web-bugs/issues/31792 ?
Thanks.
Comment 80•5 years ago
|
||
As per this comment, it was in fact bug 1555511 which fixed the mentioned issue.
Updated•5 years ago
|
Assignee | ||
Comment 81•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 82•5 years ago
|
||
Assignee | ||
Comment 83•5 years ago
|
||
Assignee | ||
Comment 84•5 years ago
|
||
Assignee | ||
Comment 85•5 years ago
|
||
Assignee | ||
Comment 86•5 years ago
|
||
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:
- 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.
- 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.
- 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.
Comment 87•5 years ago
|
||
(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.
Assignee | ||
Comment 88•5 years ago
|
||
Assignee | ||
Comment 89•5 years ago
|
||
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
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 90•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 91•5 years ago
|
||
Depends on D40676
Comment 92•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 93•5 years ago
|
||
Assignee | ||
Comment 94•5 years ago
|
||
Depends on D41625
Assignee | ||
Comment 95•5 years ago
|
||
Depends on D41631
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 96•5 years ago
|
||
Comment 97•5 years ago
|
||
Comment 98•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a41fe6ffee8a
https://hg.mozilla.org/mozilla-central/rev/c28d03b42e7e
https://hg.mozilla.org/mozilla-central/rev/564953d25158
https://hg.mozilla.org/mozilla-central/rev/78fda23b5ee4
https://hg.mozilla.org/mozilla-central/rev/cc691c0e8775
Comment 99•5 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Description
•