710126 - images wider than they are long can't be fully viewed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Alex Keybl [:akeybl]]

Web pages that are wider than they are long can't be viewed in full. This is especially an issue with images and presumably video.

Comment 1

[Alex Keybl [:akeybl]]

See https://bug706667.bugzilla.mozilla.org/attachment.cgi?id=580811 on a phone in portrait mode. This may be specific to images, haven't tested elsewhere.

Comment 2

[Alex Keybl [:akeybl]]

Attachment: fennec screenshot

Comment 3

[Alex Keybl [:akeybl]]

Attachment: browser screenshot

Comment 4

[Tony Chung [:tchung]]

recommend a P2 fix.  this is a lousy user experience

Comment 5

[Martijn Wargers (dead)]

Is this still an issue in current builds?

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #5)
> Is this still an issue in current builds?

Yes, as of 3 days ago this was still a problem with all images wider than they are long. Are you unable to reproduce or something?

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Can you please try again with the latest build and if this occurs, can you place in the device you used?  This does not reproduce for us with the latest build.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Naoki Hirata :nhirata from comment #7)
> Can you please try again with the latest build and if this occurs, can you
> place in the device you used?  This does not reproduce for us with the
> latest build.

Today's build (3/5).

1) Go to http://1.bp.blogspot.com/-j14W8K8Yvug/TbnWYj9gUQI/AAAAAAAADZM/sNgtfe4-c3M/s1600/beautiful-blue-bird-wallpaper+1+%25281%2529.jpg
2) Cannot zoom out to view full image because it is wider than it is long

Comment 9

[Alex Keybl [:akeybl]]

Not fixing this for our 1.0 means we're OK with direct image viewing being functionally broken for presumably half of the images on the internet at release.

Comment 11

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

Attachment: Scale images to fit on load

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 608477 [details] [diff] [review]
Scale images to fit on load

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

r+ with the comment below addressed.

::: mobile/android/chrome/content/browser.js
@@ +2220,5 @@
>            // and then use the metadata to figure out how it needs to be updated
>            ViewportHandler.updateMetadata(this);
>  
> +          // for images, scale to fit width
> +          if (contentDocument instanceof ImageDocument) {

Might be nice to stick the above this.setResolution into an else clause here, so that they don't ever accidentally get re-ordered... Also, the tab's viewport meta-data will be out of date after this call (as ViewportHandler.updateMetadata(this) is called above), is this intentional?

Comment 13

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

(In reply to Chris Lord [:cwiiis] from comment #12)
> Might be nice to stick the above this.setResolution into an else clause
> here, so that they don't ever accidentally get re-ordered... Also, the tab's
> viewport meta-data will be out of date after this call (as
> ViewportHandler.updateMetadata(this) is called above), is this intentional?

Are you referring to the setResolution that's above the updateMetadata call? The order of calls here is important, yes, and I think needs to be done in exactly the way I wrote it. First we reset the zoom to "default". Then we run updateMetadata, which figures out the right CSS viewport and *scales* the zoom to match. Note that it doesn't clobber the default zoom, but uses it as a factor in the calculation of the new zoom, so setting the "default" zoom must be done beforehand. (Note that updateMetadata gets called from other places as well so we have to do that scaling relative to the existing zoom instead of just coming up with some absolute zoom).

Finally, after setting the CSS viewport (which affects page size), we can figure out what zoom we need to make image-pages fit based on the new page size. Does that make sense?

I think the only real bit of redundancy with my patch as written is that the updateMetadata call (at the end of updateViewportSize) sets a resolution and sends a viewport update that is immediately clobbered by the new code I added. However this is only the case for image documents. (Actually, now that I'm looking at the whole code path, the sendViewportUpdate() I had you leave in this before-first-paint handler is not needed because the updateMetadata will do that anyway. I can move it into the image document if-condition if you prefer).

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #13)
> (In reply to Chris Lord [:cwiiis] from comment #12)
> > Might be nice to stick the above this.setResolution into an else clause
> > here, so that they don't ever accidentally get re-ordered... Also, the tab's
> > viewport meta-data will be out of date after this call (as
> > ViewportHandler.updateMetadata(this) is called above), is this intentional?
> 
> Are you referring to the setResolution that's above the updateMetadata call?
> The order of calls here is important, yes, and I think needs to be done in
> exactly the way I wrote it. First we reset the zoom to "default". Then we
> run updateMetadata, which figures out the right CSS viewport and *scales*
> the zoom to match. Note that it doesn't clobber the default zoom, but uses
> it as a factor in the calculation of the new zoom, so setting the "default"
> zoom must be done beforehand. (Note that updateMetadata gets called from
> other places as well so we have to do that scaling relative to the existing
> zoom instead of just coming up with some absolute zoom).
> 
> Finally, after setting the CSS viewport (which affects page size), we can
> figure out what zoom we need to make image-pages fit based on the new page
> size. Does that make sense?
> 
> I think the only real bit of redundancy with my patch as written is that the
> updateMetadata call (at the end of updateViewportSize) sets a resolution and
> sends a viewport update that is immediately clobbered by the new code I
> added. However this is only the case for image documents. (Actually, now
> that I'm looking at the whole code path, the sendViewportUpdate() I had you
> leave in this before-first-paint handler is not needed because the
> updateMetadata will do that anyway. I can move it into the image document
> if-condition if you prefer).

I think moving it to the if-condition and adding a comment in said block explaining this would be good.

Comment 15

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

Attachment: Scale images to fit on load (v2)

Updated comments and moved the sendViewportUpdate into the if block. Re-requesting review (please be as harsh as you like, I want to make sure this code is understandable and correct...)

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 608703 [details] [diff] [review]
Scale images to fit on load (v2)

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

Comments read well to me, r+, but comment below may be something to consider.

::: mobile/android/chrome/content/browser.js
@@ +2232,5 @@
> +          // the MozScrolledAreaChanged event. If that didn't happen, the updateMetadata
> +          // call above does so at the end of the updateViewportSize function. As long
> +          // as that is happening, we don't need to do it again here.
> +
> +          if (contentDocument instanceof ImageDocument) {

Out of interest, do we want to do something similar for SVGDocument too? And videos? Maybe it'd be better to do a !(contentDocument instanceof HTMLDocument) instead?

Comment 17

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

(In reply to Chris Lord [:cwiiis] from comment #16)
> Out of interest, do we want to do something similar for SVGDocument too? And
> videos? Maybe it'd be better to do a !(contentDocument instanceof
> HTMLDocument) instead?

Unfortunately we can't just do !(contentDocument instanceof HTMLDocument) because all ImageDocuments are also HTMLDocuments. But yeah, it makes sense to do this for SVGDocument as well. I don't know what video ends up as, will try to find out.

Comment 18

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

So it appears that the VideoDocument and MediaDocument interfaces are not exposed to JS, so we can't really do the same for those types. SVGs are already fit to width because gecko rescales SVG images to fit in the CSS viewport if needed, and bug 712065 took care of making sure it shows up ok (I verified that it still works in latest code). So while it is kind of annoying that we can't do this consistently across all non-HTML document types, I think is the best we can do for now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa32e30bab34

Comment 20

[Matt Brubeck (:mbrubeck)]

(In reply to Kartikaya Gupta (:kats) from comment #18)
> So it appears that the VideoDocument and MediaDocument interfaces are not
> exposed to JS, so we can't really do the same for those types.

You can use "if (contentDocument.mozSyntheticDocument)" to check for all of those types (see bug 719271 for example).

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[Martijn Wargers (dead)]

Verified fixed in today's build on the Samsung Galaxy Nexus.