Closed
Bug 1233762
Opened 9 years ago
Closed 8 years ago
Images are zoomed to fill entire display; cannot zoom out to view entire image
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Tracking
(firefox42 unaffected, firefox43 wontfix, firefox44 wontfix, firefox45 verified, firefox46 verified, fennec45+)
RESOLVED
FIXED
Firefox 46
People
(Reporter: arv, Assigned: kats)
References
Details
(Keywords: regression)
Attachments
(2 files)
1.49 KB,
patch
|
esawin
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
247.84 KB,
image/png
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 Build ID: 20151029151421 Steps to reproduce: Load an image file whose aspect ratio does not exactly match the display. Examples (for a mobile device in portrait orientation): http://images4.wikia.nocookie.net/__cb20130222100631/mrmen/images/d/d2/Mrtallimage.png http://www.webmastergrade.com/wp-content/uploads/2009/09/Beautiful-Morning-WideScreen-Wallpaper.jpg Actual results: The image is zoomed so the narrowest dimension of the image fills the entire display, causing the other dimension to overflow the display and require scrolling to see. Zooming out to view the entire image on the screen is disabled. Before FF 43, tall images were treated this way, but wide images were not. Since FF 43, both tall and wide images are zoomed like this, which makes FF unusable as an image viewer. Zooming in still works, but FF does not allow zooming out beyond the default zoom level at which you cannot see the entire image. Expected results: Zooming out to see the entire image should always be possible. I would prefer the image to be zoomed to fit on a single screen without requiring scrolling by default, but this is subjective.
Summary: Images are zoomed to fit entire display; cannot zoom out to view entire image → Images are zoomed to fill entire display; cannot zoom out to view entire image
Assignee | ||
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
status-firefox42:
--- → unaffected
status-firefox43:
--- → affected
Ever confirmed: true
Keywords: regression
Iona can you see where this broke?
Assignee | ||
Comment 2•9 years ago
|
||
I'm fairly confident this broke with switching Fennec over to the MobileViewportManager code (bug 1180267). I'm not sure a regression window is useful here, it's just something that we need to fix. I'll park it with me for now but won't get to it until after APZ is riding the trains on desktop.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(ioana.chiorean)
Assignee | ||
Updated•9 years ago
|
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox46:
--- → affected
Keywords: regressionwindow-wanted
Comment 3•9 years ago
|
||
Seems like Kats has it all figured already.
Updated•8 years ago
|
tracking-fennec: 43+ → ?
Assignee | ||
Comment 4•8 years ago
|
||
I spent some time thinking about this today and it's not an easy problem to solve. In order to be able to zoom out to the full image width, we need to make sure the page height is taller than the image height. i.e. the page height should be page_width * screen_height / screen_width. The page height in turn depends on the CSS viewport height. Prior to bug 1180267, the code in browser.js did a two-pass viewport size computation: the first pass would figure out what page_width was and then the second pass would set the viewport height based on that. However, that caused a lot of other issues and I would like to avoid doing that here. That means we need some other way to figure out how to size the CSS viewport. I'll think about it more and poke around in the APIs that we have to see if anything allows us to do this without too much extra computation and firing of events.
Assignee | ||
Comment 5•8 years ago
|
||
Ah, after looking at how it works on desktop the solution seems rather simple. The nsIImageDocument interface already provides an API to do shrink-to-fit, so we can just use that.
Assignee | ||
Comment 6•8 years ago
|
||
I don't recall exactly how the restoredSessionZoom stuff will come into play on image documents, but I think this seems reasonable. This works for me with APZ disabled, I'll test with APZ enabled as well.
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8705853 [details] [diff] [review] Patch Yup, that works.
Attachment #8705853 -
Flags: review?(esawin)
Comment 8•8 years ago
|
||
Comment on attachment 8705853 [details] [diff] [review] Patch Review of attachment 8705853 [details] [diff] [review]: ----------------------------------------------------------------- r=me This will shrink-to-fit tall images, too, which is a change in behavior, do we want that?
Attachment #8705853 -
Flags: review?(esawin) → review+
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #8) > r=me > > This will shrink-to-fit tall images, too, which is a change in behavior, do > we want that? Good point. I think that's not a bad change. It matches what happens on desktop and makes things more consistent between tall and wide images. And regardless you can zoom in by pinching so it shouldn't be a problem.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/387061724760
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
tracking-fennec: ? → 45+
Kats, can you uplift to Aurora please?
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8705853 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: bug 1180267 most likely [User impact if declined]: when viewing an image document in fennec (i.e. navigating directly to an image url) the image is displayed zoomed in, and if it's a "wide" image, you can no longer zoom out far enough to fit the whole width of the image on the screen. [Describe test coverage new/current, TreeHerder]: no automated tests for this; tested locally [Risks and why]: fairly low risk; small patch which has had some bake time. [String/UUID change made/needed]: none
Attachment #8705853 -
Flags: approval-mozilla-aurora?
Comment 14•8 years ago
|
||
Images are not displayed zoomed in, so: Verified as fixed using: Device: Nexus 6 (Android 6.0) Build: Firefox for Android 46.0a1 (2016-01-17)
Updated•8 years ago
|
Updated•8 years ago
|
Comment 15•8 years ago
|
||
Comment on attachment 8705853 [details] [diff] [review] Patch Fix a regression, taking it.
Attachment #8705853 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/bcf61edc0983
Comment 17•8 years ago
|
||
Verified as fixed using: Device: Nexus 6 (Android 6.0) Build: Firefox for Android 45.0a2 (2016-01-25)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•