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

Status: RESOLVED FIXED

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

Description

[csheany]

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.

Comment 1

[csheany]

Attachment: Screenshot_20181123-135157.jpg

Comment 2

[Botond Ballo [:botond]]

Confirmed. This is a regression compared to Firefox 57.

Comment 3

[Botond Ballo [:botond]]

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?

Comment 4

[csheany]

Thank you for looking into it.

I am using Nightly 2018-11-21

Comment 5

[Botond Ballo [:botond]]

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.

Comment 6

[csheany]

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

[Jan Henning [:JanH]]

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

[Jan Henning [:JanH]]

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

Comment 9

[Botond Ballo [:botond]]

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

Comment 10

[Botond Ballo [:botond]]

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

Comment 11

[Botond Ballo [:botond]]

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.

Comment 12

[Botond Ballo [:botond]]

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

Comment 13

[Botond Ballo [:botond]]

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.

Comment 14

[Jan Henning [:JanH]]

(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?

Comment 15

[Botond Ballo [:botond]]

(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

[Jan Henning [:JanH]]

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.

Comment 17

[Botond Ballo [:botond]]

I have a fix for this (#2 from comment 13) locally, just need to clean it up a bit.

Comment 18

[Botond Ballo [:botond]]

Attachment: Bug 1509552 - MobileViewportManager::UpdateResolution cleanup, part 1. r=kats

* Introduce helpers for converting between zoom and resolution
* Perform calculations in terms of zoom rather than resolution

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1509552 - MobileViewportManager::UpdateResolution cleanup, part 2. r=kats

* 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

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1509552 - MobileViewportManager::UpdateResolution cleanup, part 3. r=kats

Adds an |UpdateType| parameter to UpdateResolution() to better separate the
pre-reflow and post-reflow logic.

Depends on D13313

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1509552 - Ensure MobileViewportManager doesn't choose a zoom level that makes the content smaller than the visual viewport. r=kats

Depends on D13314

Comment 22

[Botond Ballo [:botond]]

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

Comment 23

[csheany]

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?

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[csheany]

So...

If Comment 11 (1st state) then Comment 13 (A)

If Comment 11 (2nd state) then Commment 13 (B)

Correct?

Comment 26

[Botond Ballo [:botond]]

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

Comment 27

[csheany]

Why is portrait still affected if the page isn't reloaded?

Comment 28

[Botond Ballo [:botond]]

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

Comment 29

[csheany]

With just this patch? 

I thought portrait is still zoomed in?

Comment 30

[Botond Ballo [:botond]]

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

Comment 31

[csheany]

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

[Pulsebot]

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

[Noemi Erli[:noemi_erli]]

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

Comment 34

[Botond Ballo [:botond]]

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.

Comment 35

[csheany]

Thank you.

Nightly 2018-11-30 is much improved.

Now for the other half.

Comment 36

[csheany]

I believe it was Bug 1423709 that made this more noticeable.

Comment 37

[Jan Henning [:JanH]]

Attachment: Bug 1509552 - Correctly set desktop mode viewport when creating a tab. r?esawin

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

[Phabricator Automation]

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

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1509552 - Avoid clamping zoom to intrinsic scale. r=botond

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

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1509552 - Remove various hacks to restore resolution only after a full zoom change. r=botond,bradwerth

This is no longer needed now that MVM doesn't restore the zoom.

Depends on D72550

Comment 41

[Phabricator Automation]

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

[Phabricator Automation]

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.