Closed
Bug 664895
Opened 14 years ago
Closed 13 years ago
Make the details pane not jump when a screenshot image is loaded
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: Unfocused, Assigned: darktrojan)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Bug 664889 will (hopefully) give us the dimensions of the screenshot image before it's loaded - so it's size can be set as soon as we load the pane, rather than whenever the image eventually loads from the network.
Unfortunately, that will mean that until the image has loaded, there will be this big white space there for no apparent reason. So there should probably be some indication that an image is loading - a loading spinner/icon, and maybe a border around the box.
Comment 1•14 years ago
|
||
Lets get some feedback from UX. Jennifer, what do you think would be the best?
Keywords: uiwanted
Assignee | ||
Comment 2•13 years ago
|
||
I've started work on this.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #555685 -
Flags: review?(bmcbride)
Reporter | ||
Comment 4•13 years ago
|
||
Comment on attachment 555685 [details] [diff] [review]
patch
Review of attachment 555685 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. Definitely need some loading indicator though (see comment 0) - especially on slow connections, there will now be a huge white space for no apparent reason, until the image loads.
::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +977,4 @@
> let caption = self._getDescendantTextContent(aPreviewNode, "caption");
> + let screenshot = new AddonManagerPrivate.
> + AddonScreenshot(fullURL, fullWidth, fullHeight, thumbnailURL,
> + thumbnailWidth, thumbnailHeight, caption);
Style nit: Don't line-break between AddonManagerPrivate and AddonScreenshot - it makes it look like you're creating a new AddonManagerPrivate.
@@ +1278,5 @@
> insertDeveloper: "INSERT INTO developer VALUES (:addon_internal_id, " +
> ":num, :name, :url)",
>
> + // We specify column names here because the columns
> + // could be out of order due to schema changes
Style nit: Comments are sentences, and should end in a fullstop.
@@ +1359,5 @@
> + this.connection.schemaVersion = DB_SCHEMA;
> + } catch (e) {
> + ERROR("Failed to create database schema", e);
> + this.logSQLError(this.connection.lastError, this.connection.lastErrorString);
> + this.connection.rollbackTransaction();
If this errors, then chances are it will do the same next time too. It should remove the file, and try again - and only throw if it's the second try (which I think should never happen, since there will be no migration step on a second try).
@@ +1854,5 @@
> + let thumbnailHeight = aRow.getResultByName("thumbnailHeight");
> + let caption = aRow.getResultByName("caption");
> + return new AddonManagerPrivate.
> + AddonScreenshot(url, width, height, thumbnailURL,
> + thumbnailWidth, thumbnailHeight, caption);
Ditto with this linebreak.
Attachment #555685 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 5•13 years ago
|
||
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #555685 -
Attachment is obsolete: true
Attachment #555957 -
Flags: review?(bmcbride)
Assignee | ||
Comment 7•13 years ago
|
||
Comment on attachment 555957 [details] [diff] [review]
patch v2
Hmm. Should run hg qrefresh before exporting a patch.
Attachment #555957 -
Attachment is obsolete: true
Attachment #555957 -
Flags: review?(bmcbride)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #556203 -
Flags: review?(bmcbride)
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 556203 [details] [diff] [review]
patch v2, again
Review of attachment 556203 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1325,5 @@
>
> let dbfile = FileUtils.getFile(KEY_PROFILEDIR, [FILE_DATABASE], true);
> let dbMissing = !dbfile.exists();
>
> + function tryAgain() {
tryAgain() needs to set this.initialized = false at the start, since the dbFile.remove() call later on can throw.
::: toolkit/mozapps/extensions/content/extensions.js
@@ +2550,3 @@
> screenshot.src = aAddon.screenshots[0].thumbnailURL;
> + screenshot.width = aAddon.screenshots[0].thumbnailWidth;
> + screenshot.height = aAddon.screenshots[0].thumbnailHeight;
Noticed that this is setting the attribute value to "undefined", rather than leaving it alone (you'd think the properties would handle that, but apparently not). This might be why the image takes up no space immediately after an DB upgrade, but before the database has updated data.
@@ +2562,3 @@
> screenshot.hidden = true;
> + screenshot.width = null;
> + screenshot.height = null;
Sadly, this doesn't remove the attribute. Could use setAttribute() above and removeAttribute() here, but no biggie.
::: toolkit/mozapps/extensions/content/extensions.xul
@@ +536,5 @@
> <label id="detail-creator" class="creator"/>
> </vbox>
> <hbox id="detail-desc-container" align="start">
> <vbox pack="center"> <!-- Necessary to work around bug 394738 -->
> + <image id="detail-screenshot" hidden="true" onload="this.removeAttribute('loading');"/>
When using onload, you also need to use onerror, in case the image fails to load (this wasn't a problem when the image didn't take up space by default). At the very least, it needs to remove the loading attribute. And thinking about it, it'd be nice if it indicated that it failed to load. Could keep the background/border, but replace the throbber with chrome://global/skin/media/error.png
::: toolkit/mozapps/extensions/test/xpcshell/test_migrateAddonRepository.js
@@ +6,5 @@
> +function run_test() {
> + do_test_pending();
> + createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
> +
> + // Write out a minimal database
Style nit: fullstop.
@@ +78,5 @@
> +
> + Services.obs.addObserver({
> + observe: function () {
> + Services.obs.removeObserver(this, "addon-repository-shutdown");
> + // Check the DB schema has changed once AddonRepository has freed it
Style nit: fullstop.
::: toolkit/themes/winstripe/mozapps/extensions/extensions.css
@@ +778,5 @@
> max-width: 300px;
> max-height: 300px;
> }
>
> +#detail-screenshot[loading] {
Feels like the sharp corners here are a bit out of place - pretty much everything else has slightly rounded corners. border-radius of 3px looked good, but I didn't play with other values. Ditto for pinstripe, where the sharp corners are more noticeable (due to the darker background).
Attachment #556203 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #556203 -
Attachment is obsolete: true
Attachment #556519 -
Flags: review?(bmcbride)
Assignee | ||
Updated•13 years ago
|
Attachment #555956 -
Attachment is obsolete: true
Reporter | ||
Comment 11•13 years ago
|
||
Comment on attachment 556519 [details] [diff] [review]
patch v3
Review of attachment 556519 [details] [diff] [review]:
-----------------------------------------------------------------
Good to go :)
Attachment #556519 -
Flags: review?(bmcbride) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: uiwanted → checkin-needed
Comment 12•13 years ago
|
||
Fails try:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_details.js | Screenshot dimensions should not be set - Got 160, expected
Stack trace:
JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 372
JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_details.js :: <TOP_LEVEL> :: line 706
JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/head.js :: log_exceptions :: line 90
JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/head.js :: <TOP_LEVEL> :: line 216
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_details.js | Screenshot dimensions should not be set - Got 120, expected
Stack trace:
JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 372
JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_details.js :: <TOP_LEVEL> :: line 707
JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/head.js :: log_exceptions :: line 90
JS frame :: chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/head.js :: <TOP_LEVEL> :: line 216
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=6269685&full=1
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=aeb46bc613de
Keywords: checkin-needed
Assignee | ||
Comment 13•13 years ago
|
||
Bah! I thought I'd already put this through Try, clearly not.
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #556519 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 15•13 years ago
|
||
Keywords: checkin-needed
Target Milestone: --- → mozilla9
Comment 16•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
Hi guys.
Can you please tell me how can I verify this?
Thanks
Comment 18•13 years ago
|
||
This patch has probably caused bug 694007. We don't show a screenshot anymore in the details view of the search pane.
(In reply to Vlad [QA] from comment #17)
> Can you please tell me how can I verify this?
As best in the above mentioned view, when we directly load screenshot and do not have to reflow the page. But due to the bug it's not possible right now.
Flags: in-testsuite+
Flags: in-litmus?
You need to log in
before you can comment on or make changes to this bug.
Description
•