601143 - Add loading/failure UI for Get Add-ons

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We should display a small loading message before the page loads and some fallback content if it never loads.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we not already have these strings from previous versions of Firefox?

Comment 2

[Dave Townsend [:mossop]]

Attachment: Loading

This is what we already have for the detail view and I think it is safe to just reuse it, though it could do with a little prettification.

Comment 3

[Dave Townsend [:mossop]]

Attachment: FAIL

Rough reproduction of the main text as we talked about. Feels to me a little as if we want an additional line beneath saying something about coming back later or when they have an internet connection to see more information.

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #3)
> Created attachment 480151 [details]
> FAIL
> 
> Rough reproduction of the main text as we talked about. Feels to me a little as
> if we want an additional line beneath saying something about coming back later
> or when they have an internet connection to see more information.

Maybe all that's needed is one sentence on the end of that, such as "When you're connected to the internet, this pane will feature some of the best and most popular add-ons for you to try out!"

Comment 5

[Dave Townsend [:mossop]]

Attachment: FAIL v2

Added the extra text, can I get UI sign-off on the strings so we can get them landed, the specific styling can come shortly after.

Comment 6

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 480222 [details]
FAIL v2

ui-r+ for string. Some final styling is needed on the right pane

Comment 7

[Dave Townsend [:mossop]]

Attachment: WIP

WIP for reference

Comment 8

[Dave Townsend [:mossop]]

Attachment: strings patch rev 1

Just the strings for pre-landing

Comment 9

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

(In reply to comment #6)
> Comment on attachment 480222 [details]
> FAIL v2
> 
> ui-r+ for string. Some final styling is needed on the right pane

Having a separate paragraph for the offline message would be nice. Right now it's hard to separate from the existing text. Not sure if users will read everything. They will probably miss the last sentence.

Comment 10

[Dave Townsend [:mossop]]

Attachment: strings patch rev 2 [checked in]

Comment 11

[Dave Townsend [:mossop]]

Attachment: FAIL v3

Comment 12

[Dave Townsend [:mossop]]

Strings landed: http://hg.mozilla.org/mozilla-central/rev/b1557040c156

Comment 13

[Rimas Kudelis [use needinfo to ping]]

The strings contain hardcoded word "Firefox" instead of "&brandShortName;". I guess you would want to fix this and notify the localizers.

Comment 14

[Dave Townsend [:mossop]]

Attachment: strings patch 2 rev 1 [checked in]

Nice catch, this fixes that.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 480505 [details] [diff] [review]
strings patch 2 rev 1 [checked in]

Landed: http://hg.mozilla.org/mozilla-central/rev/1bbc1bc4d118

Comment 16

[Alfred Kayser]

Something got landed with this bug as reference:
http://hg.mozilla.org/mozilla-central/rev/3019bcf3e9ab

But I don't see any patch in the attachment list that corresponds,
and the change are for Pinstripe/Mac only???

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> Something got landed with this bug as reference:
> http://hg.mozilla.org/mozilla-central/rev/3019bcf3e9ab

And got backed out again immediately after http://hg.mozilla.org/mozilla-central/rev/398deaf9cbdf

Comment 18

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This implements the loading a failure cases, for now it treats moving to a new domain as a failure, unless we figure out what is causing bug 602256 we actually can't reliably block the load of the new domain so opening them into a new window would leave the page shown in both the new window and get add-ons which doesn't seem like the right choice.

The theming is taken directly from the existing discovery pane and is the same on all platforms.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 481279 [details] [diff] [review]
patch rev 1

>+  home: null,

Had to look at the code to see what this was - rename to something like homepageURL?

>+        gDiscoverView.home = Services.io.newURI(url, null, null);
>+      }
>+      catch (e) {
>+        gDiscoverView.showError();
>+        return;
>+      }
>+
>+      if (gDiscoverView.loaded)
>+        gDiscoverView.loadBrowser(notifyInitialized);

Nit: Would prefer if |var self = this;| was used here - that's the way used through most of the rest of the code for this sort of thing.


>+  loadBrowser: function(aCallback) {
>+    this.node.selectedPanel = this._loading;
>+
>+    if (aCallback)
>+      this.loadListeners.push(aCallback);
>+
>+    gDiscoverView._browser.goHome();
>+  },

In my testing, if the first load doesn't complete (eg, by switching to offline mode, or hitting the stop button in the navigation bar), subsequent loads won't complete either - it just constantly shows the "Loading..." message.

>+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>+    // Only care about the network stop status events
>+    if (!(aStateFlags & (Ci.nsIWebProgressListener.STATE_IS_NETWORK)) ||
>+        !(aStateFlags & (Ci.nsIWebProgressListener.STATE_STOP)))
>+      return;
>+
>+    // Sometimes we stop getting onLocationChange events so we must redo the
>+    // url tests here (bug 602256)
>+    var location = this._browser.currentURI;
>+
>+    // If the new page is not on the correct host or is not https when the
>+    // default page is https or there was an error loading the page then show
>+    // the error message
>+    if (location.host != this.home.host ||

Since the initial page is about:blank, its possible for this to be called for that URL - which has no host, so accessing location.host will throw. Its also possible that this.home won't be initialized yet, as the webprogress listener is registered before the async call (that sets this.home) completes.

>+        (aRequest && aRequest instanceof Ci.nsIHttpChannel && !aRequest.requestSucceeded)) {

If AMO returns a 404 or 403 error, it doesn't seem correct to display a message that suggests the problem lies with the user's internet connection. It should either say there was some error with the webservice, or just display whatever the webservive returned.

>+  onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress,
>+                             aMaxSelfProgress, aCurTotalProgress,
>+                             aMaxTotalProgress) { },
>+  onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) { },

No need to specify what the arguments are, if the functions are never used.

>+        <deck id="discover-view" flex="1" class="view-pane">
>+          <vbox id="discover-loading" align="center" pack="center" flex="1">
>+            <hbox class="loading discover-box" align="center">
>+              <image/>
>+              <label value="&loading.label;"/>
>+            </hbox>
>+          </vbox>
>+          <hbox id="discover-error" flex="1" align="center">
>+            <spacer flex="1"/>
>+            <hbox flex="1" class="discover-box" align="center">
>+              <image class="discover-logo"/>
>+              <vbox flex="1" align="stretch">
>+                <label class="discover-title">&discover.title;</label>
>+                <description class="discover-description">&discover.description2;</description>
>+                <description class="discover-footer">&discover.footer;</description>
>+              </vbox>
>+            </hbox>
>+            <spacer flex="1"/>
>+          </hbox>

The loading message box should be styled the same as other loading message boxes. The other box here could be either, though I'm leaning towards having that constistent too. I'm happy to have all such boxes using the style you've added here (for bug 601022, I've made them quite similar anyway) - just as long as they're consistent. For bug 601022 I've been using class="view-alert".

Also, other similar boxes have a <spacer flex="1"/> at the top, and <spacer flex="3"/> at the bottom - so it appears more to the top of the view, closer to where you'd typically start reading from.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 481279 [details] [diff] [review]
patch rev 1

>-#detail-view .loading > image {
>+.loading > image {
>   list-style-image: url("chrome://global/skin/icons/loading_16.png");
> }

This should be moved out of the details section, since its no longer specific to that.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #19)
> Comment on attachment 481279 [details] [diff] [review]
> patch rev 1
> 
> >+  home: null,
> 
> Had to look at the code to see what this was - rename to something like
> homepageURL?

Done

> >+        gDiscoverView.home = Services.io.newURI(url, null, null);
> >+      }
> >+      catch (e) {
> >+        gDiscoverView.showError();
> >+        return;
> >+      }
> >+
> >+      if (gDiscoverView.loaded)
> >+        gDiscoverView.loadBrowser(notifyInitialized);
> 
> Nit: Would prefer if |var self = this;| was used here - that's the way used
> through most of the rest of the code for this sort of thing.

Done

> >+  loadBrowser: function(aCallback) {
> >+    this.node.selectedPanel = this._loading;
> >+
> >+    if (aCallback)
> >+      this.loadListeners.push(aCallback);
> >+
> >+    gDiscoverView._browser.goHome();
> >+  },
> 
> In my testing, if the first load doesn't complete (eg, by switching to offline
> mode, or hitting the stop button in the navigation bar), subsequent loads won't
> complete either - it just constantly shows the "Loading..." message.

Fixed and tested

> >+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> >+    // Only care about the network stop status events
> >+    if (!(aStateFlags & (Ci.nsIWebProgressListener.STATE_IS_NETWORK)) ||
> >+        !(aStateFlags & (Ci.nsIWebProgressListener.STATE_STOP)))
> >+      return;
> >+
> >+    // Sometimes we stop getting onLocationChange events so we must redo the
> >+    // url tests here (bug 602256)
> >+    var location = this._browser.currentURI;
> >+
> >+    // If the new page is not on the correct host or is not https when the
> >+    // default page is https or there was an error loading the page then show
> >+    // the error message
> >+    if (location.host != this.home.host ||
> 
> Since the initial page is about:blank, its possible for this to be called for
> that URL - which has no host, so accessing location.host will throw. Its also
> possible that this.home won't be initialized yet, as the webprogress listener
> is registered before the async call (that sets this.home) completes.

Taken care of the first by explicitly ignoring the about:blank cases. For the second I've made it so the progress listener isn't registered until homepageURL is set.

> >+        (aRequest && aRequest instanceof Ci.nsIHttpChannel && !aRequest.requestSucceeded)) {
> 
> If AMO returns a 404 or 403 error, it doesn't seem correct to display a message
> that suggests the problem lies with the user's internet connection. It should
> either say there was some error with the webservice, or just display whatever
> the webservive returned.

I don't think this warrants adding new strings but I'm also positive that we don't want to display any AMO failure page here. We did agree in the meeting that we were just going to use the same message for all cases but I guess that was before we added the more specific tagline. I guess we could just hide that bit in these cases? A fair bit more effort for what should be a very rare case though.

> >+  onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress,
> >+                             aMaxSelfProgress, aCurTotalProgress,
> >+                             aMaxTotalProgress) { },
> >+  onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) { },
> 
> No need to specify what the arguments are, if the functions are never used.

Done

> >+        <deck id="discover-view" flex="1" class="view-pane">
> >+          <vbox id="discover-loading" align="center" pack="center" flex="1">
> >+            <hbox class="loading discover-box" align="center">
> >+              <image/>
> >+              <label value="&loading.label;"/>
> >+            </hbox>
> >+          </vbox>
> >+          <hbox id="discover-error" flex="1" align="center">
> >+            <spacer flex="1"/>
> >+            <hbox flex="1" class="discover-box" align="center">
> >+              <image class="discover-logo"/>
> >+              <vbox flex="1" align="stretch">
> >+                <label class="discover-title">&discover.title;</label>
> >+                <description class="discover-description">&discover.description2;</description>
> >+                <description class="discover-footer">&discover.footer;</description>
> >+              </vbox>
> >+            </hbox>
> >+            <spacer flex="1"/>
> >+          </hbox>
> 
> The loading message box should be styled the same as other loading message
> boxes. The other box here could be either, though I'm leaning towards having
> that constistent too. I'm happy to have all such boxes using the style you've
> added here (for bug 601022, I've made them quite similar anyway) - just as long
> as they're consistent. For bug 601022 I've been using class="view-alert".

So how about I just set class="view-alert" on them and let bug 601022 take care of the styling?

> Also, other similar boxes have a <spacer flex="1"/> at the top, and <spacer
> flex="3"/> at the bottom - so it appears more to the top of the view, closer to
> where you'd typically start reading from.

Fixed

Comment 22

[Alfred Kayser]

Please remove the spacers, as that should be done using css styling and instead of putting align="center" on a box use -moz-box-align:center, so themers can overrride the styling. Styling doesn't belong in content.

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #21)
> So how about I just set class="view-alert" on them and let bug 601022 take care
> of the styling?

Yep, that works.

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #22)
> Please remove the spacers, as that should be done using css styling and instead
> of putting align="center" on a box use -moz-box-align:center, so themers can
> overrride the styling. Styling doesn't belong in content.

For these cases, we don't use the same flex value for the 2 spacers - its purposefully not centered (its more to the top, nearer to where you'd normally start reading). So the effect can't be done without using spacers. However, the flex attribute on the spacers could be moved into the theme (via -moz-box-flex). I'll look into doing that in bug 601022.

Comment 25

[Dave Townsend [:mossop]]

Waiting on bug 601022 as we want to use the view-alert class there

Comment 26

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Un-butrotted and fixes from the review and other landings.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 493077 [details] [diff] [review]
patch rev 2

> var gDiscoverView = {
>   node: null,
>   enabled: true,
>   // Set to true after the view is first shown. If initialization completes
>   // after this then it must also load the discover homepage
>   loaded: false,
>   _browser: null,
>+  _loading: null,
>+  homepageURL: null,
>+  loadListeners: [],

loadListeners seems like it would be a private/internal property, so prefix it with "_" (and maybe homepageURL? not sure).

>+    this._loading = document.getElementById("discover-loading");
>+    this._error = document.getElementById("discover-error");

_error isn't listed as a property of gDiscoverView like _loaded is.

>+      self._browser.addProgressListener(self, Ci.nsIWebProgress.NOTIFY_ALL | Ci.nsIWebProgress.NOTIFY_STATE_ALL);

Nit: wrap this long line.

>+  loadBrowser: function(aCallback) {
>+    this.node.selectedPanel = this._loading;
>+
>+    if (aCallback)
>+      this.loadListeners.push(aCallback);
>+
>+    if (this._browser.currentURI.equals(this.homepageURL))
>+      this._browser.reload();
>+    else
>+      this._browser.goHome();
>+  },

Looks like this will mess up if its called before homepageURL is set (and generally shouldn't be used outside of this view's code) - prefix with "_" as a guard/warning?

>+    // If the hostname is the same and either the home scheme is not https or
>+    // the new location is https then continue with the load

Huh? Reword, or add punctuation, or something.

>+    if (aLocation.host == this.homepageURL.host &&
>+        (!this.homepageURL.schemeIs("https") || aLocation.schemeIs("https")))
>+      return;

I assume we're just blindly relying on AMO to always target external links to _blank? (I'm ok with that)

>+
>+    this.showError();
>+    aRequest.cancel(Components.results.NS_BINDING_ABORTED);

Will this leave the browser in a state where the discovery page is still usable? I would have expected to need to cancel the request before onLocationChange (using something like nsIURIContentListener.onStartURIOpen).

Also, will aborting the request trigger onStateChange? If so, might not need to be calling showError() here.

>+  onSecurityChange: function(aWebProgress, aRequest, aState) {
...
>+    this.showError();
>+    aRequest.cancel(Components.results.NS_BINDING_ABORTED);

Ditto.

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_discovery.js b/toolkit/mozapps/extensions/test/browser/browser_discovery.js
...
>+  onLocationChange: function(aWebProgress, aRequest, aLocation) { },
>+  onSecurityChange: function(aWebProgress, aRequest, aState) { },
>+  onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress,
>+                             aMaxSelfProgress, aCurTotalProgress,
>+                             aMaxTotalProgress) { },
>+  onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) { },

This would be a lot more readable if the parameters were omitted (since they're not being used anyway).

Comment 28

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Updated from comments

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 496192 [details] [diff] [review]
patch rev 3

>+    function setURL(aURL) {
>+      try {
>+        self.homepageURL = Services.io.newURI(aURL, null, null);
>+      }
>+      catch (e) {

Style consistency nit: No newline between } and catch.

Comment 30

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/0c45b1fc2f7c

Comment 31

[Dave Townsend [:mossop]]

Backed out

Comment 32

[Dave Townsend [:mossop]]

Attachment: bustage fix

For some reason sendMouseEvent works more reliably than synthesizeMouse here. Also includes a fix for an assertion that gets logged whenever the add-ons manager is closed.

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 498948 [details] [diff] [review]
bustage fix

Mind giving this a quick stamp Rob?

Comment 34

[Robert Strong (they/them - no direct email)]

Comment on attachment 498948 [details] [diff] [review]
bustage fix

not at all

Comment 35

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/33ef0dc20824

Comment 36

[Alfred Kayser]

Why is a non standard font used for Winstripe: MetaWebPro-Book ?
This ia not very common font on Windows platform, and it is a licensed font

Comment 37

[Dave Townsend [:mossop]]

(In reply to comment #36)
> Why is a non standard font used for Winstripe: MetaWebPro-Book ?
> This ia not very common font on Windows platform, and it is a licensed font

To attempt to match the get add-ons page as much as possible, if not present it should just fall back to something else.

Comment 38

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre ID:20110119030331 in off-line mode.