585950 - Implement the new list view mockups

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Attachment: mockup

- Each category of add-ons (except Themes/Backgrounds) displays at its top
level a list of all the add-ons it contains
  - Information included in each entry is:
       * Add-on name
       * Add-on icon
       * Add-on author (links to AMO profile)
       * Add-on description (curtailed after one line)
       * Date add-on was last updated
  - Buttons included in each entry are:
        * Disable/enable toggle button
        * Preferences link (launches in separate window)
        * "More" link (launches Detail View of add-on)
  - Clicking an add-on selects that entry; multi-select is enabled
  - Selected add-ons display an X in the top right corner for deletion
  - Notifications for an add-on appear above the add-on's title in its entry
  - Searching for a specific add-on produces a variation on List View, with the
added option to search for add-ons available to download as well as ones which
are currently installed

Comment 1

[Robert Kaiser]

(In reply to comment #0)
>   - Selected add-ons display an X in the top right corner for deletion

Hmm, this is almost completely undiscoverable, I'm sure not finding that will
confuse a good number of users. From all I know, users tend to see an X in the top right corner as "close this view" and not as "uninstall this item". Also, this is not visible in your mockups.

Comment 2

[Leman Bennett [Omega]]

Using an "X" similar to the Close Tab button should communicate that the view can be closed using the "X". It just has to be big enough. 

Also, I don't think it will be confused with "Uninstall This Item" since that is never communicated anywhere in any OS I've used.

Comment 3

[Josh Tumath]

An "X" is used for closing items, not removing them.

Windows has a "wobbly X" for deleting items. You could use that in a button next to "disable".

Comment 4

[Dave Townsend [:mossop]]

Attachment: initial patch rev 1

This patch gets us 90% of the way there so I'd like to get this reviewed and landed in advance of the final pieces. The colours are just approximations of what they will finally be but changing the styles is an easy follow-up job once we have them.

A couple of important changes happen here. Add-on richlistitems now only have two possible "status" values. "installing" is for items in the process of being downloaded or installed. Once installed (even if a restart is pending) they become "installed" and show full details. A "notification" attribute is set if there is something to warn the user about (blocklisting, incompatibility). A "pending" attribute is set if an operation is waiting for a restart to complete. For uninstalling that switches to the simpler binding as previously, for others it shows the notification at the top with the option to undo or restart.

I'm going to work up some tests for the notifications and pending operation cases tomorrow.

Comment 5

[Dave Townsend [:mossop]]

Attachment: overall styling patch rev 1

And for lack of a better place to put it this is the overall styling for the add-ons manager with some theme specific bits. Again colours are approximations. On Linux I've used system colours which seems to work out ok at least on the couple of Ubuntu themes I've tested on.

Comment 6

[Michael Monreal [:micmon]]

>> An "X" is used for closing items, not removing them.

Right this irritated me as well. On linux you can use the "gtk-delete" icon instead. But personally I would prefer a button with "Uninstall" label next to disable, as it would be easier to understand and look better too IMHO

>> On Linux I've used system colours which seems to work out ok at
>> least on the couple of Ubuntu themes I've tested on.

Looks mostly fine on the themes I have tested as well. Only the list "border" should probably have another color.

Comment 7

[Michael Monreal [:micmon]]

Btw, does the redesign get rid of the "Restart now to complete installation" link at the top of the page that is currently displayed when extensions are newly installed?

Comment 8

[Dave Townsend [:mossop]]

Attachment: Initial patch rev 2

Updated patch. I added a decent testcase for the main list view, it revealed one or two minor issues with the code and the mock add-ons that I've corrected. Also tweaked your is_element_hidden to take simpler arguments and have a matching is_element_visible function, they also check whether parent elements are hidden.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #6)
> >> On Linux I've used system colours which seems to work out ok at
> >> least on the couple of Ubuntu themes I've tested on.
> 
> Looks mostly fine on the themes I have tested as well. Only the list "border"
> should probably have another color.

Yeah that is one bit that I didn't check, I'll look to see if there is a good OS colour choice when I go over the colours in a follow-up patch.

Comment 10

[Dave Townsend [:mossop]]

Note that bug 562902 tweaks some of the styles from this patch but I didn't want to iterate too much here. Also as there the tests for hte update date are broken on some platforms due to formatting.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

General notes first. Some may need some UX input.

* Still really really dislike the close button
* Links on selected items are hard to read
* Selected but not active items look ugly (ie, when the listbox doesn't have focus, but does have an item selected)
* Header takes up too much space, and having the header text aligned to the far left (rather than the viewport) looks awkward
* Sorters need re-styled
* When enabling an addon, the disable button should show (and the reverse for disabling)
* When uninstalling, install notification doesn't mention anything about needing to restart to complete the uninstall.
* For new installs, download progress widget is not aligned to the far right
* For upgrades, no message is shown in the notification, and the download status widget displays "Installing", which is misleading. Also, item should be a different color (or somehow more easy to pick out in the list)
* When an upgrade is pending restart, switching to another view and back again breaks the binding.
* Would like spacer elements on the left of the categories container and the right of the viewport deck, with a small flex (the viewport with a large flex). This should make the margins scale when on a very large screen, and help combat the huge whitespace between the left and right sides of listview items (and the detail view too).
* Opening the release notes makes the icon slide down - it should stay aligned with the addon name.
* The "More" button to go into detail view isn't aligned perperly with the description line.
* "More" button looks out of place when there is no description.
* On a small screen, the last update date looks weird because its not lined up with the addon name
* Remove "Size" from the sorters
* AFAIK (I didn't test this), if an addon is softblocked and disabled, and you enable it, the pending restart notification will be shown but the softblock notification won't be. I think they should both be shown (ie, blocklist notifications should always shown, no matter what).


Its hard to say much about the CSS colors while its in flux like this. I imagine there will be a lot of followups and small things to fix/tweak once the final colors and textures are finally put in place.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 466856 [details] [diff] [review]
Initial patch rev 2

>+<!ENTITY addon.details                        "More">

No tooltip?

>+#LOCALIZATION NOTE (notification.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
>+notification.incompatible=%1$S is incompatible with %2$S %3$S.
>+#LOCALIZATION NOTE (notification.blocked) %1$S is the add-on name
>+notification.blocked=%1$S has been blocked due to stability issues.

Blocklist message needs to say something about security issues too.

>+#LOCALIZATION NOTE (notification.softblocked) %1$S is the add-on name
>+notification.softblocked=%1$S may cause stability issues.

Ditto.

Also, since you're handling bug 553491 here, need to add a string for outdated plugins (does the plugin provider support that?), and/or file a followup bug.

>+#LOCALIZATION NOTE (notification.enable) %1$S is the add-on name, %2$S is brand name
>+notification.enable=%1$S will be enabled after you restart %2$S.
>+#LOCALIZATION NOTE (notification.disable) %1$S is the add-on name, %2$S is brand name
>+notification.disable=%1$S will be disabled after you restart %2$S.
>+#LOCALIZATION NOTE (notification.install) %1$S is the add-on name, %2$S is brand name
>+notification.install=%1$S will be installed after you restart %2$S.
>+#LOCALIZATION NOTE (notification.uninstall) %1$S is the add-on name, %2$S is brand name
>+notification.uninstall=%1$S will be uninstalled after you restart %2$S.

Missing a string for updates.

>-          } else if (this.mAddon) {
>-
>-            // Not using an AddonInstall object for this action
>-            var pending = this.mAddon.pendingOperations;
>-            if (pending & AddonManager.PENDING_INSTALL) {
>-              this.showMessage("installInstallPending", true);
>-            } else if (pending & AddonManager.PENDING_UPGRADE) {
>-              this.showMessage("installUpdatePending", true);
>-            } else if (pending & AddonManager.PENDING_ENABLE) {
>-              this.showMessage("installEnablePending", true);
>-              showUndo = true;
>-            } else if (pending & AddonManager.PENDING_DISABLE) {
>-              this.showMessage("installDisablePending", true);
>-              showUndo = true;
>-            }
>-            showRestartNeeded = true;
>-

Why is this removed? Are external installs (ie, without an AddonInstall object) handled ok still?

>+          <xul:hbox align="end" class="description-container">
>+            <xul:label flex="1" anonid="description" class="description" crop="end"/>
>+            <xul:button class="details button-link"
>+                        label="&addon.details;"
>+                        oncommand="document.getBindingParent(this).showInDetailView();"/>
>+            <xul:spacer flex="5000"/> <!-- Necessary to make the description crop -->

This crops fine for me without the spacer element.

>         this._creator.setCreator(creator, creatorURL || homepageURL);

With the homepage no longer having a dedicated link, I think the homepage should get priority over the author's homepage here.


>+          }
>+          else {

Nit: dominant style here is to not have the newline between the "}" and the "else". (Real reason: that newline really bugs me, but I can probably live with it. Maybe.)

  
>       <method name="uninstall">
>         <body><![CDATA[
>           // If uninstalling does not require a restart then just disable it
>           // and show the undo UI.
>           if (!this.opRequiresRestart("uninstall")) {
>+            this.setAttribute("wasDisabled", this.mAddon.userDisabled);
>+            // Disable first so _updateState doesn't overwrite the
>+            // pending/notification preferences
>+            this.mAddon.userDisabled = true;

This isn't clear to me.

>diff --git a/toolkit/mozapps/extensions/test/browser/head.js b/toolkit/mozapps/extensions/test/browser/head.js
>   isVisible: function(aCategory) {
>     isnot(this.window, null, "Should not check visible state when manager window is not loaded");
>     if (aCategory.hasAttribute("disabled") &&
>         aCategory.getAttribute("disabled") == "true")
>       return false;
> 
>-    var style = this.window.document.defaultView.getComputedStyle(aCategory, "");
>-    return style.display != "none" && style.visibility == "visible";
>+    return !is_hidden(aCategory);;

Nit: double semicolon.


>+.icon {
>   list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
> }

Should at least have max-width and max-height of 48px here.

>+.warning, .pending, .error, .info {
>+  -moz-margin-start: 48px;
>+}

The makes the notification text not quite line up the with addon name. I think its off by about 2 pixels.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 466544 [details] [diff] [review]
overall styling patch rev 1

>+/* HTML link elements do weird things to the layout if they are not hidden */
>+link {
>+  display: none;
>+}

Prefix this with the xhtml namespace.

>-    <splitter class="pane-splitter" collapse="none"/>

I still really want to keep this splitter. It makes it less painful on small screens, since the categories take up so much room.

>+#view-port {
>+  background-color: -moz-field;
>+  color: -moz-fieldtext;

Shouldn't these colours be Window and WindowText?

> .category {
>-  border: none;
>   -moz-appearance: none;
>+  border-width: 1px 0 1px 1px;
>+  border-style: solid;
>+  -moz-border-radius-topleft: 5px;
>+  -moz-border-radius-bottomleft: 5px;
>+  border-color: transparent;
>   padding: 10px 4px;
>   -moz-box-align: center;
>   overflow: hidden;
>   min-height: 0px;
> }

This isn't RTL friendly.

> .addon-control {
>-  -moz-appearance: none;
>-  padding: 0px 5px;
>-  border: 1px solid #A2A6AD;
>-  -moz-border-radius: 100%;
>-  background-image: -moz-linear-gradient(#F9F9F9, #DFDFDF);
> }

Was it intentional to leave this block empty?

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 466856 [details] [diff] [review]
Initial patch rev 2

Making the control buttons visible again for pending actions and fixed all the below, just updating the tests and then will upload the new version.

(In reply to comment #12)
> Comment on attachment 466856 [details] [diff] [review]
> Initial patch rev 2
> 
> >+<!ENTITY addon.details                        "More">
> 
> No tooltip?

Done

> >+#LOCALIZATION NOTE (notification.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
> >+notification.incompatible=%1$S is incompatible with %2$S %3$S.
> >+#LOCALIZATION NOTE (notification.blocked) %1$S is the add-on name
> >+notification.blocked=%1$S has been blocked due to stability issues.
> 
> Blocklist message needs to say something about security issues too.
>
> >+#LOCALIZATION NOTE (notification.softblocked) %1$S is the add-on name
> >+notification.softblocked=%1$S may cause stability issues.
> 
> Ditto.

Done these, also remembered we need a "More information" link to the blocklist page.

> Also, since you're handling bug 553491 here, need to add a string for outdated
> plugins (does the plugin provider support that?), and/or file a followup bug.

I'll do something along those lines in bug 563135.

> >+#LOCALIZATION NOTE (notification.enable) %1$S is the add-on name, %2$S is brand name
> >+notification.enable=%1$S will be enabled after you restart %2$S.
> >+#LOCALIZATION NOTE (notification.disable) %1$S is the add-on name, %2$S is brand name
> >+notification.disable=%1$S will be disabled after you restart %2$S.
> >+#LOCALIZATION NOTE (notification.install) %1$S is the add-on name, %2$S is brand name
> >+notification.install=%1$S will be installed after you restart %2$S.
> >+#LOCALIZATION NOTE (notification.uninstall) %1$S is the add-on name, %2$S is brand name
> >+notification.uninstall=%1$S will be uninstalled after you restart %2$S.
> 
> Missing a string for updates.

Good catch.

> >-          } else if (this.mAddon) {
> >-
> >-            // Not using an AddonInstall object for this action
> >-            var pending = this.mAddon.pendingOperations;
> >-            if (pending & AddonManager.PENDING_INSTALL) {
> >-              this.showMessage("installInstallPending", true);
> >-            } else if (pending & AddonManager.PENDING_UPGRADE) {
> >-              this.showMessage("installUpdatePending", true);
> >-            } else if (pending & AddonManager.PENDING_ENABLE) {
> >-              this.showMessage("installEnablePending", true);
> >-              showUndo = true;
> >-            } else if (pending & AddonManager.PENDING_DISABLE) {
> >-              this.showMessage("installDisablePending", true);
> >-              showUndo = true;
> >-            }
> >-            showRestartNeeded = true;
> >-
> 
> Why is this removed? Are external installs (ie, without an AddonInstall object)
> handled ok still?

Yes, external installs are now just created as regular state="installed" list items so none of this logic is necessary anymore.

> >+          <xul:hbox align="end" class="description-container">
> >+            <xul:label flex="1" anonid="description" class="description" crop="end"/>
> >+            <xul:button class="details button-link"
> >+                        label="&addon.details;"
> >+                        oncommand="document.getBindingParent(this).showInDetailView();"/>
> >+            <xul:spacer flex="5000"/> <!-- Necessary to make the description crop -->
> 
> This crops fine for me without the spacer element.
> 
> >         this._creator.setCreator(creator, creatorURL || homepageURL);
> 
> With the homepage no longer having a dedicated link, I think the homepage
> should get priority over the author's homepage here.

I agree and I've done it but lets follow up on it after this has landed to confirm we're ok with the behaviour.

> >+          }
> >+          else {
> 
> Nit: dominant style here is to not have the newline between the "}" and the
> "else". (Real reason: that newline really bugs me, but I can probably live with
> it. Maybe.)

I can switch.

> 
> 
> >       <method name="uninstall">
> >         <body><![CDATA[
> >           // If uninstalling does not require a restart then just disable it
> >           // and show the undo UI.
> >           if (!this.opRequiresRestart("uninstall")) {
> >+            this.setAttribute("wasDisabled", this.mAddon.userDisabled);
> >+            // Disable first so _updateState doesn't overwrite the
> >+            // pending/notification preferences
> >+            this.mAddon.userDisabled = true;
> 
> This isn't clear to me.

Tried to improve the comment, basically when setting userDisabled _updateState will get called which will remove any pending attribute from the element. So we have to set the pending attribute after disabling for it all to work.

> >diff --git a/toolkit/mozapps/extensions/test/browser/head.js b/toolkit/mozapps/extensions/test/browser/head.js
> >   isVisible: function(aCategory) {
> >     isnot(this.window, null, "Should not check visible state when manager window is not loaded");
> >     if (aCategory.hasAttribute("disabled") &&
> >         aCategory.getAttribute("disabled") == "true")
> >       return false;
> > 
> >-    var style = this.window.document.defaultView.getComputedStyle(aCategory, "");
> >-    return style.display != "none" && style.visibility == "visible";
> >+    return !is_hidden(aCategory);;
> 
> Nit: double semicolon.

Fixed.

> >+.icon {
> >   list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
> > }
> 
> Should at least have max-width and max-height of 48px here.

Fixed.

> >+.warning, .pending, .error, .info {
> >+  -moz-margin-start: 48px;
> >+}
> 
> The makes the notification text not quite line up the with addon name. I think
> its off by about 2 pixels.

Ah, didn't spot that that is what we were going for, will correct.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 466544 [details] [diff] [review]
overall styling patch rev 1

(In reply to comment #13)
> Comment on attachment 466544 [details] [diff] [review]
> overall styling patch rev 1
> 
> >+/* HTML link elements do weird things to the layout if they are not hidden */
> >+link {
> >+  display: none;
> >+}

Done

> Prefix this with the xhtml namespace.
> 
> >-    <splitter class="pane-splitter" collapse="none"/>
> 
> I still really want to keep this splitter. It makes it less painful on small
> screens, since the categories take up so much room.

I'll see how well that works out with the new design.

> >+#view-port {
> >+  background-color: -moz-field;
> >+  color: -moz-fieldtext;
> 
> Shouldn't these colours be Window and WindowText?

Possibly yes, though richlistbox defaults to moz-field and moz-fieldtext.

> > .category {
> >-  border: none;
> >   -moz-appearance: none;
> >+  border-width: 1px 0 1px 1px;
> >+  border-style: solid;
> >+  -moz-border-radius-topleft: 5px;
> >+  -moz-border-radius-bottomleft: 5px;
> >+  border-color: transparent;
> >   padding: 10px 4px;
> >   -moz-box-align: center;
> >   overflow: hidden;
> >   min-height: 0px;
> > }
> 
> This isn't RTL friendly.

Yeah...

> > .addon-control {
> >-  -moz-appearance: none;
> >-  padding: 0px 5px;
> >-  border: 1px solid #A2A6AD;
> >-  -moz-border-radius: 100%;
> >-  background-image: -moz-linear-gradient(#F9F9F9, #DFDFDF);
> > }
> 
> Was it intentional to leave this block empty?

Probably not.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> > >-    <splitter class="pane-splitter" collapse="none"/>
> > 
> > I still really want to keep this splitter. It makes it less painful on small
> > screens, since the categories take up so much room.
> 
> I'll see how well that works out with the new design.

This turns out to be pretty difficult to do and unless you have a quick suggestion for how to make it work I'd rather not hold up this patch on it. It is something we can land post-beta 5. The problem is that to allow the category to appear to eat into the border of the view the category list has to slightly overlap the view which means it also overlaps any splitter there and so you can't interact with it.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #11)
> General notes first. Some may need some UX input.
> 
> * Still really really dislike the close button
> * Links on selected items are hard to read
> * Selected but not active items look ugly (ie, when the listbox doesn't have
> focus, but does have an item selected)
> * Header takes up too much space, and having the header text aligned to the far
> left (rather than the viewport) looks awkward

The text will go away and be replaced by the buttons from the mockups in bug 562797

> * Sorters need re-styled

Waiting on the final colours etc.

> * When enabling an addon, the disable button should show (and the reverse for
> disabling)

Fixed.

> * When uninstalling, install notification doesn't mention anything about
> needing to restart to complete the uninstall.

I don't understand what you mean here.

> * For new installs, download progress widget is not aligned to the far right

Strange, worked in some cases but not others. Should be fixed now though.

> * For upgrades, no message is shown in the notification, and the download
> status widget displays "Installing", which is misleading. Also, item should be
> a different color (or somehow more easy to pick out in the list)

Fixed though the undo button doesn't work, this is a backend problem, I've filed bug 588925 on it.

> * When an upgrade is pending restart, switching to another view and back again
> breaks the binding.

Actually I think the binding was wrong before the switch. Corrected.

> * Would like spacer elements on the left of the categories container and the
> right of the viewport deck, with a small flex (the viewport with a large flex).
> This should make the margins scale when on a very large screen, and help combat
> the huge whitespace between the left and right sides of listview items (and the
> detail view too).

Not sure if this makes things any better or just look mismatched, but we can try it out.

> * Opening the release notes makes the icon slide down - it should stay aligned
> with the addon name.

Fixed.

> * The "More" button to go into detail view isn't aligned perperly with the
> description line.

Looks better now I think.

> * "More" button looks out of place when there is no description.

Not sure what else to do in that case.

> * On a small screen, the last update date looks weird because its not lined up
> with the addon name

Let's figure out how to handle this separately.

> * Remove "Size" from the sorters

Done.

> * AFAIK (I didn't test this), if an addon is softblocked and disabled, and you
> enable it, the pending restart notification will be shown but the softblock
> notification won't be. I think they should both be shown (ie, blocklist
> notifications should always shown, no matter what).

Boriss only wants to show one thing at a time, and I agree.

> Its hard to say much about the CSS colors while its in flux like this. I
> imagine there will be a lot of followups and small things to fix/tweak once the
> final colors and textures are finally put in place.

Yes, thankfully that is something we can do after b5.

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> > * Would like spacer elements on the left of the categories container and the
> > right of the viewport deck, with a small flex (the viewport with a large flex).
> > This should make the margins scale when on a very large screen, and help combat
> > the huge whitespace between the left and right sides of listview items (and the
> > detail view too).
> 
> Not sure if this makes things any better or just look mismatched, but we can
> try it out.

Actually this does horrible things making the layout bounce as you switch between categories. Let's do this separately if we can figure it out.

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #12)
> >+          <xul:hbox align="end" class="description-container">
> >+            <xul:label flex="1" anonid="description" class="description" crop="end"/>
> >+            <xul:button class="details button-link"
> >+                        label="&addon.details;"
> >+                        oncommand="document.getBindingParent(this).showInDetailView();"/>
> >+            <xul:spacer flex="5000"/> <!-- Necessary to make the description crop -->
> 
> This crops fine for me without the spacer element.

Without the spacer the description expands to fill the horizontal space pushing the link off to the side which isn't what we want, but it does crop. If I take flex off the description then it doesn't crop at all. Having the spacer is the only configuration that did both correctly for me.

Comment 20

[Dave Townsend [:mossop]]

Attachment: list view patch rev 3

Update patch for the list view part. I also had to make the notification messages at the top able to wrap, it is a little sucky as we can't wrap the links in-line with them but the blocklist message is long and causes scrollbar issues on linux, and test failures because then the buttons go out of view and can't be clicked on.

Comment 21

[Dave Townsend [:mossop]]

Attachment: overall styling patch rev 2

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #19)
> Without the spacer the description expands to fill the horizontal space pushing
> the link off to the side which isn't what we want, but it does crop. If I take
> flex off the description then it doesn't crop at all. Having the spacer is the
> only configuration that did both correctly for me.

Ah, quite right.

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #20)
> I also had to make the notification
> messages at the top able to wrap, it is a little sucky as we can't wrap the
> links in-line with them

Hm, tricky. might be able to do it by mixing in some HTML elements and/or inline layout trickery. File a followup?

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 467611 [details] [diff] [review]
list view patch rev 3

> .addon[status="installed"] {
>+  -moz-box-orient: vertical;
>   -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#addon-generic");
> }

Since there's no case you'd *not* want vertical orientation, instead add orient="vertical" to the XBL content element.

>+.addon:not([notification="warning"]) .warning,
>+.addon:not([notification="error"]) .error,
>+.addon:not([notification="info"]) .info,
>+.addon:not([pending]) .pending,
>+.addon:not([upgrade="true"]) .update-postfix,
>+.addon[active="true"] .disabled-postfix,
> #detail-view[loading] > .detail-view-container {
>   display: none;
> }

This results in items pending install incorrectly showing the "(disabled)" postfix, since they have active="false".

>   <!-- Sorters - displays and controls the sort state of a list. -->
>   <binding id="sorters">
>     <content orient="horizontal">
>       <xul:button anonid="btn-name" class="sorter"
>                   label="&sort.name.label;" tooltiptext="&sort.name.tooltip;"
>                   oncommand="this.parentNode._handleChange('name');"/>
>-      <xul:button anonid="btn-size" class="sorter"
>-                  label="&sort.size.label;" tooltiptext="&sort.size.tooltip;"
>-                  oncommand="this.parentNode._handleChange('size');"/>

Need to also remove the strings.


>+            if (this.mAddon.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) {
>+              this.setAttribute("notification", "error");
>+              this._error.textContent = gStrings.ext.formatStringFromName(
>+                "notification.blocked",
>+                [this.mAddon.name], 1
>+              );
>+              this._errorLink.value = gStrings.ext.GetStringFromName("notification.blocked.link");
>+              this._errorLink.href = Services.urlFormatter.formatURLPref("extensions.blocklist.detailsURL");
>+              this._errorLink.hidden = false;
>+            } else if (!this.mAddon.isCompatible) {
>+              this.setAttribute("notification", "warning");
>+              this._warning.textContent = gStrings.ext.formatStringFromName(
>+                "notification.incompatible",
>+                [this.mAddon.name, gStrings.brandShortName, gStrings.appVersion], 3
>+              );
>+              this._warningLink.hidden = true;
>+            } else if (this.mAddon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED) {
>+              this.setAttribute("notification", "warning");
>+              this._warning.textContent = gStrings.ext.formatStringFromName(
>+                "notification.softblocked",
>+                [this.mAddon.name], 1
>+              );
>+              this._warningLink.value = gStrings.ext.GetStringFromName("notification.softblocked.link");
>+              this._warningLink.href = Services.urlFormatter.formatURLPref("extensions.blocklist.detailsURL");
>+              this._warningLink.hidden = false;
>+            } else {
>+              this.removeAttribute("notification");
>+            }

Is it intentional to have the incompatibility warning be of higher priority then the softblock warning? Softblocked seems more important to me, and also significantly less common than incompatibility.


>diff --git a/toolkit/mozapps/extensions/test/browser/browser_list.js
 ...
>+// Check that the list appears to have displayed correctly and trigger some
>+// changes
>+add_test(function() {
>+  gCategoryUtilities.openType("extension", function() {
>+    let list = gManagerWindow.document.getElementById("addon-list");
>+    is(list.childNodes.length, 6, "Should be six add-on installed");

Nit: Grammar.



r=me with those small changes, and a bunch of followups blocking final.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 467612 [details] [diff] [review]
overall styling patch rev 2

>+    <button id="header-utils-btn" type="menu">
>+      <menupopup id="utils-menu">
>+        <menuitem id="utils-updateNow"
>+                  label="&updates.updateAddonsNow.label;"
>+                  accesskey="&updates.updateAddonsNow.accesskey;"
>+                  command="cmd_findAllUpdates"/>
>+        <menuitem id="utils-viewUpdates"
>+                  label="&updates.viewUpdates.label;"
>+                  accesskey="&updates.viewUpdates.accesskey;"
>+                  command="cmd_goToRecentUpdates"/>
>+        <menuseparator/>
>+        <menuitem id="utils-backgroudUpdateCheck"
>+                  label="&updates.backgroudUpdateCheck.label;"
>+                  accesskey="&updates.backgroudUpdateCheck.accesskey;"
>+                  type="checkbox" autocheck="false"
>+                  command="cmd_toggleBackgroundUpdateCheck"/>
>+      </menupopup>

When you rebase this, be sure to add back in the menuitem for Install From File.

> .category {
>-  border: none;
>   -moz-appearance: none;
>+  border-width: 1px;
>+  -moz-border-end-width: 0;
>+  border-style: solid;
>+  border-color: transparent;
>   padding: 10px 4px;
>   -moz-box-align: center;
>   overflow: hidden;
>   min-height: 0px;
>+  -moz-border-radius-topleft: 5px;
>+  -moz-border-radius-bottomleft: 5px;
>+}
>+
>+.category:-moz-locale-dir(rtl) {
>+  -moz-border-radius-topright: 5px;
>+  -moz-border-radius-bottomright: 5px;
> }

This is still a little broken in RTL, because the left corners still have a radius applied.


r=me with those 2 fixes.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot of uninstalled item

(In reply to comment #17)
> > * When uninstalling, install notification doesn't mention anything about
> > needing to restart to complete the uninstall.
> 
> I don't understand what you mean here.

That should have read: When uninstalling, uninstall notification text doesn't mention anything about needing to restart to complete the uninstall.

See the attached screenshot.

Turns out, I was actually referring to the uninstallNotice entity in extensions.properties - which has been there since before we first landed everything on mozilla-central.

It's a little inconstant with the wording of the other notifications. The only difference between an uninstall that requires a restart and one that doesn't, is the presence of the "Restart Now" button. But I guess it does still convey the message.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 467611 [details] [diff] [review]
list view patch rev 3

>-            <xul:button type="checkbox" anonid="toggle-more" class="toggle-more"
>-                        label="&showMore.label;"
>-                        tooltiptext="&showMore.tooltip;"
>-                        showless="&showLess.label;"
>-                        showlesstooltip="&showLess.tooltip;"
>-                        showmore="&showMore.label;"
>-                        showmoretooltip="&showMore.tooltip;"
>-                        oncommand="document.getBindingParent(this).toggleDetails();"/>

Almost missed this - need to remove the strings too.

Comment 28

[Dave Townsend [:mossop]]

(In reply to comment #24)
> Comment on attachment 467611 [details] [diff] [review]
> list view patch rev 3
> 
> > .addon[status="installed"] {
> >+  -moz-box-orient: vertical;
> >   -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#addon-generic");
> > }
> 
> Since there's no case you'd *not* want vertical orientation, instead add
> orient="vertical" to the XBL content element.

This actually needs to stay. The problem with putting attributes on the content element is that if you later change bindings, say from the installed to the uninstalled binding, the attribute remains potentially causing the new layout to be incorrect. I don't remember the precise case but I certainly saw it here but either way I'd like to avoid using attributes on these when we know the binding is going to change.

> >+.addon:not([notification="warning"]) .warning,
> >+.addon:not([notification="error"]) .error,
> >+.addon:not([notification="info"]) .info,
> >+.addon:not([pending]) .pending,
> >+.addon:not([upgrade="true"]) .update-postfix,
> >+.addon[active="true"] .disabled-postfix,
> > #detail-view[loading] > .detail-view-container {
> >   display: none;
> > }
> 
> This results in items pending install incorrectly showing the "(disabled)"
> postfix, since they have active="false".

Fixed.

> >+            if (this.mAddon.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) {
> >+              this.setAttribute("notification", "error");
> >+              this._error.textContent = gStrings.ext.formatStringFromName(
> >+                "notification.blocked",
> >+                [this.mAddon.name], 1
> >+              );
> >+              this._errorLink.value = gStrings.ext.GetStringFromName("notification.blocked.link");
> >+              this._errorLink.href = Services.urlFormatter.formatURLPref("extensions.blocklist.detailsURL");
> >+              this._errorLink.hidden = false;
> >+            } else if (!this.mAddon.isCompatible) {
> >+              this.setAttribute("notification", "warning");
> >+              this._warning.textContent = gStrings.ext.formatStringFromName(
> >+                "notification.incompatible",
> >+                [this.mAddon.name, gStrings.brandShortName, gStrings.appVersion], 3
> >+              );
> >+              this._warningLink.hidden = true;
> >+            } else if (this.mAddon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED) {
> >+              this.setAttribute("notification", "warning");
> >+              this._warning.textContent = gStrings.ext.formatStringFromName(
> >+                "notification.softblocked",
> >+                [this.mAddon.name], 1
> >+              );
> >+              this._warningLink.value = gStrings.ext.GetStringFromName("notification.softblocked.link");
> >+              this._warningLink.href = Services.urlFormatter.formatURLPref("extensions.blocklist.detailsURL");
> >+              this._warningLink.hidden = false;
> >+            } else {
> >+              this.removeAttribute("notification");
> >+            }
> 
> Is it intentional to have the incompatibility warning be of higher priority
> then the softblock warning? Softblocked seems more important to me, and also
> significantly less common than incompatibility.

Going to land this as-is but we should think about this more. The reason I think that incompatible is more important is that that is normally what blocks you from using the add-on, soft-blocked add-ons can still be enabled by the user. Perhaps we should give softblocking preference when the user has compatibility checking disabled?

> r=me with those small changes, and a bunch of followups blocking final.

Going to go through and file these today and follow-up with a list.

Comment 29

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/76559e9039e2
http://hg.mozilla.org/mozilla-central/rev/307a672ee758

Comment 30

[pal-moz]

wrong bug # on push log,
307a672ee758	Dave Townsend — Bug 585960: Implement the overall theme changes. r=Unfocused, a=blocking-beta5

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #28)
> > Is it intentional to have the incompatibility warning be of higher priority
> > then the softblock warning? Softblocked seems more important to me, and also
> > significantly less common than incompatibility.
> 
> Going to land this as-is but we should think about this more. The reason I
> think that incompatible is more important is that that is normally what blocks
> you from using the add-on, soft-blocked add-ons can still be enabled by the
> user. Perhaps we should give softblocking preference when the user has
> compatibility checking disabled?

Hm, yes, I like this solution.

Comment 32

[Michael Monreal [:micmon]]

Is the styling basicly considered "done" in today's nightly? If not I will hold on with filing follow-up bugs for polish.

Comment 33

[Peter Henkel [:Terepin]]

Basicaly, yes. There are some elements that need to be tweaked, such as edge of list area, and color styling of entire page, but could be done when chromeless In-Content UI lands.

Comment 34

[Josh Tumath]

(In reply to comment #33)
> but could be done when chromeless In-Content UI lands.

Bug 590130?

Comment 35

[Peter Henkel [:Terepin]]

No, bug 571970.

Comment 36

[Peter Henkel [:Terepin]]

Btw, I can fill fine-tuning bug, if that's OK.

Comment 37

[Peter Henkel [:Terepin]]

(In reply to comment #36)
> Btw, I can fill fine-tuning bug, if that's OK.

Nevermind...

Comment 38

[Dave Townsend [:mossop]]

(In reply to comment #32)
> Is the styling basicly considered "done" in today's nightly? If not I will hold
> on with filing follow-up bugs for polish.

No. The functionality and language of the list view should be complete now,
final styling and pixel polish is waiting on details from UX and will be taken
care of in bug 590175.

Comment 39

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

Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre