Closed Bug 562902 Opened 10 years ago Closed 9 years ago

Implementation of the Detail View design mockups

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: whimboo, Assigned: mossop)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [AddonsRewriteTestday][rewrite][strings])

Attachments

(7 files, 6 obsolete files)

12.89 KB, patch
Unfocused
: review+
rstrong
: superreview+
Details | Diff | Splinter Review
347.42 KB, image/png
Details
138.61 KB, image/png
Details
1.98 MB, image/png
Details
113.84 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
132.55 KB, image/png
Details
1.36 KB, patch
Details | Diff | Splinter Review
We have to fully implement the Detail View. Right now most of the elements are only placeholders with no action behind.

Design docs are here:
https://wiki.mozilla.org/Firefox/Projects/Extension_Manager_Redesign/design#Detail_View
Depends on: 562925
Depends on: 562937
Depends on: 562942
Depends on: 562956
Priority: -- → P2
No longer depends on: 562956
I'm not sure there is much to be gained in keeping this bug around. Are there any other issues that aren't separately filed?
I don't think that everything has been filed yet. So if you would prefer to have it closed, please do so and I will check for items which have to be filed as new bugs.
I think it would make sense to have a tracking bug for the implementation of the new mockups we will get soon. So I would prefer to have it still existent for our spreadsheet. Dave, is that a target for beta 2?
blocking2.0: --- → ?
blocking2.0: ? → final+
Assignee: nobody → dtownsend
Depends on: 578086
blocking2.0: final+ → betaN+
Depends on: 561261
Depends on: 562916
Depends on: 563072
Depends on: 562927
Part of this involves adding support for a larger icon to be displayed in the details view. This patch adds that to the API as icon64URL which maps to icon64URL in the install.rdf or icon64.png in the XPI's files.
Attachment #462847 - Flags: review?(robert.bugzilla)
Attached patch UI patch rev 1 (obsolete) — Splinter Review
This is probably not the final patch but it contains the bulk of the work including fixes for most of the bugs this blocks, all that will likely be left after this patch is arguing over which fields display when so I'd like to just get this reviewed and landed soon.
Attachment #462851 - Flags: review?(bmcbride)
Blocks: 553461
Needed by string freeze
blocking2.0: betaN+ → beta5+
No longer depends on: 562927
Comment on attachment 462851 [details] [diff] [review]
UI patch rev 1

Bunch of high-level stuff first, some of which is more directed at Boriss:


Having "Updated" and "Updates" as the only two rows makes those headings harder to skim. Perhaps rename "Updated" to "Last Updated"? Also improves consistency with the sort button in list view.

Addons that have updated via AMO have "releases.mozilla.org" as their download location - which is technically true, but not what I'd expect to see (since I originally got it from AMO, I expect to see addons.mozilla.org). Would be good to have this as a link too.

There's no strong indication as to whether an add-on is enabled or disabled - you have to determine it based on the actions available (ie, if you can enable it, then it must currently be disabled).

Description is vertically aligned to the bottom of the screenshot - should be aligned to the top.

Uncached wallpapers have no icon or screenshot - which makes it look like something is broken (when really it is not).

Screenshot thumbnail shows the full screenshot, rather than a scaled down thumbnail. Is that going to be fixed here or in bug 553461? I assume that depends on bug 585069.

Plugins have no icons.

Header for rows of information at the bottom is in bold - mockups have this as normal font weight.

Lines between rows at the bottom are black instead of the grey in the mockups.

When there is only one row of information at the bottom, having a line underneath it looks weird. Making the lines lighter (see above) may alleviate this.

Check For Updates button is styled as a button, rather than a text link like in the mockups.

Addons with only the 32x32px icon look inferior to addons with the larger 64x64px icon. Can we try scaling up 32x32px icons? Using image-rendering: -moz-crisp-edges; might help with keeping them looking decent.

Need more rows of information.
(In reply to comment #7)
> Comment on attachment 462851 [details] [diff] [review]
> UI patch rev 1
> 
> Bunch of high-level stuff first, some of which is more directed at Boriss:
> 
> 
> Having "Updated" and "Updates" as the only two rows makes those headings harder
> to skim. Perhaps rename "Updated" to "Last Updated"? Also improves consistency
> with the sort button in list view.

Is this a typo? Not sure what you mean here.

> Addons that have updated via AMO have "releases.mozilla.org" as their download
> location - which is technically true, but not what I'd expect to see (since I
> originally got it from AMO, I expect to see addons.mozilla.org). Would be good
> to have this as a link too.

Yes I don't like it either. I really just want to replace this with the homepage, we're not going to have anything useful for the user here I think.

> There's no strong indication as to whether an add-on is enabled or disabled -
> you have to determine it based on the actions available (ie, if you can enable
> it, then it must currently be disabled).
> 
> Description is vertically aligned to the bottom of the screenshot - should be
> aligned to the top.

Not according to the mockup that Boriss gave me.

> Uncached wallpapers have no icon or screenshot - which makes it look like
> something is broken (when really it is not).

This isn't a new problem and we should have a separate bug on file for it.

> Screenshot thumbnail shows the full screenshot, rather than a scaled down
> thumbnail. Is that going to be fixed here or in bug 553461? I assume that
> depends on bug 585069.

I guess we should do the thumbnail.

> Lines between rows at the bottom are black instead of the grey in the mockups.

I need colours etc. for everything but unfortunately we still have no mockups for anything but Windows, first pass is just to get everything right, colours can be tweaked easily enough.

> Check For Updates button is styled as a button, rather than a text link like in
> the mockups.

Going to talk to Boriss about this, I think it should be a button. Using links in the content page to perform an action seems wrong to me.
Attaching image of an extension in detail view for Windows 7
Comment on attachment 462851 [details] [diff] [review]
UI patch rev 1

Going to rework this now
Attachment #462851 - Flags: review?(bmcbride)
Attachment #462847 - Flags: review?(robert.bugzilla) → review?(bmcbride)
Comment on attachment 462847 [details] [diff] [review]
Add icon64URL to API

> const PROP_METADATA      = ["id", "version", "type", "internalName", "updateURL",
>-                            "updateKey", "optionsURL", "aboutURL", "iconURL"]
>+                            "updateKey", "optionsURL", "aboutURL", "iconURL",
>+                            "icon64URL"]

How annoying would it be to rename iconURL to icon32URL? It would reduce a lot of ambiguity, and possibly save having a lot of issues if/when 32x32px icons are depreciated. Similarly, I wonder if it makes sense to also support icon32.png (in addition to icon.png) and an addon.icon32URL property. I'm fine with either - but its something that should be considered.


> const FIELDS_ADDON = "internal_id, id, location, version, type, internalName, " +
>-                     "updateURL, updateKey, optionsURL, aboutURL, iconURL, " +
>-                     "defaultLocale, visible, active, userDisabled, appDisabled, " +
>-                     "pendingUninstall, descriptor, installDate, updateDate, " +
>-                     "applyBackgroundUpdates, bootstrap, skinnable, size, " +
>-                     "sourceURI, releaseNotesURI";
>+                     "updateURL, updateKey, optionsURL, aboutURL, iconURL," +
>+                     "icon64URL, defaultLocale, visible, active, userDisabled, " +
>+                     "appDisabled, pendingUninstall, descriptor, installDate, " +
>+                     "updateDate, applyBackgroundUpdates, bootstrap, skinnable, " +
>+                     "size, sourceURI, releaseNotesURI";

Nit: First changed line only removes a space at the end of the string (which all other lines keep).

The DB schema migration seems to be overkill (taking out all data, re-creating the DB from scratch, and re-inserting all data) for this small change. Whats the long-term plan there? Keep the current generic solution as-is, or do a more granular schema upgrade like what places does?

r=me with that one nit fixed. The rest is gravy and/or followup fodder.
Attachment #462847 - Flags: review?(bmcbride) → review+
(In reply to comment #7)
> Comment on attachment 462851 [details] [diff] [review]
> UI patch rev 1

(see attachment for mockups of the following)

> Having "Updated" and "Updates" as the only two rows makes those headings harder
> to skim. Perhaps rename "Updated" to "Last Updated"? Also improves consistency
> with the sort button in list view.

1. The current mockups actually say "Last Updated" and "Updates."  However, these strings could still be confusingly similar; they could both relate to the last update.  The meaning of "Updates" in particular is not clear from the name of the category.  Also, you could argue that "manual" is a confusing term for "it's the user's responsibility" - it's not a term we use elsewhere in Firefox that I'm aware of.  What we could do instead is rename the second category to "Automatic updates" and then have the radio buttons simply say "on" and "off."  This clears up both the meaning of the category (whether your updates are automatic) and simplifies the terms by changing the potentially confusing automatic/manual to the much simpler on/off.

> Addons that have updated via AMO have "releases.mozilla.org" as their download
> location - which is technically true, but not what I'd expect to see (since I
> originally got it from AMO, I expect to see addons.mozilla.org). Would be good
> to have this as a link too.

2. If an add-on was downloaded from AMO, let's show its AMO profile URL and link to it on this line.  So far on this detail page there's a direct link to an add-on's comments and author, but no explicit link to its profile.  That presents a bit of a missing piece which the line for source is intended to solve.  If an add-on does not come from AMO, let's present instead the homepage stored in its .xpi file.  We should also specify which is being shown via the string.  Perhaps we can use "Firefox Add-ons Profile" for AMO and "Add-on's Homepage" for the .xpi file link.

> There's no strong indication as to whether an add-on is enabled or disabled -
> you have to determine it based on the actions available (ie, if you can enable
> it, then it must currently be disabled).

3. If an add-on is disabled, this should be explicit in the details page.  This can be done by appending "(disabled)" to the title, reducing the opacity of the title and icon by 75% and adding a 10% black overlay to the background.  If a user presses disable, the transition to 10% black overlay would be a smooth transition.

> Description is vertically aligned to the bottom of the screenshot - should be
> aligned to the top.

4. The reason the description is aligned to bottom is to create a solid block of clean whitespace on the top right.  The alternative is to create two areas of white space separated by a visually awkward block of horizontal text.

> Screenshot thumbnail shows the full screenshot, rather than a scaled down
> thumbnail. Is that going to be fixed here or in bug 553461? I assume that
> depends on bug 585069.

5. For each preview and screenshot, we should be showing the image that displays on addons.mozilla.org.  This will be a scaled-down thumbnail in most cases.

> Plugins have no icons.

6. Since Plug-ins have no icons, let's just show the generic add-on "puzzle" icon, but in 32x32 pixel mode.  This is similar to how any add-on would appear if only a 32x32 icons is available.

> Header for rows of information at the bottom is in bold - mockups have this as
> normal font weight.

7. Normal font weight should be sufficient for the category names; the spacing should sufficiently differentiate them.

> Check For Updates button is styled as a button, rather than a text link like in
> the mockups.

8. It's a fair point that it's inconsistent to style the action for manual updating the way external links are styled in the details page.  This is at least true on the details page - in general we do sometimes use this styling for in-content actions, such as undo and restart.

The issue with the manual update action is that it's meant as an advanced feature that I'd hesitate to promote to the level of primary UI implied with button styling.  If it were easy to put the entire update choice under an add-on's preferences, I'd recommend that.  But, given that this area is now a pop-up window styled only by the add-on's author, it seems wrong to infringe on their area and difficult to add an item to whatever kind of UI might be written in preferences.  In future Firefox, when an add-on's preferences are shown inside the details pane, this might be possible. 

We also can't add this option to the advanced "gear" menu, because it is a specific rather than global option.

Given these difficulties, I'm going to recommend we keep "Check for Updates" styled as a link not because it's a fabulous solution, but that temporarily it's the lesser of a few evils.  What we lose by keeping it is consistency within the page and possibly surprising the user by not opening another page.  What we gain is an easily scannable layout that doesn't have too many unnecessary buttons and does not give the user the incorrect impression that manually updating is recommended functionality.

One change I'd like to propose is that we only give the option to update manually if automatic updates are off.  Considering Firefox should be keeping automatically updated add-ons up-to-date the vast majority of the time, the only time an add-on would not be up to date is if Firefox has not yet checked for updates, or an update has not yet been applied because the user has not been idle between the automatic download and installation of an update.  These would not constitute much time: far less than 24 hours, for instance.  That makes manually updating while automatic updates are selected fairly advanced functionality, and I think we gain understandability of this process by not exposing the option to manually update by default.

> Addons with only the 32x32px icon look inferior to addons with the larger
> 64x64px icon. Can we try scaling up 32x32px icons? Using image-rendering:
> -moz-crisp-edges; might help with keeping them looking decent.

9. If moz-crisp-edges makes the vast majority of add-on icons look better, we should use it.  However, I have some doubts considering how many add-on icons are not line and vector drawings.  What I'd recommend if most icons don't look better scaled up is to adjust the margin slightly depending if no 64x64 icon is available with a consistent ratio of icon size to margin.
(In reply to comment #13)
> of the category.  Also, you could argue that "manual" is a confusing term for
> "it's the user's responsibility" - it's not a term we use elsewhere in Firefox
> that I'm aware of.  What we could do instead is rename the second category to
> "Automatic updates" and then have the radio buttons simply say "on" and "off." 

Why do we need two separate controls here? Shouldn't things like that get a checkbox?

> stored in its .xpi file.  We should also specify which is being shown via the
> string.  Perhaps we can use "Firefox Add-ons Profile" for AMO and "Add-on's
> Homepage" for the .xpi file link.

When I read this the first time it wasn't clear for me for what profile stands for. Can't we always call it out "Homepage"? An add-on always has a home page whether it is located on AMO or comes from another website.

> > Screenshot thumbnail shows the full screenshot, rather than a scaled down
> > thumbnail. Is that going to be fixed here or in bug 553461? I assume that
> > depends on bug 585069.
> 
> 5. For each preview and screenshot, we should be showing the image that
> displays on addons.mozilla.org.  This will be a scaled-down thumbnail in most
> cases.

How many of those screenshots are we going to display? Is it only the first one for the time being until we have the lightbox viewer in place?

> 9. If moz-crisp-edges makes the vast majority of add-on icons look better, we
> should use it.  However, I have some doubts considering how many add-on icons
> are not line and vector drawings.  What I'd recommend if most icons don't look
> better scaled up is to adjust the margin slightly depending if no 64x64 icon is
> available with a consistent ratio of icon size to margin.

IMO the same should apply to icons for add-ons then.
(In reply to comment #13)
> 1. The current mockups actually say "Last Updated" and "Updates."  However,
> these strings could still be confusingly similar; they could both relate to the
> last update.  The meaning of "Updates" in particular is not clear from the name
> of the category.  Also, you could argue that "manual" is a confusing term for
> "it's the user's responsibility" - it's not a term we use elsewhere in Firefox
> that I'm aware of.  What we could do instead is rename the second category to
> "Automatic updates" and then have the radio buttons simply say "on" and "off." 
> This clears up both the meaning of the category (whether your updates are
> automatic) and simplifies the terms by changing the potentially confusing
> automatic/manual to the much simpler on/off.

I like naming it "Automatic updates". Though On/Off feels a lot like Windows' Ok/Cancel buttons (ie, they're not self-describing). But maybe that's not an issue here? 

We could use the same/similar wording as the application update preferences use. That uses "Ask me what I want to do" for the manual case, and "Automatically download and install the update" for the automatic case.

(In reply to comment #14)
> Why do we need two separate controls here? Shouldn't things like that get a
> checkbox?

It currently uses a checkbox. But when I first saw it done as two radio boxes, my first reaction was "oh, I like this much better". Note that the application update preference uses two radio buttons too.


(In reply to comment #13)
> > Description is vertically aligned to the bottom of the screenshot - should be
> > aligned to the top.
> 
> 4. The reason the description is aligned to bottom is to create a solid block
> of clean whitespace on the top right.  The alternative is to create two areas
> of white space separated by a visually awkward block of horizontal text.

Hm, I kinda see that reasoning. But it still makes it look broken to me.


> The issue with the manual update action is that it's meant as an advanced
> feature that I'd hesitate to promote to the level of primary UI implied with
> button styling.

I agree with that - being a button, it screams "CLICK ME".


> One change I'd like to propose is that we only give the option to update
> manually if automatic updates are off.

This is a double-edged sword. Normal users should have no use for it, but I know I'd want to be able to explicitly check for updates for a specific addon that I had set to automatically update. For example, if I know there's a new version and I want it NOW. To force an update check, I'd have to disable automatic checking first - and I may not remember to re-enable it. 

Although, there's still the context menu item to do this in the list view. Even if it'd feel strange to have an action available in the list view that's not available in the detail view.


> 9. If moz-crisp-edges makes the vast majority of add-on icons look better, we
> should use it.  However, I have some doubts considering how many add-on icons
> are not line and vector drawings.

Think we'll need to experiment here, and get input from the AMO guys. -moz-crisp-edges works great with pixel-art images (because it doesn't add any blurring), but not as great with smooth gradients.


(In reply to comment #14)
> When I read this the first time it wasn't clear for me for what profile stands
> for. Can't we always call it out "Homepage"? An add-on always has a home page
> whether it is located on AMO or comes from another website.

Agreed - "profile" is ambiguous to me too. But the AMO profile and the addon homepage are distinctly different (and are used differently). And not all addons have a homepage, not all addons have an AMO profile, while many addons have both a homepage and an AMO profile.
(In reply to comment #15)
> (In reply to comment #13)
> > One change I'd like to propose is that we only give the option to update
> > manually if automatic updates are off.
> 
> This is a double-edged sword. Normal users should have no use for it, but I
> know I'd want to be able to explicitly check for updates for a specific addon
> that I had set to automatically update. For example, if I know there's a new
> version and I want it NOW. To force an update check, I'd have to disable
> automatic checking first - and I may not remember to re-enable it. 
> 
> Although, there's still the context menu item to do this in the list view. Even
> if it'd feel strange to have an action available in the list view that's not
> available in the detail view.

I'm for it.  Being able to manually check is one hell of an advanced feature, and I have no problem making it an easter egg.  We could have it be a context menu item in detail view as well as list view.  I share your skepticism about having a context menu item on an "area" rather than an item, but that seems less strange than having context-level functionality in list view that's not in detail view.  Detail view should always be a superset of add-on functionality, for consistency and workflow.  Imagine you know there is an update for Add-on X, but you're not sure if it's compatible with your browser.  You'd have to first enter detail view to check the compatibility, but then go back to list view to get the context menu item to manually update.  Bleh.

> > 9. If moz-crisp-edges makes the vast majority of add-on icons look better, we
> > should use it.  However, I have some doubts considering how many add-on icons
> > are not line and vector drawings.
> 
> Think we'll need to experiment here, and get input from the AMO guys.
> -moz-crisp-edges works great with pixel-art images (because it doesn't add any
> blurring), but not as great with smooth gradients.

Yep, let's try some test cases on gradient icons.  Unfortunately, what with the devastating popularity of this "web 2.0" the kids like these days, gradients are everywhere.  In the 10 most popular add-ons' icons, 9 are using gradients (props to Wladimir Palant).  It's unlikely to be a positive enough result to apply everywhere.
(In reply to comment #16)
> I'm for it.  Being able to manually check is one hell of an advanced feature,
> and I have no problem making it an easter egg.  We could have it be a context
> menu item in detail view as well as list view.

I fully agree here. Actually, even working on developing a Mozilla-based app for advanced users, this enable/disable of updates looked like a stranger to me when I looked at the mockups. It's nice to have some UI for it, but we shouldn't be so in-the-face with it, esp. as we are going the exact opposite way with app update, which we are trying to hide from the user as much as possible.
This may be because they are just mockups, but I am pretty sure that the profile URLs are supposed to be of the form https://addons.mozilla.org/addon/1328/ - ie without language or application.
>6. Since Plug-ins have no icons, let's just show the generic add-on "puzzle"
>icon, but in 32x32 pixel mode.  This is similar to how any add-on would appear
>if only a 32x32 icons is available.

Shouldn't Plug-ins use the pluginGeneric graphic, or better yet an updated version based on the OOPP graphics?  It seems confusing to use the puzzle piece for something that behaves and installs completely different than extensions or themes. A plug-in isn't technically an "add-on" in the same way a Theme or Extension or Search-Engine is.  Users may not understand the difference, but the UI shouldn't encourage the misunderstanding.
(In reply to comment #18)
> This may be because they are just mockups, but I am pretty sure that the
> profile URLs are supposed to be of the form
> https://addons.mozilla.org/addon/1328/ - ie without language or application.

For the AMO profile page, we'd actually get that URL from AMO's API. I'm not sure exactly what format it will ultimately be in, as the current official API doesn't add the language or application, but Zamboni's API does (that's AMO's new backend that's being developed). However, both versions append an additional src parameter, so the final URL is more like:

https://addons.mozilla.org/addon/1865?src=api (current API)
https://addons.mozilla.org/en-US/firefox/addon/1865/?src=api (Zamboni's API)

This makes it less friendly to display as-is. However, I'm not sure if we can normalize what is displayed, since we need to support repositories other than AMO, which may use different URL formats.

Another URL we could potentially display is the addon's support URL. Addon authors can tell AMO where users should do to get support for an addon - whether it be the addon's homepage, a knowledge base, or a forum.
(In reply to comment #12)
> Comment on attachment 462847 [details] [diff] [review]
> Add icon64URL to API
> 
> > const PROP_METADATA      = ["id", "version", "type", "internalName", "updateURL",
> >-                            "updateKey", "optionsURL", "aboutURL", "iconURL"]
> >+                            "updateKey", "optionsURL", "aboutURL", "iconURL",
> >+                            "icon64URL"]
> 
> How annoying would it be to rename iconURL to icon32URL? It would reduce a lot
> of ambiguity, and possibly save having a lot of issues if/when 32x32px icons
> are depreciated. Similarly, I wonder if it makes sense to also support
> icon32.png (in addition to icon.png) and an addon.icon32URL property. I'm fine
> with either - but its something that should be considered.

Trouble is that the outcome of the icon size discussions was that we'd be using the basic icon for the 48px version, so this is really a 32 or 48 pixel icon so I don't think it makes sense to distinguish. It is just the basic icon.

> The DB schema migration seems to be overkill (taking out all data, re-creating
> the DB from scratch, and re-inserting all data) for this small change. Whats
> the long-term plan there? Keep the current generic solution as-is, or do a more
> granular schema upgrade like what places does?

I think the plan is to just keep the generic migration for now. It is going to be fairly rare where a migration just involves db manipulations. In this case for example as well as adding the column to the database we also need to re-read all install.rdf files to pick up any new data. The benefits of the current solution are that the code ends up being much better tested (we are after all using the same code to add new items during a migration that we do during a normal install) and it is useful for all migrations, forwards and backwards. It is perhaps less efficient but the less one-off code the better I think.
Comment on attachment 462847 [details] [diff] [review]
Add icon64URL to API

Bleh, this is adding an icon64URL property to the Addon API and so needs sr.
Attachment #462847 - Flags: superreview?(robert.bugzilla)
Attachment #462847 - Flags: superreview?(robert.bugzilla) → superreview+
(In reply to comment #21)
> Trouble is that the outcome of the icon size discussions was that we'd be using
> the basic icon for the 48px version, so this is really a 32 or 48 pixel icon so
> I don't think it makes sense to distinguish. It is just the basic icon.

Bah, forgot about that. Hope that's not going to be too confusing later down the line :\


> I think the plan is to just keep the generic migration for now. It is going to
> be fairly rare where a migration just involves db manipulations. In this case
> for example as well as adding the column to the database we also need to
> re-read all install.rdf files to pick up any new data. The benefits of the
> current solution are that the code ends up being much better tested (we are
> after all using the same code to add new items during a migration that we do
> during a normal install) and it is useful for all migrations, forwards and
> backwards. It is perhaps less efficient but the less one-off code the better I
> think.

Yea, that makes sense. Really love being able to use DBs with future schemas.
Blocks: 553491
Instead of 'Firefox Addon's Profile' maybe 'Addon's Firefox Profile'?
Attached patch patch rev 2 (obsolete) — Splinter Review
This takes on board most of the comments, I'd like to push any further iteration on the UI to follow-up patches so that we can get the bulk of this into nightlies a.s.a.p.

Note I know that the tests of the update date fail on some platforms due to date formatting issues, going to try to figure that out shortly.
Attachment #462851 - Attachment is obsolete: true
Attachment #467262 - Flags: review?(bmcbride)
Oh also this patch is on top of that in bug 585950
Status: NEW → ASSIGNED
Attached patch patch rev 3 (obsolete) — Splinter Review
Updated to apply cleanly over the list view patches and to incorporate the same fixes from there namely showing the control buttons even for pending operations and having a More Info link for blocklist details. Also fixes the date tests.
Attachment #467262 - Attachment is obsolete: true
Attachment #467824 - Flags: review?(bmcbride)
Attachment #467262 - Flags: review?(bmcbride)
Comment on attachment 467824 [details] [diff] [review]
patch rev 3

Random notes:
* Doesn't scroll vertically when the content is too big to fit in the window
* Remote addons don't have an Install button
* Notification is aligned to the name, rather than the far left of the pane. Also, too much space above the notification (but not enough below?)



> <!-- detail view -->
> <!ENTITY detail.version.label "Version">
>-<!ENTITY detail.updated.label "Updated">
>+<!ENTITY detail.updated.label "Last Updated">

Usual warning about changing strings without changing entity names, etc etc.

>+<!ENTITY detail.updateAutomatic "On">
>+<!ENTITY detail.updateManual "Off">

Add tooltips for these.

>-<!ENTITY showMore.label "Show more">
>-<!ENTITY showMore.tooltip "Show more details">
>-<!ENTITY showLess.label "Show less">
>-<!ENTITY showLess.tooltip "Show fewer details">

Should probably have removed these in bug 585950 instead of here.


>+#LOCALIZATION NOTE (details.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
>+details.incompatible=%1$S is incompatible with %2$S %3$S.
>+#LOCALIZATION NOTE (details.blocked) %1$S is the add-on name
>+details.blocked=%1$S has been disabled due to security or stability issues.
>+details.blocked.link=More Information
>+#LOCALIZATION NOTE (details.softblocked) %1$S is the add-on name
>+details.softblocked=%1$S is known to cause security or stability issues.
>+details.softblocked.link=More Information
>+#LOCALIZATION NOTE (details.enable) %1$S is the add-on name, %2$S is brand name
>+details.enable=%1$S will be enabled after you restart %2$S.
>+#LOCALIZATION NOTE (details.disable) %1$S is the add-on name, %2$S is brand name
>+details.disable=%1$S will be disabled after you restart %2$S.
>+#LOCALIZATION NOTE (details.install) %1$S is the add-on name, %2$S is brand name
>+details.install=%1$S will be installed after you restart %2$S.
>+#LOCALIZATION NOTE (details.uninstall) %1$S is the add-on name, %2$S is brand name
>+details.uninstall=%1$S will be uninstalled after you restart %2$S.

Think the entity names here should indicate that these strings are actually notifications.

>+    cmd_contribute: {
>+      isEnabled: function(aAddon) {
>+        return true;
>+        if (!aAddon)
>+          return false;
>+        return ("contributeURL" in aAddon && aAddon.contributeURL);
>+      },
>+      doCommand: function(aAddon) {
>+        openURL(aAddon.contributeURL);
>+      }

If that early return is intentional, add in a todo note. If it isn't intentional, kill it with fire.

>+ }
>+ else {

Style nit: Remove newline after the "}" and before the "else"

>+ // TODO if the add-on was downloaded from releases.mozilla.org link to the
>+ // AMO profile

Are you intending to fix this in this bug? If not, add a bug number to the comment.

>+    var rating = document.getElementById("detail-rating");
>+
>+    if (aAddon.averageRating) {

Nit: Remove the empty line (none of the other similar blocks have an empty line there).

>+    var downloadsRow = document.getElementById("detail-downloads");
>+    if (aAddon.totalDownloads && aIsRemote) {
>+      var downloads = aAddon.totalDownloads;
>+      downloadsRow.value = downloads;
>+    }
>+    else {
>+      downloadsRow.value = null;
>+    }

Does the API give a URL to the download statistics page? Would be cool to be able to link to that.

>+    this._autoUpdate.value = aAddon.applyBackgroundUpdates;

I think this control will need to be disabled/hidden if applyBackgroundUpdates isn't implemented for that addon object (since it is an optional property).

>     AddonManager.getAddonByID(aAddonId, function(aAddon) {
>       self.clearLoading();
> 
>       if (gViewController && aRequest != gViewController.currentViewRequest)
>         return;
> 
>-      if (!aAddon && (aAddonId in gCachedAddons))
>-        aAddon = gCachedAddons[aAddonId];
>+      if (aAddon) {
>+        self._updateView(aAddon, false);
>+        return;
>+      }
> 
>-      self._addon = aAddon;
>-      gEventManager.registerAddonListener(self, aAddon.id);
>+      // Look for an add-on pending install
>+      AddonManager.getAllInstalls(function(aInstalls) {
>+        for (let i = 0; i < aInstalls.length; i++) {
>+          if (aInstalls[i].state == AddonManager.STATE_INSTALLED &&
>+              aInstalls[i].addon.id == aAddonId) {
>+            self._updateView(aInstalls[i].addon, false);
>+            return;
>+          }
>+        }

self.clearLoading() is called too early now (because of the possibility of waiting for AddonManager.getAllInstalls) - move the call to _updateView() ?

>+      else if (!this._addon.isCompatible) {
>+        this.node.setAttribute("notification", "warning");
>+        document.getElementById("detail-warning").textContent = gStrings.ext.formatStringFromName(
>+          "details.incompatible",
>+          [this._addon.name, gStrings.brandShortName, gStrings.appVersion], 3
>+        );
>+        document.getElementById("detail-warning-link").hidden = true;
>+      }
>+      else if (this._addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED) {

Same comment as I made in bug 585950, regarding the priority of incompability vs softblock. When you file the followup bug, be sure to make reference to the detail view too.


>+                <vbox id="detail-contributions">
>+                  <description id="detail-contrib-description">
>+                    &detail.contributions.description;
>+                  </description>
>+                  <hbox align="center">
>+                    <label id="detail-contrib-suggested"/>
>+                    <spacer flex="1"/>
>+                    <button id="detail-contrib-button" class="addon-control"
>+                            label="&cmd.contribute.label;"
>+                            accesskey="&cmd.contribute.accesskey;"
>+                            tooltiptext="&cmd.contribute.tooltip;"
>+                            command="cmd_contribute"/>
>+                  </hbox>
>+                </vbox>

The styling for the contributions button is kinda broken and the contributions box generally needs updated to the styles in newer mockups. I think the button looks broken because its missing -moz-appearance: none; (Note: I'm testing on Windows.)


>+                        <button id="detail-findUpdates" class="button-link"
>+                                label="&detail.checkForUpdates.label;"
>+                                accesskey="&detail.checkForUpdates.accesskey;"
>+                                tooltiptext="&detail.checkForUpdates.tooltip;"
>+                                command="cmd_findItemUpdates"/>

Having an accesskey for text that is underlined makes it look funny. The mockups don't use underlined text (or any accesskeys, for that matter), but the rest of Firefox (and toolkit) does - and consistency generally wins. However, in the examples of underlined links I found in Firefox's Perferences window, none of them have an accesskey (and I think this is the only case in the EM that has an accesskey). So... remove the accesskey?

>-          <spacer flex="1"/>

Think we'd still benefit from having the spacers on each side of the main container - on large screens, there's a *lot* of white space between related parts of the UI - which kinda ruins the feel of it.

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_details.js 

Need to test remote addons here too, since some of the behaviour differs if its not actualy installed.


>+.detail-row,
>+.detail-row-complex {
>+  border-bottom: 1px solid grey;

The last row shouldn't have a border at the bottom.
Attachment #467824 - Flags: review?(bmcbride) → review-
Blocks: 590218
Blocks: 590344
(In reply to comment #28)
> Comment on attachment 467824 [details] [diff] [review]
> patch rev 3
> 
> Random notes:
> * Doesn't scroll vertically when the content is too big to fit in the window

Fixed

> * Remote addons don't have an Install button

Fixed

> * Notification is aligned to the name, rather than the far left of the pane.
> Also, too much space above the notification (but not enough below?)

Tweaked, see how it looks now but it seems like more adjustment is follow-up work.

> > <!-- detail view -->
> > <!ENTITY detail.version.label "Version">
> >-<!ENTITY detail.updated.label "Updated">
> >+<!ENTITY detail.updated.label "Last Updated">
> 
> Usual warning about changing strings without changing entity names, etc etc.

Bleh, could have sworn this didn't exist before.

> >+<!ENTITY detail.updateAutomatic "On">
> >+<!ENTITY detail.updateManual "Off">
> 
> Add tooltips for these.

Done.

> >-<!ENTITY showMore.label "Show more">
> >-<!ENTITY showMore.tooltip "Show more details">
> >-<!ENTITY showLess.label "Show less">
> >-<!ENTITY showLess.tooltip "Show fewer details">
> 
> Should probably have removed these in bug 585950 instead of here.

That's what I get for trying to wrangle so many of these patches at once.
 
> >+#LOCALIZATION NOTE (details.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
> >+details.incompatible=%1$S is incompatible with %2$S %3$S.
> >+#LOCALIZATION NOTE (details.blocked) %1$S is the add-on name
> >+details.blocked=%1$S has been disabled due to security or stability issues.
> >+details.blocked.link=More Information
> >+#LOCALIZATION NOTE (details.softblocked) %1$S is the add-on name
> >+details.softblocked=%1$S is known to cause security or stability issues.
> >+details.softblocked.link=More Information
> >+#LOCALIZATION NOTE (details.enable) %1$S is the add-on name, %2$S is brand name
> >+details.enable=%1$S will be enabled after you restart %2$S.
> >+#LOCALIZATION NOTE (details.disable) %1$S is the add-on name, %2$S is brand name
> >+details.disable=%1$S will be disabled after you restart %2$S.
> >+#LOCALIZATION NOTE (details.install) %1$S is the add-on name, %2$S is brand name
> >+details.install=%1$S will be installed after you restart %2$S.
> >+#LOCALIZATION NOTE (details.uninstall) %1$S is the add-on name, %2$S is brand name
> >+details.uninstall=%1$S will be uninstalled after you restart %2$S.
> 
> Think the entity names here should indicate that these strings are actually
> notifications.

Done

> >+    cmd_contribute: {
> >+      isEnabled: function(aAddon) {
> >+        return true;
> >+        if (!aAddon)
> >+          return false;
> >+        return ("contributeURL" in aAddon && aAddon.contributeURL);
> >+      },
> >+      doCommand: function(aAddon) {
> >+        openURL(aAddon.contributeURL);
> >+      }
> 
> If that early return is intentional, add in a todo note. If it isn't
> intentional, kill it with fire.

Damn testing code.

> 
> >+ }
> >+ else {
> 
> Style nit: Remove newline after the "}" and before the "else"

Done throughout

> >+ // TODO if the add-on was downloaded from releases.mozilla.org link to the
> >+ // AMO profile
> 
> Are you intending to fix this in this bug? If not, add a bug number to the
> comment.

Filed bug 590344, it requires another bug that can't land till Thursday so didn't seem much point in doing the logic yet.

> >+    var rating = document.getElementById("detail-rating");
> >+
> >+    if (aAddon.averageRating) {
> 
> Nit: Remove the empty line (none of the other similar blocks have an empty line
> there).

Done

> >+    var downloadsRow = document.getElementById("detail-downloads");
> >+    if (aAddon.totalDownloads && aIsRemote) {
> >+      var downloads = aAddon.totalDownloads;
> >+      downloadsRow.value = downloads;
> >+    }
> >+    else {
> >+      downloadsRow.value = null;
> >+    }
> 
> Does the API give a URL to the download statistics page? Would be cool to be
> able to link to that.

It doesn't so I think that will be something for the future.

> >+    this._autoUpdate.value = aAddon.applyBackgroundUpdates;
> 
> I think this control will need to be disabled/hidden if applyBackgroundUpdates
> isn't implemented for that addon object (since it is an optional property).

Good point.

> >     AddonManager.getAddonByID(aAddonId, function(aAddon) {
> >       self.clearLoading();
> > 
> >       if (gViewController && aRequest != gViewController.currentViewRequest)
> >         return;
> > 
> >-      if (!aAddon && (aAddonId in gCachedAddons))
> >-        aAddon = gCachedAddons[aAddonId];
> >+      if (aAddon) {
> >+        self._updateView(aAddon, false);
> >+        return;
> >+      }
> > 
> >-      self._addon = aAddon;
> >-      gEventManager.registerAddonListener(self, aAddon.id);
> >+      // Look for an add-on pending install
> >+      AddonManager.getAllInstalls(function(aInstalls) {
> >+        for (let i = 0; i < aInstalls.length; i++) {
> >+          if (aInstalls[i].state == AddonManager.STATE_INSTALLED &&
> >+              aInstalls[i].addon.id == aAddonId) {
> >+            self._updateView(aInstalls[i].addon, false);
> >+            return;
> >+          }
> >+        }
> 
> self.clearLoading() is called too early now (because of the possibility of
> waiting for AddonManager.getAllInstalls) - move the call to _updateView() ?

Done

> >+      else if (!this._addon.isCompatible) {
> >+        this.node.setAttribute("notification", "warning");
> >+        document.getElementById("detail-warning").textContent = gStrings.ext.formatStringFromName(
> >+          "details.incompatible",
> >+          [this._addon.name, gStrings.brandShortName, gStrings.appVersion], 3
> >+        );
> >+        document.getElementById("detail-warning-link").hidden = true;
> >+      }
> >+      else if (this._addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED) {
> 
> Same comment as I made in bug 585950, regarding the priority of incompability
> vs softblock. When you file the followup bug, be sure to make reference to the
> detail view too.

Filed bug 590347.

> >+                <vbox id="detail-contributions">
> >+                  <description id="detail-contrib-description">
> >+                    &detail.contributions.description;
> >+                  </description>
> >+                  <hbox align="center">
> >+                    <label id="detail-contrib-suggested"/>
> >+                    <spacer flex="1"/>
> >+                    <button id="detail-contrib-button" class="addon-control"
> >+                            label="&cmd.contribute.label;"
> >+                            accesskey="&cmd.contribute.accesskey;"
> >+                            tooltiptext="&cmd.contribute.tooltip;"
> >+                            command="cmd_contribute"/>
> >+                  </hbox>
> >+                </vbox>
> 
> The styling for the contributions button is kinda broken and the contributions
> box generally needs updated to the styles in newer mockups. I think the button
> looks broken because its missing -moz-appearance: none; (Note: I'm testing on
> Windows.)

In my defense it almost perfectly matches one of the earlier mockups!

> >+                        <button id="detail-findUpdates" class="button-link"
> >+                                label="&detail.checkForUpdates.label;"
> >+                                accesskey="&detail.checkForUpdates.accesskey;"
> >+                                tooltiptext="&detail.checkForUpdates.tooltip;"
> >+                                command="cmd_findItemUpdates"/>
> 
> Having an accesskey for text that is underlined makes it look funny. The
> mockups don't use underlined text (or any accesskeys, for that matter), but the
> rest of Firefox (and toolkit) does - and consistency generally wins. However,
> in the examples of underlined links I found in Firefox's Perferences window,
> none of them have an accesskey (and I think this is the only case in the EM
> that has an accesskey). So... remove the accesskey?
> 
> >-          <spacer flex="1"/>
> 
> Think we'd still benefit from having the spacers on each side of the main
> container - on large screens, there's a *lot* of white space between related
> parts of the UI - which kinda ruins the feel of it.
> 
> >diff --git a/toolkit/mozapps/extensions/test/browser/browser_details.js 
> 
> Need to test remote addons here too, since some of the behaviour differs if its
> not actualy installed.

Going to need to wait for bug 551274 to land before I can test those properly.

> >+.detail-row,
> >+.detail-row-complex {
> >+  border-bottom: 1px solid grey;
> 
> The last row shouldn't have a border at the bottom.

Do you know of a smart way to do this? At the moment the only thing I can think of is to manually set an attribute from the js on the last visible row.
(In reply to comment #29)
> > >+.detail-row,
> > >+.detail-row-complex {
> > >+  border-bottom: 1px solid grey;
> > 
> > The last row shouldn't have a border at the bottom.
> 
> Do you know of a smart way to do this? At the moment the only thing I can think
> of is to manually set an attribute from the js on the last visible row.

Hm, I thought there was, using :last-child or :last-of-type - but it seems you can't be specific when using those selectors. So AFAIK, it looks like setting an attribute in JS is the only way (which really surprises me).
Are there any plans to improve navigation within the add-ons manager?  I have 62 enabled extensions (another 63 that are disabled), and this add-ons manager doesn't handle navigation well at all.

For example, scroll position is not remembered and there is no "back" navigation; thus, if I click "more" (to get to the detail view) for any extension, the only way to get back (since there is no back button -- and there should be) is to click on the main "Extensions" tab, which takes me back to the top and forces me to manually scroll back to the place I last was.  Imagine doing this several times in a row, given the size of my extensions list.  This really really sucks.

If I should be filing a bug report, I can definitely do that, but I am reluctant to waste time filing another feature-related bug report, for this add-ons manager, only to have it marked wontfix.  But of course I have time to rant. ;-)

I've looked at the mockups and existing bugs and I see no indication that this is even remotely a consideration -- unless I've missed something.

It would be great if this could be addressed, as this is one of the worst aspects of this new add-on manager.

Thanks
(In reply to comment #31)
> For example, scroll position is not remembered and there is no "back"
> navigation; thus, if I click "more" (to get to the detail view) for any

For remembering the scroll position see bug 586956.
(In reply to comment #31)
> Are there any plans to improve navigation within the add-ons manager?  I have
> 62 enabled extensions (another 63 that are disabled), and this add-ons manager
> doesn't handle navigation well at all.
> 
> For example, scroll position is not remembered and there is no "back"
> navigation; thus, if I click "more" (to get to the detail view) for any
> extension, the only way to get back (since there is no back button -- and there
> should be) is to click on the main "Extensions" tab, which takes me back to the
> top and forces me to manually scroll back to the place I last was.  Imagine
> doing this several times in a row, given the size of my extensions list.  This
> really really sucks.
> 
> If I should be filing a bug report, I can definitely do that, but I am
> reluctant to waste time filing another feature-related bug report, for this
> add-ons manager, only to have it marked wontfix.  But of course I have time to
> rant. ;-)
> 
> I've looked at the mockups and existing bugs and I see no indication that this
> is even remotely a consideration -- unless I've missed something.
> 
> It would be great if this could be addressed, as this is one of the worst
> aspects of this new add-on manager.
> 
> Thanks

If I am not mistaken, there is a Back to Extension link on the top left corner of Add-on Manager
(In reply to comment #33)
> If I am not mistaken, there is a Back to Extension link on the top left corner
> of Add-on Manager

Ugh.  Thanks.  Hard to notice, as positioning is bad -- should be above the detailed view box where a person's eyes will be focused.

Am I sounding like a nag yet?

(In reply to comment #32)
> For remembering the scroll position see bug 586956.

Thanks for the ref.
After bug 571970 lands, there will be back/forward buttons, as they are on mockups.
Attached patch patch rev 4 (obsolete) — Splinter Review
(In reply to comment #29)
> (In reply to comment #28)
> > Comment on attachment 467824 [details] [diff] [review] [details]
> > patch rev 3
> > 
> > Random notes:
> > * Doesn't scroll vertically when the content is too big to fit in the window
> 
> Fixed

Correction not fixed. This is something that I don't think I can fix by tomorrow since I seem to be seeing some kind of XUL widget bug. I know there are issues with wrapping descriptions breaking scrolling calculations and I think this is happening here (see bug 413336)

> > * Remote addons don't have an Install button
> 
> Fixed

Correction, not fixed. This may be related to the above but while I can make the install button appear it isn't clickable.

My thought right now is that this is landable with blocking bugs filed on those  things. If you don't agree we'll just have to look at landing the strings as-is.

Note in this patch I tweaked the setCreator code to support the new way creatorURLs are passed.
Attachment #467824 - Attachment is obsolete: true
Attachment #469681 - Flags: review?(bmcbride)
(In reply to comment #36)
> > > * Doesn't scroll vertically when the content is too big to fit in the window
> > 
> > Fixed
> 
> Correction not fixed. This is something that I don't think I can fix by
> tomorrow since I seem to be seeing some kind of XUL widget bug. I know there
> are issues with wrapping descriptions breaking scrolling calculations and I
> think this is happening here (see bug 413336)

Hm, yea, I remember hitting something like that awhile ago too. And the brokenness from that seems a lot worse than not having any scrolling at all.

> Note in this patch I tweaked the setCreator code to support the new way
> creatorURLs are passed.

This is broken - renamed the first variable, but usage of them didn't get updated. Or something.

I'm fine with landing this without the install button, but the brokenness with the scrolling and setCreator is a showstopper. Think we'll need to just revert to not having any scrolling for the time being, and see if we can find a workaround in a followup bug (assuming bug 413336 can't be fixed in time).

Wasn't able to get a more thorough look at this today (or get much of anything done, for that matter). Will try to get through it in the weekend.
We might be able to rush this into b5 but scrolling and the install button would be open so I think it's better to take a little time and land it early in b6 instead.
blocking2.0: beta5+ → beta6+
Attached patch patch rev 5 (obsolete) — Splinter Review
This fixes the setCreator stuff and now applies on top of bug 562797 which I hope to land imminently. I'm going to try to resolve the scrolling and install button before landing and attach a follow-up patch.
Attachment #469681 - Attachment is obsolete: true
Attachment #470058 - Flags: review?(bmcbride)
Attachment #469681 - Flags: review?(bmcbride)
Attached patch patch rev 5 (obsolete) — Splinter Review
de-bitrotted
Attachment #470058 - Attachment is obsolete: true
Attachment #470563 - Flags: review?(bmcbride)
Attachment #470058 - Flags: review?(bmcbride)
Attached patch patch rev 6Splinter Review
This patch fixes the scrollbar issues and the install button. XUL layout is weird, Enn is a genius. The description needs a transparent outline (do not ask me why!)
Attachment #470563 - Attachment is obsolete: true
Attachment #470875 - Flags: review?(bmcbride)
Attachment #470563 - Flags: review?(bmcbride)
Attached image Scrolling screenshot
(In reply to comment #41)
> This patch fixes the scrollbar issues and the install button. XUL layout is
> weird, Enn is a genius. The description needs a transparent outline (do not ask
> me why!)

Certainly magnitudes better! Not quite perfect though - I have a couple of addons where the scrolling region doesn't fill its container. See the attached screenshot.

I also had the same thing happen for Firebug (which is disabled and incompatible) - until it loaded the screenshot, then it fixed itself. And another theme - when the screenshot loaded, it *almost* fixed itself (the scrolling region expanded, but not enough to fill its container). The common factor seems to be if the height of the description is greater then the height of the screenshot box.
Comment on attachment 470875 [details] [diff] [review]
patch rev 6

Addons pending install still show the "(disabled)" postfix.

>+#LOCALIZATION NOTE (details.notification.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
>+details.notification.incompatible=%1$S is incompatible with %2$S %3$S.
>+#LOCALIZATION NOTE (details.notification.blocked) %1$S is the add-on name
>+details.notification.blocked=%1$S has been disabled due to security or stability issues.
>+details.notification.blocked.link=More Information
>+#LOCALIZATION NOTE (details.notification.softblocked) %1$S is the add-on name
>+details.notification.softblocked=%1$S is known to cause security or stability issues.
>+details.notification.softblocked.link=More Information
>+#LOCALIZATION NOTE (details.notification.enable) %1$S is the add-on name, %2$S is brand name
>+details.notification.enable=%1$S will be enabled after you restart %2$S.
>+#LOCALIZATION NOTE (details.notification.disable) %1$S is the add-on name, %2$S is brand name
>+details.notification.disable=%1$S will be disabled after you restart %2$S.
>+#LOCALIZATION NOTE (details.notification.install) %1$S is the add-on name, %2$S is brand name
>+details.notification.install=%1$S will be installed after you restart %2$S.
>+#LOCALIZATION NOTE (details.notification.uninstall) %1$S is the add-on name, %2$S is brand name
>+details.notification.uninstall=%1$S will be uninstalled after you restart %2$S.

Missing the update notification, which ends up breaking everything when it tries to show an addon thats pending upgrade.

>+    var screenshot = document.getElementById("detail-screenshot");
>+    if (aAddon.screenshots && aAddon.screenshots.length > 0) {
>+      screenshot.src = aAddon.screenshots[0].url;
>+      screenshot.hidden = false;
>+    } else {
>+      screenshot.hidden = true;
>+    }

Should be using the thumbnail URL here. A large screenshot can potentially take up the whole screen.

Do you know if the API guarantees a thumbnail for every screenshot? If not, and it gets a screenshot without a thumbnail, we may want to fallback to scaling the full screenshot down to thumbnail size (future work, if anything).

>+    this._autoUpdate.parentNode.parentNode.hidden = !canUpdate;

I had to look at the XUL to find out what this was hiding - could you make it use getElementById to get the row, instead.

>+
>+    if ("applyBackgroundUpdates" in aAddon) {
>+      this._autoUpdate.value = aAddon.applyBackgroundUpdates;
>+      document.getElementById("detail-findUpdates").hidden = this._addon.applyBackgroundUpdates;
>+    } else {
>+      this._autoUpdate.hidden = true;
>+      document.getElementById("detail-findUpdates").hidden = false;
>+    }

You need to unhide this._autoUpdate in the first case, otherwise it stays hidden permanently for all addons.

>+#detail-desc-container {
>+  margin-bottom: 2em;
>+  /* This is necessary to fix layout issues with multi-line descriptions */
>+  outline: solid transparent;
> }

Is there a bug this comment can reference?

>+.detail-row,
>+.detail-row-complex {
>+  border-bottom: 1px solid grey;

File a follwup bug to make it so the last visible row doesn't have a line underneath it (that line really annoys me, for some reason).

r=me, with those fixes.
Attachment #470875 - Flags: review?(bmcbride) → review+
Whiteboard: [AddonsRewriteTestday][rewrite] → [AddonsRewriteTestday][rewrite][strings]
(In reply to comment #42)
> Created attachment 471014 [details]
> Scrolling screenshot
> 
> (In reply to comment #41)
> > This patch fixes the scrollbar issues and the install button. XUL layout is
> > weird, Enn is a genius. The description needs a transparent outline (do not ask
> > me why!)
> 
> Certainly magnitudes better! Not quite perfect though - I have a couple of
> addons where the scrolling region doesn't fill its container. See the attached
> screenshot.
> 
> I also had the same thing happen for Firebug (which is disabled and
> incompatible) - until it loaded the screenshot, then it fixed itself. And
> another theme - when the screenshot loaded, it *almost* fixed itself (the
> scrolling region expanded, but not enough to fill its container). The common
> factor seems to be if the height of the description is greater then the height
> of the screenshot box.

Let's take this to a follow-up. It seems to be usable now and there is a lot of change here that we need to get into nightlies a.s.a.p. Filed bug 592705
(In reply to comment #43)
> >+    var screenshot = document.getElementById("detail-screenshot");
> >+    if (aAddon.screenshots && aAddon.screenshots.length > 0) {
> >+      screenshot.src = aAddon.screenshots[0].url;
> >+      screenshot.hidden = false;
> >+    } else {
> >+      screenshot.hidden = true;
> >+    }
> 
> Should be using the thumbnail URL here. A large screenshot can potentially take
> up the whole screen.
> 
> Do you know if the API guarantees a thumbnail for every screenshot? If not, and
> it gets a screenshot without a thumbnail, we may want to fallback to scaling
> the full screenshot down to thumbnail size (future work, if anything).

I've gone with using the thumbnail if available and falling back to the full size if not. I think you're right though we need to implement some kind of scaling in extreme cases.

> >+#detail-desc-container {
> >+  margin-bottom: 2em;
> >+  /* This is necessary to fix layout issues with multi-line descriptions */
> >+  outline: solid transparent;
> > }
> 
> Is there a bug this comment can reference?

Filed bug 592712

> >+.detail-row,
> >+.detail-row-complex {
> >+  border-bottom: 1px solid grey;
> 
> File a follwup bug to make it so the last visible row doesn't have a line
> underneath it (that line really annoys me, for some reason).

Bah, I was bound to forget something, filed bug 592708
Landed
http://hg.mozilla.org/mozilla-central/rev/c89c3dab1eb5
http://hg.mozilla.org/mozilla-central/rev/ea01bf6029f1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Flags: in-testsuite+
Flags: in-litmus-
Comment on attachment 470875 [details] [diff] [review]
patch rev 6

>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>+<!ENTITY detail.updateManual.tooltip          "Don't automaticall install updates">
automaticall?
Attached patch locale fixSplinter Review
(In reply to comment #47)
> Comment on attachment 470875 [details] [diff] [review]
> patch rev 6
> 
> >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
> >+<!ENTITY detail.updateManual.tooltip          "Don't automaticall install updates">
> automaticall?

*facepalm*
(In reply to comment #48)
> Created attachment 471208 [details] [diff] [review]
> locale fix
> 
> (In reply to comment #47)
> > Comment on attachment 470875 [details] [diff] [review] [details]
> > patch rev 6
> > 
> > >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
> > >+<!ENTITY detail.updateManual.tooltip          "Don't automaticall install updates">
> > automaticall?
> 
> *facepalm*

Vlad pushing this quick fix as http://hg.mozilla.org/mozilla-central/rev/4fb704bd0772
Comment on attachment 470875 [details] [diff] [review]
patch rev 6

>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>+#LOCALIZATION NOTE (details.notification.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
>+details.notification.incompatible=%1$S is incompatible with %2$S %3$S.
>+#LOCALIZATION NOTE (details.notification.blocked) %1$S is the add-on name
>+details.notification.blocked=%1$S has been disabled due to security or stability issues.
>+details.notification.blocked.link=More Information
>+#LOCALIZATION NOTE (details.notification.softblocked) %1$S is the add-on name
>+details.notification.softblocked=%1$S is known to cause security or stability issues.
>+details.notification.softblocked.link=More Information
>+#LOCALIZATION NOTE (details.notification.enable) %1$S is the add-on name, %2$S is brand name
>+details.notification.enable=%1$S will be enabled after you restart %2$S.
>+#LOCALIZATION NOTE (details.notification.disable) %1$S is the add-on name, %2$S is brand name
>+details.notification.disable=%1$S will be disabled after you restart %2$S.
>+#LOCALIZATION NOTE (details.notification.install) %1$S is the add-on name, %2$S is brand name
>+details.notification.install=%1$S will be installed after you restart %2$S.
>+#LOCALIZATION NOTE (details.notification.uninstall) %1$S is the add-on name, %2$S is brand name
>+details.notification.uninstall=%1$S will be uninstalled after you restart %2$S.

how are these different from the notification.* set in the same file? I see no difference, so unless there is a strong reason to duplicate those it's just excessive workload on localizers.
(In reply to comment #50)
> Comment on attachment 470875 [details] [diff] [review]
> patch rev 6
> 
> >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
> >+#LOCALIZATION NOTE (details.notification.incompatible) %1$S is the add-on name, %2$S is brand name, %3$S is application version
> >+details.notification.incompatible=%1$S is incompatible with %2$S %3$S.
> >+#LOCALIZATION NOTE (details.notification.blocked) %1$S is the add-on name
> >+details.notification.blocked=%1$S has been disabled due to security or stability issues.
> >+details.notification.blocked.link=More Information
> >+#LOCALIZATION NOTE (details.notification.softblocked) %1$S is the add-on name
> >+details.notification.softblocked=%1$S is known to cause security or stability issues.
> >+details.notification.softblocked.link=More Information
> >+#LOCALIZATION NOTE (details.notification.enable) %1$S is the add-on name, %2$S is brand name
> >+details.notification.enable=%1$S will be enabled after you restart %2$S.
> >+#LOCALIZATION NOTE (details.notification.disable) %1$S is the add-on name, %2$S is brand name
> >+details.notification.disable=%1$S will be disabled after you restart %2$S.
> >+#LOCALIZATION NOTE (details.notification.install) %1$S is the add-on name, %2$S is brand name
> >+details.notification.install=%1$S will be installed after you restart %2$S.
> >+#LOCALIZATION NOTE (details.notification.uninstall) %1$S is the add-on name, %2$S is brand name
> >+details.notification.uninstall=%1$S will be uninstalled after you restart %2$S.
> 
> how are these different from the notification.* set in the same file? I see no
> difference, so unless there is a strong reason to duplicate those it's just
> excessive workload on localizers.

I was under the impression that we are supposed to use different strings when the string is used in different contexts
The strings are really straighforward, both versions are notifications (at least the entity names say so), so I'm not sure how they can be used in different contexts. Could you enlighten me a bit?

CCing Pike and l10n community as well
(In reply to comment #52)
> The strings are really straighforward, both versions are notifications (at
> least the entity names say so), so I'm not sure how they can be used in
> different contexts. Could you enlighten me a bit?
> 
> CCing Pike and l10n community as well

One is included at the top of the list items in the list vew, the other is shown at the top of the details view for the add-on.
FWIW, I've glanced at the patch earlier today, and let it pass as it looked like the two strings could face different space constraints. Couldn't tell exactly from glancing at things, so I opted for rather safe than sorry.
(In reply to comment #53)
> One is included at the top of the list items in the list vew, the other is
> shown at the top of the details view for the add-on.
But that's still a single context - it's a notification for particular add-on.
Doesn't matter if there's just one add-on or a list of add-ons, the string
always applies to one particular add-on, right? So there's no need to have
different strings for it.

(In reply to comment #54)
> FWIW, I've glanced at the patch earlier today, and let it pass as it looked
> like the two strings could face different space constraints. Couldn't tell
> exactly from glancing at things, so I opted for rather safe than sorry.

I think the space constraints aren't any different, both strings are shown on the top of the respective views.
(In reply to comment #55)
> (In reply to comment #53)
> > One is included at the top of the list items in the list vew, the other is
> > shown at the top of the details view for the add-on.
> But that's still a single context - it's a notification for particular add-on.
> Doesn't matter if there's just one add-on or a list of add-ons, the string
> always applies to one particular add-on, right? So there's no need to have
> different strings for it.
> 
> (In reply to comment #54)
> > FWIW, I've glanced at the patch earlier today, and let it pass as it looked
> > like the two strings could face different space constraints. Couldn't tell
> > exactly from glancing at things, so I opted for rather safe than sorry.
> 
> I think the space constraints aren't any different, both strings are shown on
> the top of the respective views.

They are shown at different font sizes and with differing margins and available area. I don't know whether that is worth different strings or not, especially since they should be line wrapping when getting too long. If we want to it would be easy to just use the same strings everywhere.
Depends on: 592947
Blocks: 562937
No longer depends on: 562937
Blocks: 578086
No longer depends on: 578086
Depends on: 593152
No longer depends on: 592947
Blocks: 562241
Marking as verified fixed with Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100922 Firefox/4.0b7pre.

Any remaining follow-up issues are tracked on the dependency list.
Status: RESOLVED → VERIFIED
Depends on: 595317
This still isn't the right colour on Windows 7. Which isn't either in the dependency list nor the block list.
(In reply to comment #58)
> This still isn't the right colour on Windows 7. Which isn't either in the
> dependency list nor the block list.

Should be covered by bug 590218
No longer blocks: 623199
You need to log in before you can comment on or make changes to this bug.