Closed
Bug 750846
Opened 12 years ago
Closed 12 years ago
Make thumbnails look better
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14 verified, firefox15 verified, blocking-fennec1.0 soft)
VERIFIED
FIXED
Firefox 15
People
(Reporter: bnicholson, Assigned: bnicholson)
References
Details
Attachments
(6 files, 4 obsolete files)
248.82 KB,
image/png
|
Details | |
254.04 KB,
image/png
|
Details | |
196.93 KB,
image/png
|
Details | |
149.82 KB,
image/png
|
Details | |
134.85 KB,
image/png
|
Details | |
11.83 KB,
patch
|
blassey
:
review+
joe
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
This is the same set of sites with the patch applied.
Assignee | ||
Comment 3•12 years ago
|
||
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)
Updated•12 years ago
|
blocking-fennec1.0: ? → soft
Comment 5•12 years ago
|
||
If this does fix bug 744980 we'll take this one ASAP
Assignee | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Here's a screenshot using the latest patch, which uses the maximum dimension between scrollWidth/clientWidth and scrollHeight/clientHeight.
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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 11•12 years ago
|
||
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 12•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/7e237e96018f
Assignee | ||
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
Backing out until problems in comment 13 are fixed. http://hg.mozilla.org/integration/mozilla-inbound/rev/3de88b31bbcb
Assignee | ||
Comment 15•12 years ago
|
||
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)
Comment 16•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #620970 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 17•12 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
Comment 18•12 years ago
|
||
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•12 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•12 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.
Assignee | ||
Comment 21•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/860d97778e21
Comment 22•12 years ago
|
||
(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.
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/860d97778e21
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Assignee | ||
Comment 24•12 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?
Comment 25•12 years ago
|
||
(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
Comment 26•12 years ago
|
||
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 28•12 years ago
|
||
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+
Assignee | ||
Comment 29•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/ca2e6ec11127
Updated•12 years ago
|
status-firefox14:
--- → fixed
status-firefox15:
--- → fixed
Comment 31•12 years ago
|
||
Verified fixed on Nightly 15.0a1 (2012-05-20)
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
•