562890 - Preferences button for the add-on should be placed in the entry of the digest view

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre ID:20100425040736
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre ID:20100425040736

Preferences of the extension is at the deep hierarchy.it  should be in a list view.
I think Author/Rating/Date are not necessary in list view if space problem.

Comment 1

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

Alice, are you talking about the details view? Mockups are up but probably need a revisit. Some items have been changed meanwhile.

Comment 2

[Alice0775 White]

Preferences button of the extension  should be placed in Digest View

Comment 3

[Legewiel]

I Agree it would help a lot for the newbies a preference button at the left side of each add-on in the add-ons manager window.

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

Yes, this is fine for now.  I'll attach a mockup in a few

Comment 5

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Preferences button in content of digest view

Comment 6

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

Will this preference button be aligned to the Remove and Disable buttons or at a fixed position? Aligning to the other buttons wouldn't be a good idea because the Remove button could be hidden (for globally installed addons) and causes the Preference button to move right.

Also if we go on with the idea on bug 562776 to not show buttons for list entries which are not selected, the Preference button should also only be visible for the currently selected entry.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #6)
> Will this preference button be aligned to the Remove and Disable buttons or at
> a fixed position? Aligning to the other buttons wouldn't be a good idea because
> the Remove button could be hidden (for globally installed addons) and causes
> the Preference button to move right.

For add-ons that cannot be removed, the remove button should be disabled and greyed out rather than gone.  So, the spacing of the buttons should not change.

> Also if we go on with the idea on bug 562776 to not show buttons for list
> entries which are not selected, the Preference button should also only be
> visible for the currently selected entry.

This is unlikely to happen - I think we're safe to go ahead and implement Preferences to the left of disable and remove

Comment 8

[IU]

What are the chances of you swapping the position of the "remove" and "disable" buttons?  "Remove" is an action of last resort.  A person is more likely to disable, so I believe "remove" should be furthest from other buttons -- not in the middle of them.  Thanks.

Comment 9

[Tony Mechelynck [:tonymec]]

(In reply to comment #8)
> What are the chances of you swapping the position of the "remove" and "disable"
> buttons?  "Remove" is an action of last resort.  A person is more likely to
> disable, so I believe "remove" should be furthest from other buttons -- not in
> the middle of them.  Thanks.

What about one row of buttons at the bottom of the entry, maybe something like

[More info][Preferences]|...elastic space...|[Enable/Disable][Uninstall]

(either the selected entry as before the rewrite, or every entry)? This would make the buttons look less "all cramped in a small space at right". This proposal would also put "Uninstall" at the end of the row, i.e., farthest from "the middle of them". (Whether to call it "Remove" or "Uninstall", or the one in some themes and the other in others, is a "novelty vs. tradition" question, IMHO of only cosmetic importance. On SeaMonkey which comes with two themes packed-in, I imagine that the "default" theme, which looks more like Firefox, could call it "Remove", while the "Modern" theme, which IMHO looks more like NS/MozSuite, and thus is actually of more "traditional" look-and-feel, could call it "Uninstall". But that's only my imagination running wild.)

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Note that I'm passing in the addon object to doCommand(), since consumers might call that without the addon item being selected. Should have allowed doing that in the first place.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 448677 [details] [diff] [review]
Patch v1

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd

> <!ENTITY detail.updateAutomatically.label     "Update automatically">
> <!ENTITY detail.updateAutomatically.accesskey "U">
> <!ENTITY detail.updateAutomatically.tooltip   "Update this add-on automatically whenever there is an update available">
> <!ENTITY detail.checkForUpdates.label         "Check for updates">
> <!ENTITY detail.checkForUpdates.accesskey     "F">
> <!ENTITY detail.checkForUpdates.tooltip       "Check for updates for this add-on">
>-<!ENTITY detail.showPreferences.label         "Preferences">
>-<!ENTITY detail.showPreferences.accesskey     "P">
>-<!ENTITY detail.showPreferences.tooltip       "Change this add-on's preferences">

Double check this with Pike. We may have to keep both strings around as we are showing the button in two different contexts.

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml

>       <method name="_updateState">
>         <body><![CDATA[
>           this.setAttribute("incompatible", !this.mAddon.isCompatible);
> 
>+          this._preferencesBtn.hidden = !(this.mAddon.isActive &&
>+                                          this.mAddon.optionsURL);

The API should be returning null for optionsURL on disabled add-ons so I think rely on that, on the off chance we add an add-on type that supports settings while disabled.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

Finally got around to fixing this up and adding a test.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 452624 [details] [diff] [review]
Patch v1.1

I think you need to add the access key to the XUL.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1.1 (ready for checkin)

After discussion on IRC: the suggestion in comment 13 would cause bug 562776 (lets stop using "digest view" - its confusing). However, there was an unneeded accesskey entity in extensions.dtd - this updated patch removes that. Not landing yet, as the tree looks scary right now.

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/96ad49c97c4f

Comment 16

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

Verified fixed with trunk builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre

Looks like a good automated test which doesn't need any Mozmill or manual test.

Comment 17

[Alon Zakai (:azakai)]

Running mochitest-browser-chrome on Ubuntu 10.04, on latest mozilla-central, debug build, I get a failure for the test for this bug,

 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Timed out
 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Found an unexpected tab at the end of test run: about:addons

However it appears to be running ok on tinderbox.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #17)
> Running mochitest-browser-chrome on Ubuntu 10.04, on latest mozilla-central,
> debug build, I get a failure for the test for this bug,

Hmm, its constantly failing? The first failure would cause the second failure. Any idea where its timing out?

Comment 19

[Alon Zakai (:azakai)]

It constantly fails, yes.

Is there a straightforward way to see where it times out? Here is the full test output, maybe that can help:

TEST-START | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
pldhash: for the table at address 0x4524c6b8, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0x4524c650 == 8
++DOMWINDOW == 14 (0x46e10d38) [serial = 14] [outer = (nil)]
++DOMWINDOW == 15 (0x46e10eb8) [serial = 15] [outer = 0x46e10d00]
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
++DOMWINDOW == 16 (0x482834b8) [serial = 16] [outer = 0x46e10d00]
pldhash: for the table at address 0x4524cc88, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0x4524cc20 == 9
++DOMWINDOW == 17 (0x48283ab8) [serial = 17] [outer = (nil)]
WARNING: NS_ENSURE_TRUE(chromeWindow) failed: file /scratchbox/users/alon/home/alon/mozilla-central/content/base/src/nsFrameLoader.cpp, line 1905
WARNING: NS_ENSURE_TRUE(mutableURL) failed: file /scratchbox/users/alon/home/alon/mozilla-central/content/xul/templates/src/nsXULContentUtils.cpp, line 338
++DOMWINDOW == 18 (0x482849b8) [serial = 18] [outer = 0x48283a80]
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Should have an add-ons manager window
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Should be displaying the correct UI
LoadPlugin() /scratchbox/users/alon/home/alon/m-c-ff/modules/plugin/test/testplugin/libnptest.so returned 48ee2ae0
GetMIMEDescription() returned "application/x-test:tst:Test mimetype"
LoadPlugin() /usr/lib/mozilla/plugins/librhythmbox-itms-detection-plugin.so returned 493112c0
GetMIMEDescription() returned "application/itunes-plugin::;"
LoadPlugin() /usr/lib/flashplugin-installer/libflashplayer.so returned 493113a0
GetMIMEDescription() returned "application/x-shockwave-flash:swf:Shockwave Flash;application/futuresplash:spl:FutureSplash Player"
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Prefs button should be hidden for addon with no optionsURL set
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Prefs button should be shown for addon with a optionsURL set
*** loading ISO8601DateUtils
--DOMWINDOW == 17 (0x46e0e7b8) [serial = 10] [outer = 0x46e0d100] [url = about:blank]
--DOMWINDOW == 16 (0x46e10eb8) [serial = 15] [outer = 0x46e10d00] [url = about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Found a tab after previous test timed out: about:addons
WARNING: NS_ENSURE_TRUE(bCollapsed) failed: file /scratchbox/users/alon/home/alon/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp, line 1009
TEST-INFO | checking window state
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | before wait for focus -- loaded: complete active window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank desired window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank docshell visible: true
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | already focused
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank desired window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank docshell visible: true
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584