664895 - Make the details pane not jump when a screenshot image is loaded

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets get some feedback from UX. Jennifer, what do you think would be the best?

Comment 2

[Geoff Lankow (:darktrojan)]

I've started work on this.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: patch

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: changes to patch

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 555957 [details] [diff] [review]
patch v2

Hmm. Should run hg qrefresh before exporting a patch.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: patch v2, again

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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("[e-mail]", "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).

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 556519 [details] [diff] [review]
patch v3

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

Good to go :)

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Geoff Lankow (:darktrojan)]

Bah! I thought I'd already put this through Try, clearly not.

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: patch v4

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ce73fa4fd8c0

That's better!

Comment 15

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/80de869f7273

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/80de869f7273

Comment 17

[Vlad [QA]]

Hi guys.
Can you please tell me how can I verify this?
Thanks

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.