Closed Bug 750846 Opened 12 years ago Closed 12 years ago

Make thumbnails look better

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox14 verified, firefox15 verified, blocking-fennec1.0 soft)

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- soft

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(6 files, 4 obsolete files)

Attached image thumbnails screenshot
I've seen several problems with thumbnails. For one, if the page is zoomed in, the thumbnails also get zoomed in, which means they only show a very small portion of the top left page. Second, thumbnails taken when the phone is in the horizontal orientation crop a lot of the page.

I've attached a screenshot of what the thumbnails look like. The pages are all cropped, making the thumbnails unattractive.
Attached patch patch (obsolete) — Splinter Review
This patch uses the size from the page itself to determine the source width/height rather than using the phone's screen dimensions. With this change, thumbnails are now taken using the maximum width possible, and the height is cropped to be proportional to the thumbnail size (or vice-versa if the page is wide).

Since the source width/height are determined in Gecko, createScreenshotEvent() no longer needs to provide srcW/srcH. This allows us to remove a number of methods from Tab.java. sWidth and sHeight were kind of broken anyway since they weren't recalculated when the phone switched orientations.

I also removed the cropping code in updateThumbnail(); I think this is left over from when we were taking full screenshots for pause/resume.
Attachment #620026 - Flags: review?(blassey.bugs)
This is the same set of sites with the patch applied.
Attached patch patch v2 (obsolete) — Splinter Review
This moves the about:home check to getAndProcessThumbnailForTab(). All calls to processThumbnail() first go to getAndProcessThumbnailForTab(), so we may as well do this check earlier and avoid unnecessary JNI calls for about:home thumbnails.
Attachment #620026 - Attachment is obsolete: true
Attachment #620026 - Flags: review?(blassey.bugs)
Attachment #620168 - Flags: review?(blassey.bugs)
I can verify that this also fixes bug 744980.
Blocks: 744980
blocking-fennec1.0: ? → soft
If this does fix bug 744980 we'll take this one ASAP
Attached patch patch v3 (obsolete) — Splinter Review
The previous patch used only body.clientWidth/body.clientHeight to determine the dimensions of the page. clientWidth/clientHeight generally return the dimensions of the page, but on some sites (cnn.com, abc.com), they're actually the dimensions of the window. This can lead to clipping when the phone is in horizontal orientation.

To fix this, we can use document.documentElement.scrollWidth/Height, which always return the actual dimensions of the page. We may still want to use the window width/height in certain situations (if the page is small), so using the maximum dimension should work for all cases.
Attachment #620168 - Attachment is obsolete: true
Attachment #620168 - Flags: review?(blassey.bugs)
Attachment #620533 - Flags: review?(blassey.bugs)
For comparison, here's a screenshot of these two sites using only clientWidth/clientHeight. Notice how they are both clipped on the right side.
Here's a screenshot using the latest patch, which uses the maximum dimension between scrollWidth/clientWidth and scrollHeight/clientHeight.
Comment on attachment 620533 [details] [diff] [review]
patch v3

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

::: widget/android/AndroidBridge.cpp
@@ +2254,5 @@
> +        nsCOMPtr<nsIContent> bodyContent = nodelist->GetNodeAt(0);
> +        if (!bodyContent)
> +            return NS_ERROR_FAILURE;
> +
> +        nsCOMPtr<nsIDOMElement> bodyElement = do_QueryInterface(bodyContent);

don't rely on there being a body element
Attachment #620533 - Flags: review?(blassey.bugs) → review-
Attached patch patch v4 (obsolete) — Splinter Review
We can just use window.innerWidth/innerHeight instead of document.body.clientWidth/clientHeight. This is more reliable anyway.
Attachment #620533 - Attachment is obsolete: true
Attachment #620817 - Flags: review?(blassey.bugs)
Comment on attachment 620817 [details] [diff] [review]
patch v4

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

::: widget/android/AndroidBridge.cpp
@@ +2239,5 @@
>  {
> +    nsresult rv;
> +
> +    // take a screenshot, as wide as possible, proportional to the thumbnail size
> +    if (token == SCREENSHOT_THUMBNAIL) {

probably better to base this off of srcW and srcH being 0
Attachment #620817 - Flags: review?(blassey.bugs) → review+
Attached image background innerWidth
For some reason, backgrounded tabs sometimes have a larger innerWidth than expected:

05-03 18:53:00.863 25918 25935 I Gecko   : tab1 innerWidth: 540
05-03 18:53:00.863 25918 25935 I Gecko   : tab1 scrollWidth: 360
05-03 18:53:00.957 25918 25935 I Gecko   : tab2 innerWidth: 540
05-03 18:53:00.965 25918 25935 I Gecko   : tab2 scrollWidth: 360
05-03 18:53:01.019 25918 25935 I Gecko   : tab3 innerWidth: 360
05-03 18:53:01.019 25918 25935 I Gecko   : tab3 scrollWidth: 360

This is causing black lines around background tabs, as shown in the screenshot.
Attached patch patch v5Splinter Review
Recap: I originally wanted to just use the page's width/height (found using scrollWidth/scrollHeight) for the thumbnail calculations. That way, even if the page extends beyond the viewport, the thumbnail can capture as much of the page as possible. This works for most sites, except those that have a short height (e.g, http://people.mozilla.com/~bnicholson/test/short-page.html - dimensions are logged every 5 seconds to the console). On that page, the scrollHeight is only as big as the blue square, so the thumbnail gets blown up. To fix this, I also included the viewport dimensions (window.innerWidth/window.innerHeight), so we could use the max of the viewport/page dimensions. On small pages, the viewport dimensions would be used so the thumbnail wouldn't be blown up, and on larger pages, the page's dimensions would be used to reduce clipping.

That said, I was never able to come up with a test case where window.innerWidth > document.documentElement.scrollWidth. Even on the page I linked to above, the scrollWidth is the same width as the viewport, even though the scrollHeight is only as big as the blue square. I added the window.innerWidth dimension for the sake of parity with window.innerHeight, but it never appeared to make a difference for any site I looked at.

So since window.innerWidth seems to be buggy, and since I haven't found any instances where it's useful, I removed it in this patch. We still need to use window.innerHeight for the reasons mentioned above.
Attachment #620817 - Attachment is obsolete: true
Attachment #620970 - Flags: review?(blassey.bugs)
Could you file a new bug for the window.innerWidth issue with a test case/STR? That should be investigated.

The one situation I could imagine the page width being smaller than innerWidth is if you have a meta viewport tag with width=50 or some other such small value. Try testing with that.
Attachment #620970 - Flags: review?(blassey.bugs) → review+
(In reply to Kartikaya Gupta (:kats) from comment #16)
> Could you file a new bug for the window.innerWidth issue with a test
> case/STR? That should be investigated.
> 
> The one situation I could imagine the page width being smaller than
> innerWidth is if you have a meta viewport tag with width=50 or some other
> such small value. Try testing with that.

I tried setting the meta viewport here: http://people.mozilla.com/~bnicholson/test/short-page2.html. This shrinks the page width, but also the window width (I have no idea where 200 comes from):

05-04 10:10:12.708 15528 15545 E GeckoConsole: BRN: window.innerWidth = 200
05-04 10:10:12.715 15528 15545 E GeckoConsole: BRN: documentElement.scrollWidth = 200
05-04 10:10:12.723 15528 15545 E GeckoConsole: BRN: window.innerHeight = 315
05-04 10:10:12.731 15528 15545 E GeckoConsole: BRN: documentElement.scrollHeight = 8
The 200 is likely coming from http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3537 - we clamp the meta-viewport width to be no smaller than 200. I didn't realize we did that when I posted my previous comment, but it makes sense.

As for window.innerWidth, maybe that's not what I think it is. I modified your page to also print out window.outerWidth/height and that seems to be reporting sane values (view area).
(In reply to Kartikaya Gupta (:kats) from comment #16)
> Could you file a new bug for the window.innerWidth issue with a test
> case/STR? That should be investigated.
> 
Filed bug 751948.
(In reply to Kartikaya Gupta (:kats) from comment #18)
> The 200 is likely coming from
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#3537 - we clamp the meta-viewport width to be no smaller than
> 200. I didn't realize we did that when I posted my previous comment, but it
> makes sense.
> 
> As for window.innerWidth, maybe that's not what I think it is. I modified
> your page to also print out window.outerWidth/height and that seems to be
> reporting sane values (view area).

Interesting...according to https://developer.mozilla.org/en/DOM/window.outerWidth, the outerWidth is the innerWidth + any browser UI. Since we have no sidebars/scrollbars on mobile, I would have expected it to be the same as innerWidth.
(In reply to Brian Nicholson (:bnicholson) from comment #20)
> Interesting...according to
> https://developer.mozilla.org/en/DOM/window.outerWidth, the outerWidth is
> the innerWidth + any browser UI. Since we have no sidebars/scrollbars on
> mobile, I would have expected it to be the same as innerWidth.

Ah but https://developer.mozilla.org/en/DOM/window.innerWidth says the innerWidth will reflect the CSS viewport if it is set. So that might explain why it returns unexpected values.
https://hg.mozilla.org/mozilla-central/rev/860d97778e21
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Comment on attachment 620970 [details] [diff] [review]
patch v5

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Thumbnails will be zoomed in/cropped (https://bugzilla.mozilla.org/attachment.cgi?id=620020)
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: none
Attachment #620970 - Flags: approval-mozilla-aurora?
(In reply to Brian Nicholson (:bnicholson) from comment #24)
> Comment on attachment 620970 [details] [diff] [review]
> patch v5
> 
> [Approval Request Comment]
> Regression caused by (bug #): 
> User impact if declined: Thumbnails will be zoomed in/cropped
> (https://bugzilla.mozilla.org/attachment.cgi?id=620020)
> Testing completed (on m-c, etc.): m-c
> Risk to taking this patch (and alternatives if risky): low risk
> String changes made by this patch: none

Let's let this back for a couple nightlies before uplifting
We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.
Comment on attachment 620970 [details] [diff] [review]
patch v5

Be sure to land this with the patch for bug 752426.
Attachment #620970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 14.0a2 (2012-05-20)
Status: RESOLVED → VERIFIED
Verified fixed on Nightly 15.0a1 (2012-05-20)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: