Closed Bug 1202713 Opened 6 years ago Closed 6 years ago

Desktop pages not rendering at correct width on android

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

Unspecified
Android
defect
Not set
normal

Tracking

(firefox42 unaffected, firefox43 affected, firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox42 --- unaffected
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: jnicol, Assigned: kats)

References

Details

Attachments

(6 files)

The desktop youtube site is being rendered strangely, especially in landscape. I've attached a screenshot. The main content is much narrower than it should be, leaving a large section of background to the right. All the content is still rendered, but rearranged for a small screen instead of taking advantage of the large screen. And the search bar has been cut in half with the right hand side not being rendered at all.

I have bisected problem and found that it was introduced by https://hg.mozilla.org/mozilla-central/rev/636fa61990f3
Screenshot of it rendered correctly, for comparison.
What device are you seeing this on? Are you loading the page in landscape, or loading in portrait and then rotating? And are you loading the mobile site first, and then going to the desktop version? Or using the "request desktop site" feature? Or something else?
I couldn't reproduce this on my Nexus 4 on my initial attempts, but I suspect the patch on bug 1202290 will fix this. If you have a moment, please apply that patch and let me know if it still happens. Thanks!
I see this on my nexus 5, 6 and galaxy s6. it's possible the nexus 4 isn't a high enough resolution.
Using "request desktop site". doesn't matter about if you rotate or not, in fact it sort of happens in portrait too but is much worse in landscape.

will test the patch now.
The patch from bug 1202290 does indeed fix this. Should I close this as a dup?
Yup, thanks for checking!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1202290
Hmm, I must have been mistaken when I said that patch fixed this. I still get it on latest nightly, including on my Nexus 4.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
This happens at www.youtube.com with "request desktop site" checked, but not at www.youtube.com/?app=desktop (the same page) without it checked.
A similar issue was introduced with the patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1191539#c36 and fixed with the backout at https://bugzilla.mozilla.org/show_bug.cgi?id=1205863#c12. Hopefully in the next nightly this will be gone (or if you're building locally, the latest central build should have it fixed).
Depends on: 1205863
Unfortunately latest central still has this problem.
I'm still not able to reproduce on a nexus 4. I tried using the .co.uk version of YouTube but still couldn't. I'll try a nexus 5 or 6 next time I'm in the office, or if you are doing a local build please turn on the MVM_LOG dump at the top of MobileViewportManagr.cpp and provide the logcat from reproducing the problem.
Logcat with MVM enabled taken on my nexus 5. (1920x1080)
I opened the browser, selected "request desktop site", loaded youtube.com, dismissed the soft keyboard, then rotated the phone into landscape, hence all the viewport changes.

All of these stages render slightly incorrectly but it's by far most pronounced in landscape.
That's super bizarre. The relevant MVM instance in your log is 0x876119b0 and if you grep the file for that you see that during load it sets a CSS viewport size of (w=889.259277, h=1282.166748). You can see where the keyboard comes up and it resizes it to (w=889.259277, h=631.555603). And then the keyboard gets dismissed and it goes back to (w=889.259277, h=1282.166748). Finally on rotation it changes the CSS viewport to (w=535.340027, h=256.744690). Frankly none of these values make sense given the display dimensions also in the logcat.

Let's just look at the first viewport computation for simplicity:

09-30 17:54:59.429: I/Gecko(22969): MVM: 0x876119b0: Computing CSS viewport using 1080,1557
09-30 17:54:59.429: I/Gecko(22969): MVM: 0x876119b0: Computed CSS viewport (w=889.259277, h=1282.166748)

This output comes from the code at [1]. This code is trivial; all it does is get the nsViewportInfo object from the document and report the size. The code that returns the nsViewportInfo object, assuming desktop mode is enabled, is at [2]. The viewport width in particular should just be the value of the DesktopViewportWidth pref divided by the full zoom. And those should be 980 and 1.0 respectively, unless you have fiddled with that pref or somehow managed to insert a fullzoom into fennec (AFAIK there's no way to do that without changing code). So the returned viewport width should just be 980, and yet the output above shows that it is 889.259277.

I don't know if you have more time to investigate this but inspecting the flow there with a debugger or printfs should tell us where things are going off the rails.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/MobileViewportManager.cpp?rev=501ea1317fb5&mark=256-269#256
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=df5ce21dade9&mark=7848-7858#7848
Oh wait, the nsViewportInfo constructor is also dividing the display size by the default zoom, so that's where the 889 is coming from. The logic here seems wrong, let me look a bit more.
Ok, I think I know what's going wrong here. Regression from bug 1180267. I'm working on a fix.
Attached patch PatchSplinter Review
This should do the job. I did a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f7e17051ad if you want to use that to test, Jamie.
Assignee: nobody → bugmail.mozilla
Attachment #8668031 - Flags: review?(snorp)
Attachment #8668031 - Flags: feedback?(jnicol)
Whoops, that try push didn't even have the patch. Here's a better one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed51896380a3
This pretty much fixes it, however there is a minor regression still:

When changing the orientation from portrait to landscape it does not seem to automatically adjust the size. On stable firefox it zooms in so that the same width of content with the same layout fills the whole screen. A tiny scroll event is enough to make it gradually (but quickly) zoom to the right size, but I think it should have been that size all along.

Similar thing when returning to portrait - it does not automatically zoom out so the content fills the screen instead of being too wide.
Attached file logcat with patch
Logcat with the patch applied and MVM_LOG enabled.

Same procedure as last time: request desktop site, load youtube, dismiss keyboard, rotate to landscape, think I rotated back to portrait again to end.
Hm, that seems like a different bug that affects not just desktop mode but any page with a fixed css viewport width. I think the MVM is actually updating the resolution just fine but it is getting clobbered/ignored by browser.js/Java. I'll file a new bug for that.
Attachment #8668031 - Flags: review?(snorp) → review+
Comment on attachment 8668031 [details] [diff] [review]
Patch

Review of attachment 8668031 [details] [diff] [review]:
-----------------------------------------------------------------

Taking comment 18 as a f+, considering I filed a follow-up for the remaining issue.
Attachment #8668031 - Flags: feedback?(jnicol) → feedback+
https://hg.mozilla.org/mozilla-central/rev/f89a4c296fb1
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.