Add loading/failure UI for Get Add-ons

VERIFIED FIXED in mozilla2.0b9

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

(Depends on: 1 bug)

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [strings][strings landed])

Attachments

(7 attachments, 5 obsolete attachments)

(Assignee)

Description

7 years ago
We should display a small loading message before the page loads and some fallback content if it never loads.
(Assignee)

Updated

7 years ago
blocking2.0: --- → beta8+
Do we not already have these strings from previous versions of Firefox?
(Assignee)

Comment 2

7 years ago
Created attachment 480150 [details]
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.
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
(Assignee)

Updated

7 years ago
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!"
(Assignee)

Comment 5

7 years ago
Created attachment 480222 [details]
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.
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+
(Assignee)

Comment 7

7 years ago
Created attachment 480228 [details] [diff] [review]
WIP

WIP for reference
(Assignee)

Comment 8

7 years ago
Created attachment 480230 [details] [diff] [review]
strings patch rev 1

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.
(Assignee)

Comment 10

7 years ago
Created attachment 480243 [details] [diff] [review]
strings patch rev 2 [checked in]
Attachment #480230 - Attachment is obsolete: true
Attachment #480243 - Flags: review?(bmcbride)
Attachment #480230 - Flags: review?(bmcbride)
(Assignee)

Comment 11

7 years ago
Created attachment 480244 [details]
FAIL v3
Attachment #480222 - Attachment is obsolete: true
Attachment #480243 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 12

7 years ago
Strings landed: http://hg.mozilla.org/mozilla-central/rev/b1557040c156
Whiteboard: [strings] → [strings][strings landed]
(Assignee)

Updated

7 years ago
Attachment #480243 - Attachment description: strings patch rev 2 → strings patch rev 2 [checked in]

Comment 13

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

Comment 14

7 years ago
Created attachment 480505 [details] [diff] [review]
strings patch 2 rev 1 [checked in]

Nice catch, this fixes that.
Attachment #480505 - Flags: review?(bmcbride)
Attachment #480505 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 15

7 years ago
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]

Comment 16

7 years ago
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???
(Assignee)

Comment 17

7 years ago
(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
(Assignee)

Comment 18

7 years ago
Created attachment 481279 [details] [diff] [review]
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.
Attachment #481279 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
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.
(Assignee)

Comment 21

7 years ago
(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

7 years ago
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.
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][needs new patch]
(Assignee)

Comment 25

7 years ago
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]
(Assignee)

Updated

7 years ago
blocking2.0: beta8+ → betaN+
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][needs 601022] → [strings][strings landed]
(Assignee)

Comment 26

7 years ago
Created attachment 493077 [details] [diff] [review]
patch rev 2

Un-butrotted and fixes from the review and other landings.
Attachment #481279 - Attachment is obsolete: true
Attachment #493077 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
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-
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][waiting on try]
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch]
(Assignee)

Comment 28

7 years ago
Created attachment 496192 [details] [diff] [review]
patch rev 3

Updated from comments
Attachment #493077 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][needs review Unfocused]
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
(Assignee)

Comment 30

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/0c45b1fc2f7c
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed]
Target Milestone: --- → mozilla2.0b9
(Assignee)

Comment 31

7 years ago
Backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 32

7 years ago
Created attachment 498948 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed] → [strings][strings landed][has patch][waiting on try]
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch][needs review rs]
(Assignee)

Comment 33

7 years ago
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+
(Assignee)

Comment 35

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/33ef0dc20824
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [strings][strings landed][has patch][needs review rs] → [strings][strings landed]

Comment 36

7 years ago
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
(Assignee)

Comment 37

7 years ago
(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
(Assignee)

Updated

7 years ago
Depends on: 630229
Depends on: 635610
(Assignee)

Updated

6 years ago
Blocks: 556223
You need to log in before you can comment on or make changes to this bug.