Make thumbnails look better

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 15
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 620020 [details]
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.
(Assignee)

Comment 1

6 years ago
Created attachment 620026 [details] [diff] [review]
patch

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)
(Assignee)

Comment 2

6 years ago
Created attachment 620027 [details]
thumbnails screenshot (with patch)

This is the same set of sites with the patch applied.
(Assignee)

Comment 3

6 years ago
Created attachment 620168 [details] [diff] [review]
patch v2

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)
(Assignee)

Comment 4

6 years ago
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
(Assignee)

Comment 6

6 years ago
Created attachment 620533 [details] [diff] [review]
patch v3

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)
(Assignee)

Comment 7

6 years ago
Created attachment 620535 [details]
client-size only thumbnails

For comparison, here's a screenshot of these two sites using only clientWidth/clientHeight. Notice how they are both clipped on the right side.
(Assignee)

Comment 8

6 years ago
Created attachment 620537 [details]
client-size and scroll-size thumbnails

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-
(Assignee)

Comment 10

6 years ago
Created attachment 620817 [details] [diff] [review]
patch v4

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+
(Assignee)

Comment 13

6 years ago
Created attachment 620928 [details]
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.
(Assignee)

Comment 15

6 years ago
Created attachment 620970 [details] [diff] [review]
patch v5

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+
(Assignee)

Comment 17

6 years ago
(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).
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(Assignee)

Comment 24

6 years ago
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.
Duplicate of this bug: 744980
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+
status-firefox14: --- → fixed
status-firefox15: --- → fixed
Verified fixed on Aurora 14.0a2 (2012-05-20)
Status: RESOLVED → VERIFIED
Verified fixed on Nightly 15.0a1 (2012-05-20)
status-firefox14: fixed → verified
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.