1233762 - Images are zoomed to fill entire display; cannot zoom out to view entire image

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[arv]

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.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Iona can you see where this broke?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Ioana Chiorean]

Seems like Kats has it all figured already.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8705853 [details] [diff] [review]
Patch

Yup, that works.

Comment 8

[Eugen Sawin [:esawin]]

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?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/387061724760

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/387061724760

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Kats, can you uplift to Aurora please?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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

Comment 14

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-01-18 18:47:51.png

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)

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8705853 [details] [diff] [review]
Patch

Fix a regression, taking it.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bcf61edc0983

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using: 
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 45.0a2 (2016-01-25)