Pages don't fit the screen after rotating from portrait to landscape
Categories
(Core :: Panning and Zooming, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox63 | --- | wontfix |
firefox64 | --- | wontfix |
firefox65 | --- | fixed |
People
(Reporter: csheany, Assigned: botond)
References
Details
(Keywords: regression)
Attachments
(5 files, 3 obsolete files)
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.
Assignee | ||
Comment 2•6 years ago
|
||
Confirmed. This is a regression compared to Firefox 57.
Assignee | ||
Comment 3•6 years ago
|
||
Hmm... while I could reproduce the issue in a local build I happened to be running, I can't seem to reproduce it in a Nightly build. Which Nightly build were you using to test this?
Thank you for looking into it. I am using Nightly 2018-11-21
Assignee | ||
Comment 5•6 years ago
|
||
Since I could repro this in a local build, I just debugged it. The issue seems to be related to MobileViewportManager calculating a zoom that's too small in landscape mode. I have no explanation as to why it doesn't reproduce for me on a Nightly build.
It seems to be specific to desktop sites as opposed to mobile or tablet oriented sites. In the case of GitHub the page gets stuck instead of resizing more quickly.
Comment 7•6 years ago
|
||
Saw this as well on Nightly 2018-11-21. There were some recent viewport-related changes, so my first guess would be those.
Comment 8•6 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #7) > Saw this as well on Nightly 2018-11-21. ... but when attempting to use mozregression, I can no longer reproduce this. Grrr...
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to csheany from comment #6) > It seems to be specific to desktop sites as opposed to mobile or tablet > oriented sites. Yes, it seems to affect sites whose content has a fixed width regardless of orientation (which is true of typical desktop sites), and not sites whose content will resize to match the available screen width (typical mobile sites).
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #5) > I have no explanation as to why it doesn't reproduce for me on a Nightly > build. (In reply to Jan Henning [:JanH] from comment #8) > ... but when attempting to use mozregression, I can no longer reproduce > this. Grrr... I figured out a reliable way to reproduce this: 1. Open https://github.com/mozilla-mobile. 2. Request desktop site. (3. Rotate to landscape ==> bug does not occur on a fresh install. 4. Rotate back to portrait.) 5. Open tab switcher. 6. Close the tab, and then "Undo" to reopen it. 7. Now rotate to landscape ==> the bug is triggered With this STR, I see the bug going at least as far back as Firefox 57, suggesting that it's not a recent regression.
Assignee | ||
Comment 11•6 years ago
|
||
It seems to come down to this: * When you're loading the page in desktop mode for the first time (such that just before it was in mobile mode), the bug doesn't occur. * When you're reloading the page such that it's already in desktop mode, the bug occurs.
Assignee | ||
Comment 12•6 years ago
|
||
In turn that's because the first scenario hits the call to setDesktopModeViewport() here [1], while the second doesn't. [1] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/mobile/android/chrome/content/browser.js#3871
Assignee | ||
Comment 13•6 years ago
|
||
I think there are two bugs here: 1) A Fennec frontend bug, where, after doing "Request desktop site", the desktop UA string is remembered for future loads of the page (so we continue to be served the desktop site), but the setDesktopModeViewport() call only happens on that first load. 2) A platform bug where MobileViewportManager will sometimes allow a zoom level to be calculated that makes the page content smaller than the available space. Fixing the first bug would largely mask the second, but it's conceivable that a particularly badly-behaved site might have desktop-like behaviour (specifically, a fixed content width independent of (and larger than) the ICB width) even outside of desktop mode, and could run into the second issue, so it's best to fix the second issue too. I'm going to fix the second issue in this bug, and defer to people more familiar with Fennec frontend code to fix the first issue.
Assignee | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #13) > 1) A Fennec frontend bug, where, after doing "Request desktop > site", the desktop UA string is remembered for future loads > of the page (so we continue to be served the desktop site), > but the setDesktopModeViewport() call only happens on > that first load. Does "load" within this context mean any load, i.e. including a load triggered by simply clicking on a link?
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #14) > (In reply to Botond Ballo [:botond] from comment #13) > > 1) A Fennec frontend bug, where, after doing "Request desktop > > site", the desktop UA string is remembered for future loads > > of the page (so we continue to be served the desktop site), > > but the setDesktopModeViewport() call only happens on > > that first load. > > Does "load" within this context mean any load, i.e. including a load > triggered by simply clicking on a link? I'm not exactly sure. The relevant code is here, where Tab.desktopMode is set [1], which then causes us to set a desktop UA string [2]. You probably know better than me under what circumstances that Tab.desktopMode flag is persisted / remembered. But certainly, there are cases where it is remember where setDesktopModeViewport() is not called (since it's only called in this one place [3]). The desired behaviour would be to call setDesktopModeViewport() any time we're using a desktop UA string. [1] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/mobile/android/chrome/content/browser.js#3875 [2] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/mobile/android/chrome/content/browser.js#3437 [3] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/mobile/android/chrome/content/browser.js#3871
Comment 16•6 years ago
|
||
I was a bit confused initially, but the desktop mode flag seems part of the outer window and as such should persist as long as the tab exists. In that case the (or at least one) bug lies with the session restore code, which restores Fennec's desktopMode flag on the tab, but doesn't set the desktop viewport if necessary.
Assignee | ||
Comment 17•6 years ago
|
||
I have a fix for this (#2 from comment 13) locally, just need to clean it up a bit.
Assignee | ||
Comment 18•6 years ago
|
||
* Introduce helpers for converting between zoom and resolution * Perform calculations in terms of zoom rather than resolution
Assignee | ||
Comment 19•6 years ago
|
||
* Call SetResolutionAndScaleTo() in just one place * Call UpdateVisualViewportSize() in UpdateResolution() rather than requiring the caller to do it (and on non-first paints, only call it if the resolution has actually changed) Depends on D13312
Assignee | ||
Comment 20•6 years ago
|
||
Adds an |UpdateType| parameter to UpdateResolution() to better separate the pre-reflow and post-reflow logic. Depends on D13313
Assignee | ||
Comment 21•6 years ago
|
||
Depends on D13314
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to csheany from comment #0) > Zooming in even slightly fixes the > issue but then in portrait the page is larger than screen. This still happens with my patches, but should be fixed by bug 1510029.
Reporter | ||
Comment 23•6 years ago
|
||
To clarify my statement, it snaps as it should have to begin with and won't allow zooming out to the previous state. There is a slight delay in other instances but this was consistent. Also, tablets default to the desktop site so the other bug might not be as much of an issue. Can you elaborate because this only requires an initial load?
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to csheany from comment #23) > Also, tablets default to the desktop site so the other bug might not be as > much of an issue. > > Can you elaborate because this only requires an initial load? For tablets, since desktop mode is the default, we are likely always in the second case from comment 11, so the bug always occurs.
Reporter | ||
Comment 25•6 years ago
|
||
So... If Comment 11 (1st state) then Comment 13 (A) If Comment 11 (2nd state) then Commment 13 (B) Correct?
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to csheany from comment #25) > So... > > If Comment 11 (1st state) then Comment 13 (A) > > If Comment 11 (2nd state) then Commment 13 (B) > > Correct? Not quite, sorry, I should have been more clear. In comment 11 (1st state), there is no bug. In comment 11 (2nd state), we run into both issues mentioned in comment 13. The patches in this bug will make it so that after changing the orientation to landscape, the page will be resized to fill the entire width of the screen. However, after changing back to portrait, the page will be zoomed in. Bug 1510029 will fix that, so the page fits the screen exactly even after changing back to portrait.
Reporter | ||
Comment 27•6 years ago
|
||
Why is portrait still affected if the page isn't reloaded?
Assignee | ||
Comment 28•6 years ago
|
||
(In reply to csheany from comment #27) > Why is portrait still affected if the page isn't reloaded? It's not: if I run Nightly on a phone, such that initially the mobile page is loaded, then perform "Request desktop site", I can change the orientation back and forth and the page will fit the width of the screen each time.
Reporter | ||
Comment 29•6 years ago
|
||
With just this patch? I thought portrait is still zoomed in?
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to csheany from comment #29) > With just this patch? > > I thought portrait is still zoomed in? That's the case on a tablet, or on a phone after loading the page in desktop mode a second time. If you're asking why that is, it's because during the switch to landscape, the zoom has to be increased to make the page content fill the screen. When going back to portrait mode, the zoom level is unchanged, so the result is a zoomed-in page.
Reporter | ||
Comment 31•6 years ago
|
||
Thank you for the explanation. I just wanted to make sure the patch was suitable. I will wait until it lands and report my observations.
Comment 32•6 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3cdb29bf81f0 MobileViewportManager::UpdateResolution cleanup, part 1. r=kats https://hg.mozilla.org/integration/autoland/rev/8085138baf57 MobileViewportManager::UpdateResolution cleanup, part 2. r=kats https://hg.mozilla.org/integration/autoland/rev/038125c617dc MobileViewportManager::UpdateResolution cleanup, part 3. r=kats https://hg.mozilla.org/integration/autoland/rev/d8064028f0e6 Ensure MobileViewportManager doesn't choose a zoom level that makes the content smaller than the visual viewport. r=kats
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3cdb29bf81f0 https://hg.mozilla.org/mozilla-central/rev/8085138baf57 https://hg.mozilla.org/mozilla-central/rev/038125c617dc https://hg.mozilla.org/mozilla-central/rev/d8064028f0e6
Assignee | ||
Comment 34•6 years ago
|
||
Will not be uplifting this to 64, partly because we're late in the cycle and partly because this has a code dependency on bug 1423709 which is not on 64.
Reporter | ||
Comment 35•6 years ago
|
||
Thank you. Nightly 2018-11-30 is much improved. Now for the other half.
Updated•6 years ago
|
Reporter | ||
Comment 36•6 years ago
|
||
I believe it was Bug 1423709 that made this more noticeable.
Comment 37•6 years ago
|
||
The desktopMode property on Fennec's tab object is already being correctly being
preserved when (re)creating a tab [1], but we don't propagate its state to the
content window's desktop viewport mode setting.
[1] When restoring into a fresh tab, the session restore code passes the stored
desktopMode flag to addTab, whereas zombifying an existing tab destroys its
<browser>, but leaves the tab object's properties intact, so we merely need to
re-set the desktop viewport mode on the new <browser>'s content window.
Comment 38•6 years ago
|
||
Comment on attachment 9039394 [details]
Bug 1509552 - Correctly set desktop mode viewport when creating a tab. r?esawin
Revision D17774 was moved to bug 1510029. Setting attachment 9039394 [details] to obsolete.
Comment 39•4 years ago
|
||
I don't think it is correct. This comes back from bug 1509552, but I
believe it is better fixed by the change in bug 1536755.
In particular, treating the intrinsic scale as the minimum effective
zoom seems wrong. The intrinsic scale grows on full zoom increases (but
that won't change the resolution), however then decreasing the zoom will
increase the resolution, which looks wrong.
In order to work around that, bug 1561227 added a bunch of code to
restore the resolution after a full zoom change, which we can get rid
of after this.
The test for bug 1536755 still passes after this, and I manually-tested
bug 1509552 on GVE.
Comment 40•4 years ago
|
||
This is no longer needed now that MVM doesn't restore the zoom.
Depends on D72550
Comment 41•4 years ago
|
||
Comment on attachment 9143426 [details]
Bug 1509552 - Avoid clamping zoom to intrinsic scale. r=botond
Revision D72550 was moved to bug 1578008. Setting attachment 9143426 [details] to obsolete.
Comment 42•4 years ago
|
||
Comment on attachment 9143427 [details]
Bug 1509552 - Remove various hacks to restore resolution only after a full zoom change. r=botond,bradwerth
Revision D72551 was moved to bug 1578008. Setting attachment 9143427 [details] to obsolete.
Description
•