Closed Bug 1200402 Opened 5 years ago Closed 5 years ago

Reader mode pages can get load with a desktop mode viewport

Categories

(Firefox for Android :: Toolbar, defect)

43 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox43 --- verified

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

STR:
1. Long-press on a link and add it to your reading list (most things should do as long as they have wide content)
2. Open the thing you added to your reading list, it will get opened in reader mode
3. From the menu toggle "Request desktop site" so that it is enabled
4. Reload the page

Expected:
In steps 2 and 4 the page should get loaded with a mobile viewport (i.e. not horizontally scrollable)

Actual:
In step 4 the page gets loaded with a desktop viewport and is horizontally scrollable.

This is a regression from switching Fennec over to using MobileViewportManager. In Fennec about:reader is special-cased at [1] so that the desktop-mode viewport doesn't apply to it. But after the switch to MobileViewportManager this code path doesn't get used and the equivalent path at [2] doesn't have this special check.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=2d3464d58ff4#6384
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=975b92a32949#7905
Attached patch PatchSplinter Review
Feel free to throw the review to somebody else. I decided to ignore "desktop mode" for all about: pages because then I don't have to hard-code about:reader, and also because I think it makes more sense in general.
Attachment #8655054 - Flags: review?(snorp)
Comment on attachment 8655054 [details] [diff] [review]
Patch

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

I think you need another review, but it seems fine to me. I don't know who the least-overloaded DOM peer is...but let's try Kyle?
Attachment #8655054 - Flags: review?(snorp)
Attachment #8655054 - Flags: review?(khuey)
Attachment #8655054 - Flags: review+
Comment on attachment 8655054 [details] [diff] [review]
Patch

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

lol, you think I'm the least overloaded peer?
Attachment #8655054 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/40194ca33785
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Verified as fixed on latest Nightly
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.