Closed Bug 601143 Opened 10 years ago Closed 10 years ago

Add loading/failure UI for Get Add-ons

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [strings][strings landed])

Attachments

(7 files, 5 obsolete files)

We should display a small loading message before the page loads and some fallback content if it never loads.
blocking2.0: --- → beta8+
Do we not already have these strings from previous versions of Firefox?
Attached image 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.
Attached image FAIL (obsolete) —
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.
Assignee: nobody → dtownsend
Blocks: 557698
(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!"
Attached image FAIL v2 (obsolete) —
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.
Attachment #480151 - Attachment is obsolete: true
Attachment #480222 - Flags: ui-review?
Comment on attachment 480222 [details]
FAIL v2

ui-r+ for string. Some final styling is needed on the right pane
Attachment #480222 - Flags: ui-review? → ui-review+
Attached patch WIPSplinter Review
WIP for reference
Attached patch strings patch rev 1 (obsolete) — Splinter Review
Just the strings for pre-landing
Attachment #480230 - Flags: review?(bmcbride)
(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.
Attachment #480230 - Attachment is obsolete: true
Attachment #480243 - Flags: review?(bmcbride)
Attachment #480230 - Flags: review?(bmcbride)
Attached image FAIL v3
Attachment #480222 - Attachment is obsolete: true
Attachment #480243 - Flags: review?(bmcbride) → review+
Strings landed: http://hg.mozilla.org/mozilla-central/rev/b1557040c156
Whiteboard: [strings] → [strings][strings landed]
Attachment #480243 - Attachment description: strings patch rev 2 → strings patch rev 2 [checked in]
The strings contain hardcoded word "Firefox" instead of "&brandShortName;". I guess you would want to fix this and notify the localizers.
Nice catch, this fixes that.
Attachment #480505 - Flags: review?(bmcbride)
Attachment #480505 - Flags: review?(bmcbride) → review+
Comment on attachment 480505 [details] [diff] [review]
strings patch 2 rev 1 [checked in]

Landed: http://hg.mozilla.org/mozilla-central/rev/1bbc1bc4d118
Attachment #480505 - Attachment description: strings patch 2 rev 1 → strings patch 2 rev 1 [checked in]
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???
(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
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #481279 - Flags: review?(bmcbride)
Whiteboard: [strings][strings landed] → [strings][strings landed][has patch][needs review Unfocused]
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.
Attachment #481279 - Flags: review?(bmcbride) → review-
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.
(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
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.
(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.
(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.
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][needs new patch]
Waiting on bug 601022 as we want to use the view-alert class there
Depends on: 601022
Whiteboard: [strings][strings landed][needs new patch] → [strings][strings landed][needs 601022]
blocking2.0: beta8+ → betaN+
Whiteboard: [strings][strings landed][needs 601022] → [strings][strings landed]
Attached patch patch rev 2 (obsolete) — Splinter Review
Un-butrotted and fixes from the review and other landings.
Attachment #481279 - Attachment is obsolete: true
Attachment #493077 - Flags: review?(bmcbride)
Whiteboard: [strings][strings landed] → [strings][strings landed][has patch][needs review Unfocused]
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).
Attachment #493077 - Flags: review?(bmcbride) → review-
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][waiting on try]
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch]
Attached patch patch rev 3Splinter Review
Updated from comments
Attachment #493077 - Attachment is obsolete: true
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][needs review Unfocused]
Attachment #496192 - Flags: review?(bmcbride)
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.
Attachment #496192 - Flags: review?(bmcbride) → review+
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
Landed: http://hg.mozilla.org/mozilla-central/rev/0c45b1fc2f7c
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed]
Target Milestone: --- → mozilla2.0b9
Backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bustage fixSplinter Review
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.
Whiteboard: [strings][strings landed] → [strings][strings landed][has patch][waiting on try]
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch][needs review rs]
Comment on attachment 498948 [details] [diff] [review]
bustage fix

Mind giving this a quick stamp Rob?
Attachment #498948 - Flags: review?(robert.bugzilla)
Comment on attachment 498948 [details] [diff] [review]
bustage fix

not at all
Attachment #498948 - Flags: review?(robert.bugzilla) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/33ef0dc20824
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [strings][strings landed][has patch][needs review rs] → [strings][strings landed]
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
(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.
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.
Status: RESOLVED → VERIFIED
Depends on: 630229
Depends on: 635610
Blocks: 556223
You need to log in before you can comment on or make changes to this bug.