Provide a simple way for addons to have preferences UI

VERIFIED FIXED in mozilla7

Status

()

Toolkit
Add-ons Manager
--
enhancement
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

(Depends on: 5 bugs, {dev-doc-complete})

unspecified
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(13 attachments, 29 obsolete attachments)

514.59 KB, image/png
Details
501.26 KB, image/png
Details
65.12 KB, image/jpeg
Boriss
: ui-review+
Details
39.49 KB, image/jpeg
Details
3.61 KB, patch
mossop
: review+
Details | Diff | Splinter Review
9.20 KB, patch
mossop
: review+
Details | Diff | Splinter Review
25.54 KB, patch
Details | Diff | Splinter Review
23.83 KB, patch
Details | Diff | Splinter Review
19.93 KB, patch
Details | Diff | Splinter Review
2.59 KB, patch
Details | Diff | Splinter Review
1.92 KB, patch
Details | Diff | Splinter Review
185.93 KB, image/jpeg
Details
1.41 KB, application/x-xpinstall
Details
(Assignee)

Description

6 years ago
Created attachment 529020 [details] [diff] [review]
WIP patch

I've been tinkering with this for too long for it not to have a bug.

tl;dr: I want to do this in the desktop apps: https://wiki.mozilla.org/Mobile/Fennec/Extensions/Options#Options_XUL

What I want is in-addons-manager UI for addon prefs. This would be useful for both bootstrapped add-ons (it doesn't require chrome), and addons where having an options window is overkill.

So far I've copied the code from mobile and adjusted it to work inside the addon manager detail view. It lifts the settings from the file options.xul in the addon's folder (same format as on the wikimo link above). I've only done the CSS on winstripe so far.

Thoughts?
(Assignee)

Comment 1

6 years ago
Created attachment 529022 [details]
screenshot

... and a screenshot for good measure. Note the options.xul file used here isn't quite the same as the example on wikimo.
We already thought about having a pane with some but not all available preferences in the details pane. There is already a bug about it, which I can't find at the moment. Dave, do you have it handy?
(Assignee)

Comment 3

6 years ago
That really doesn't surprise me. I couldn't find the relevant bug either.
At least I can give an early mockup from the Firefox 4 dev cycle, which shows such a preferences section in the details pane:

https://wiki.mozilla.org/Firefox/Projects/Extension_Manager_Redesign/design#Detail_View
Don't think we had a bug yet, but it is a wanted feature. This is pretty awesome. The code looks pretty good at first glance but I'd like to get a few people's thoguhts on it first.

I'd also like mfinkle's take one whether we should be exactly matching what Fennec does here, even down to the file used. Do we have a risk of add-ons wanting to work in both Firefox and Fennec and wanting slightly different list of add-ons there? Also obviously let's get Boriss's take on the UI.

I also wonder if it would be good to have one shared settings.xml and make the differences with CSS only so we don't have to start fixing things in both mobile and toolkit as we make changes going forwards.
Whiteboard: uiwanted
Fennec uses the XUL file pointed too in the <em:optionURL> in install.rdf, not a hardcoded file. Fennec only allows inline options, so we don't have any ambiguity or compat issues yet. We could add a flag to install.rdf about loading options inline rather than as a dialog to help any compat problems in desktop.

One area that a hardcoded fallback might be handy is for bootstrap addons. We could look for "options.xul" or something in that case.

It would be nice to try to use the same setting.xml XBL bindings in both desktop and mobile. We could try to use CSS to style away the differences.

As for add-ons wanting to use slightly different lists in desktop and mobile, we can use app flags in the manifest to pick the proper XUL file, which is what mobile add-ons devs need to do now since Fennec uses the optionsURL.
(Assignee)

Comment 7

6 years ago
Created attachment 529627 [details] [diff] [review]
WIP patch

(In reply to comment #5)
> I also wonder if it would be good to have one shared settings.xml and make the
> differences with CSS only so we don't have to start fixing things in both
> mobile and toolkit as we make changes going forwards.

Done. I've moved the file to toolkit, since chrome://mozapps/content/extensions/ appears to be available in Fennec (which seems a bit odd, never mind).

(In reply to comment #6)
> Fennec uses the XUL file pointed too in the <em:optionURL> in install.rdf, not
> a hardcoded file. Fennec only allows inline options, so we don't have any
> ambiguity or compat issues yet. We could add a flag to install.rdf about
> loading options inline rather than as a dialog to help any compat problems in
> desktop.

Done. I've added em:optionsType (better name?) which should be set to "inline" if using an optionsURL. Uses options.xul if there is one. I'm using "inline" instead of true/false, so we can easily implement options-window-in-a-tab later.
Attachment #529020 - Attachment is obsolete: true
Comment on attachment 529627 [details] [diff] [review]
WIP patch

>-  ["optionsURL", "aboutURL"].forEach(function(aProp) {
>+  ["optionsURL", "optionsType", "aboutURL"].forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() {
>       return this.isActive ? aAddon[aProp] : null;
>     });
>   }, this);

Could you do the "options.xul" fixup here like we do for iconURL getting mapped to icon.png here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#6614

that would make this work for bootstrapped add-ons and you shouldn't need to do the extension.js below. optionsURL would just work.

>+    var optionsURL;
>+    if (aAddon.hasResource("options.xul")) {
>+      optionsURL = aAddon.getResourceURI("options.xul").spec;
>+    } else if (!!aAddon.optionsURL && aAddon.optionsType == "inline") {
>+      optionsURL = aAddon.optionsURL;
>+    }

I need to make a build with this patch to make sure the /mobile changes are OK (unless you already did)
(Assignee)

Comment 9

6 years ago
(In reply to comment #8)
> Could you do the "options.xul" fixup here like we do for iconURL getting mapped to icon.png here:

I could but it would require the inline flag in install.rdf, I guess that's not really an issue though.

> I need to make a build with this patch to make sure the /mobile changes are OK
> (unless you already did)

I did.
(Assignee)

Comment 10

6 years ago
Created attachment 529952 [details] [diff] [review]
WIP patch

I've fixed a few minor things and done the CSS for pinstripe and gnomestripe.
Attachment #529627 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 529953 [details] [diff] [review]
WIP tests
Comment on attachment 529952 [details] [diff] [review]
WIP patch


>-  ["optionsURL", "aboutURL"].forEach(function(aProp) {
>+  ["optionsURL", "optionsType", "aboutURL"].forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() {
>       return this.isActive ? aAddon[aProp] : null;
>     });
>   }, this);

I could be wrong, but I think you want to remove "optionsURL" and "optionsType" from this getter since you handle those in special getters below:

>   // Maps iconURL and icon64URL to the properties of the same name or icon.png
>   // and icon64.png in the add-on's files.

Add a similar comment for optionsURL

>-  ["icon", "icon64"].forEach(function(aProp) {
>+  ["icon", "icon64", "options"].forEach(function(aProp) {

>+        case "options":
>+          if (this.hasResource(aProp + ".xul"))
>+            return this.getResourceURI(aProp + ".xul").spec;
>+          break;
>+      }

This getter handles optionsURL / options

>+  this.__defineGetter__("optionsType", function() {
>+    if (this.isActive && aAddon["optionsType"])
>+      return aAddon["optionsType"];
>+
>+    if (this.hasResource("options.xul"))
>+      return "inline";

This getter handles optionType
(Assignee)

Comment 13

6 years ago
Created attachment 531211 [details] [diff] [review]
patch

Fixes comment 12, also hides the settings for disabled addons.
Attachment #529952 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 531213 [details] [diff] [review]
tests
Attachment #529953 - Attachment is obsolete: true
Created attachment 531482 [details]
Mockup: One-line preferences in detail view vs embedding previously written preferences

Thanks so much Geoff for working on this very important bit of missing functionality!  It really improves add-on usage to be able to make changes to add-on prefs in the detailed add-ons view.

There are two parts to the problem of adding add-on preferences to the detailed view of the add-ons manager:

1. Allowing add-on authors an easy way to write a one-line, easily integrated preferences for their add-on
2. Allowing previously written preferences for add-ons to appear in the detail view

Geoff's patch seems to cover the first and far more important case, where a subsequent bug (or possibly tweak on Geoff's patch) may cover the second.  If the first is an easy win through a patch and the second happens slowly - or possibly never - it's still an awesome improvement.  Still, I wanted to mention the second in case it's an easy addition. 

Still another phase of this might be building in easy tools that allow add-on authors to create more complex preferences within detail view that take up more than one line, but that's a long way away.
(Assignee)

Comment 16

6 years ago
Created attachment 531487 [details]
screenshot

This screenshot shows a couple of things the previous didn't:
* The longer description label allowed for in mobile. Unfortunately it messes with the 1:2 flex of the grid and it doesn't have any styling of its own yet. I've changed the font size and colour with the DOM Inspector to give an idea of what I'd do if it were up to me.
* The button control. Should we style it to match the addon manager buttons?
Attachment #531487 - Flags: ui-review?(jboriss)
(In reply to comment #15)
> There are two parts to the problem of adding add-on preferences to the
> detailed view of the add-ons manager:
> 
> 1. Allowing add-on authors an easy way to write a one-line, easily
> integrated preferences for their add-on
> 2. Allowing previously written preferences for add-ons to appear in the
> detail view

I don't think we ever want to do the second part of this to be honest. There is just nothing standard about the custom UI that developers have created, it would probably end up looking bad in almost all cases.
Especially for more complex preference windows with tabs. So I agree with Dave. For option 1 would be nice to have a better visually distinction between default settings from the Add-ons Manager and preferences brought with the add-on. I like the box with the different background, which really makes it clear.
(Assignee)

Comment 19

6 years ago
I agree too.

(In reply to comment #18)
> For option 1 would be nice to have a better visually distinction
> between default settings from the Add-ons Manager and preferences brought
> with the add-on. I like the box with the different background, which really
> makes it clear.

I could easily put it below the control buttons, I think it was originally mocked-up that way, and it's also what mobile does. The downside is that the columns would (probably) no longer line up vertically.
(In reply to comment #17)
> (In reply to comment #15)
> > 2. Allowing previously written preferences for add-ons to appear in the
> > detail view
> 
> I don't think we ever want to do the second part of this to be honest. There
> is just nothing standard about the custom UI that developers have created,
> it would probably end up looking bad in almost all cases.

Wfm!  No doubt it would be ugly as sin.
Attachment #531487 - Flags: ui-review?(jboriss) → ui-review-
Created attachment 531498 [details]
Mockup: Tweaks needed on patch 531487

There's a few things that could be tweaked a bit:

1. Consistent line spacing

Could we make extra preferences blend seamlessly with the other on-line items so that all the one-line add-on options are completely consistent?  I suppose there's some possibility of a security question, as this would make options that Mozilla provides indistinguishable from those the add-on provides... but at the point a user has an add-on installed, I'm not sure how much more harm a deceptive preference item could cause.  ("How much does Mozilla suck? () a little (X) a lot")

2. Contextually styled buttons and widgets

You're right that it would be great to style these the same way as the add-ons manager.  Same "security question" as #1, probably with the same answer.

3. Align widgets to the top of a row rather than the vertical center

This is so that heading of the permissions entry is linked to the widget rather than a point of the description of the permission entry

4. Indent permission descriptions

Indenting the description of a permission allows the headings of permissions to be easily scanned vertically while the descriptions are placed in a hierarchy level below the name of the permissions itself
(Assignee)

Comment 22

6 years ago
Created attachment 531561 [details]
screenshot

I think this solves all the above.

I had to work around the settings binding to get the column-spanning label due to the way XUL grids work. This also solves my concern about the grid getting messed up by large labels.

There's also something weird going on with the XUL menulist binding that I've had to work around to make it display properly. I'd forgotten that menulists were even allowed here.
Attachment #529022 - Attachment is obsolete: true
Attachment #531561 - Flags: ui-review?(jboriss)
(Assignee)

Comment 23

6 years ago
Oh, oops. I've just noticed your second mockup, Jennifer. :|
Keywords: dev-doc-needed
(In reply to comment #23)
> Oh, oops. I've just noticed your second mockup, Jennifer. :|

No worries, it's super-close!  Just needs #1.  Though on attachment 531561 [details], the description for the "Enable profiling" headline goes past the widget column.  It should really stop a nice indented distance away and the wrap to the next line, never crossing the widgets on the right.

Also, damn, the last updated date is far further left than it should be and doesn't appear aligned with widgets, but that looks like a problem in the add-ons manager overall rather than with your patch.  Thanks for making me realize how not-aligned those are!  :)
(In reply to comment #24)
> Also, damn, the last updated date is far further left than it should be and
> doesn't appear aligned with widgets

Actually, its the rest of the widgets being too far to the right :) They have an added left padding, while the label does not (same with the homepage link).
(Assignee)

Comment 26

6 years ago
Created attachment 531892 [details] [diff] [review]
patch

I'm going to need someone to test this on Mac to find out what the height for a row should be. Also that I've done the styling of button and menulist right. 

(In reply to comment #25)
> (In reply to comment #24)
> > Also, damn, the last updated date is far further left than it should be and
> > doesn't appear aligned with widgets
> 
> Actually, its the rest of the widgets being too far to the right :) They
> have an added left padding, while the label does not (same with the homepage
> link).

The label has had its margin removed with CSS for some reason I don't entirely understand, but anyway I'll fix it in a followup bug once I'm finished with this one.
Attachment #531211 - Attachment is obsolete: true
(Assignee)

Comment 27

6 years ago
Created attachment 533133 [details] [diff] [review]
patch

I think, at last, this is review-worthy.
Attachment #531892 - Attachment is obsolete: true
Attachment #533133 - Flags: review?(mark.finkle)
Attachment #533133 - Flags: review?(dtownsend)
(Assignee)

Updated

6 years ago
Attachment #531213 - Flags: review?(dtownsend)
Comment on attachment 533133 [details] [diff] [review]
patch

Review of attachment 533133 [details] [diff] [review]:
-----------------------------------------------------------------

This is really good work, most of these comments are pretty simple stuff though there are some additional tests I'd like to see. Will leave it to Mark to review the mobile parts.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +631,3 @@
>      // all other types they are silently ignored
>      addon.optionsURL = null;
> +    addon.optionsType = null;

You're not actually reading the optionsType from the install.rdf anywhere are you? An illegal type should make parsing the manifest fail (since it would mean the add-on used an options type we don't know about).

Kind of torn between using numeric constants in install.rdf for it so we're consistent with em:type or the more explanatory strings. Numbers pretty much rule out typos and don't suffer from difficulties across locales though so I think we should do that.

Please add something to test_manifest.js to make sure we read it properly, also something (probably to a new xpcshell test) to make sure that optionsType and optionsURL behave as expected.

@@ +4121,5 @@
>        this.connection.createTable("addon",
>                                    "internal_id INTEGER PRIMARY KEY AUTOINCREMENT, " +
>                                    "id TEXT, location TEXT, version TEXT, " +
>                                    "type TEXT, internalName TEXT, updateURL TEXT, " +
> +                                  "updateKey TEXT, optionsURL TEXT, optionsType TEXT, aboutURL TEXT, " +

rewrap these lines so it isn't quite so long

@@ +6643,5 @@
> +    if (this.isActive && this.hasResource("options.xul"))
> +      return "inline";
> +
> +    return null;
> +  }, this);

Test this.isActive first and just return null then rather than having to test it twice.

I think we need to use some defined constants in AddonManager here, OPTIONS_TYPE_INLINE and OPTIONS_TYPE_DIALOG and use those throughout the patch.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2535,5 @@
>        document.getElementById("detail-findUpdates-btn").hidden = false;
>      }
>  
> +    document.getElementById("detail-prefs-btn").hidden = !aIsRemote &&
> +      (!aAddon.optionsURL || aAddon.optionsType == "inline");

This probably should just check aIsRemote and then call cmd_showItemPreferences.isEnabled for the other bits, looks like it will allow opening options for disabled add-ons right now.

@@ +2684,5 @@
> +  emptySettingsRows: function () {
> +    var settingsRows = document.getElementById("detail-settings");
> +    while (settingsRows.lastChild) {
> +      settingsRows.removeChild(settingsRows.lastChild);
> +    }

Don't need braces around the single statement in a while loop

@@ +2691,5 @@
> +  fillSettingsRows: function () {
> +    this.emptySettingsRows();
> +
> +    var settingsRows = document.getElementById("detail-settings");
> +    if (this._addon.optionsType == "inline") {

Just return early if this isn't the case and save the indentation

@@ +2710,5 @@
> +        // remove menulist controls for replacement later
> +        var control = setting.firstElementChild;
> +        if (setting.getAttribute('type') == 'control' && control && control.localName == 'menulist') {
> +          setting.removeChild(control);
> +        }

No need for braces around the single statement (unless part of a multi-block set in which case they all have brances), here and elsewhere

@@ +2722,5 @@
> +          desc = setting.getAttribute('desc');
> +          setting.removeAttribute('desc');
> +        }
> +
> +        settingsRows.appendChild (setting);

No space before the brackets

@@ +2726,5 @@
> +        settingsRows.appendChild (setting);
> +
> +        // replicate menulist and add it to the setting (which is now part of the document)
> +        if (setting.getAttribute('type') == 'control' && control && control.localName == 'menulist') {
> +          var menulist = document.createElement('menulist');

does document.importNode make this any easier?

@@ +2732,5 @@
> +            menulist.setAttribute(control.attributes[j].nodeName, control.attributes[j].nodeValue);
> +          }
> +          var popup = control.getElementsByTagName('menupopup')[0];
> +          if (popup) {
> +            menulist.appendChild(popup.cloneNode(true));

Please use document.importNode here

@@ +2741,5 @@
> +        // add a new row containing the description
> +        if (desc) {
> +          var row = document.createElement('row');
> +          var label = document.createElement('label');
> +          label.className = 'prefdesc';

Use "preferences-description"

@@ +2745,5 @@
> +          label.className = 'prefdesc';
> +          label.textContent = desc;
> +          row.appendChild(label);
> +          settingsRows.appendChild(row);
> +        }

Just append the <label>, no need for the surrounding <row>

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1251,5 @@
>                this.removeAttribute("notification");
>              }
>            }
>  
> +          this._preferencesBtn.hidden = !this.mAddon.optionsURL || this.mAddon.optionsType == "inline";

Kind of think the options button should double up as a way to show the details in the inline case.

::: toolkit/mozapps/extensions/content/extensions.xul
@@ +572,5 @@
>                      </hbox>
>                    </vbox>
>                    <grid id="detail-grid">
>                      <columns>
> +                       <column flex="1" style="max-width: 25em;"/>

Put the style into the theme css

@@ +618,5 @@
>                            <label id="detail-reviews" class="text-link"/>
>                          </hbox>
>                        </row>
>                        <row class="detail-row" id="detail-downloads" label="&detail.numberOfDownloads.label;"/>
> +                      <rows id="detail-settings" label="settings" />

Nested rows seems odd, could you instead just add the rows to the end of the list.

::: mobile/chrome/content/bindings/setting.xml
@@ +165,3 @@
>      <content>
> +      <xul:vbox class="setting-label">
> +        <xul:label class="preftitle" xbl:inherits="value=title" crop="end" flex="1"/>

Can we make the class names a little more readable, preferences-title, preferences-description etc.

::: toolkit/mozapps/extensions/jar.mn
@@ +17,5 @@
>  * content/mozapps/extensions/update.xul                         (content/update.xul)
>  * content/mozapps/extensions/update.js                          (content/update.js)
>  * content/mozapps/extensions/eula.xul                           (content/eula.xul)
>  * content/mozapps/extensions/eula.js                            (content/eula.js)
> +* content/mozapps/extensions/setting.xml                        (content/setting.xml)

Doesn't need to be pre-processed I think
Attachment #533133 - Flags: review?(dtownsend) → review-
Comment on attachment 531213 [details] [diff] [review]
tests

Review of attachment 531213 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/browser/addons/browser_inlinesettings1/options.xul
@@ +2,5 @@
> +<vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +  <setting pref="extensions.inlinesettings1.bool" type="bool" title="Bool"/>
> +  <setting pref="extensions.inlinesettings1.boolint" type="boolint" on="1" off="2" title="BoolInt"/>
> +  <setting pref="extensions.inlinesettings1.integer" type="integer" title="Integer"/>
> +  <setting pref="extensions.inlinesettings1.string" type="string" title="String"/>

Add a test for a menulist as that case is handled specially, also for both ways of specifying the description.
Attachment #531213 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 30

6 years ago
Created attachment 533603 [details] [diff] [review]
patch, part 1

I've split the patch in two for easier handling, this part does the bindings and themes, the other does the changes to the addon manager.
Attachment #533133 - Attachment is obsolete: true
Attachment #533133 - Flags: review?(mark.finkle)
Attachment #533603 - Flags: review?(mark.finkle)
Attachment #533603 - Flags: review?(dtownsend)
(Assignee)

Comment 31

6 years ago
Created attachment 533604 [details] [diff] [review]
patch, part 2
Attachment #533604 - Flags: review?(dtownsend)
(Assignee)

Comment 32

6 years ago
Created attachment 533606 [details] [diff] [review]
tests
Attachment #531213 - Attachment is obsolete: true
Attachment #533606 - Flags: review?(dtownsend)
(Assignee)

Comment 33

6 years ago
(In reply to comment #28)
> @@ +2726,5 @@
> > +        settingsRows.appendChild (setting);
> > +
> > +        // replicate menulist and add it to the setting (which is now part of the document)
> > +        if (setting.getAttribute('type') == 'control' && control && control.localName == 'menulist') {
> > +          var menulist = document.createElement('menulist');
> 
> does document.importNode make this any easier?

No, there's something strange going on with the binding here (it has no binding in one document, but does in the other, which throws an exception). There's no way to set the initial value of the menulist on desktop anyway, which makes it pretty useless, so I'll fix this in a followup bug.

> @@ +2745,5 @@
> > +          label.className = 'prefdesc';
> > +          label.textContent = desc;
> > +          row.appendChild(label);
> > +          settingsRows.appendChild(row);
> > +        }
> 
> Just append the <label>, no need for the surrounding <row>

Without the row, the label spans both columns, which is not what we want.

> @@ +618,5 @@
> >                            <label id="detail-reviews" class="text-link"/>
> >                          </hbox>
> >                        </row>
> >                        <row class="detail-row" id="detail-downloads" label="&detail.numberOfDownloads.label;"/>
> > +                      <rows id="detail-settings" label="settings" />
> 
> Nested rows seems odd, could you instead just add the rows to the end of the
> list.

A second rows node gets drawn in the same place as the first one. Another bug to file.
Status: NEW → ASSIGNED
Comment on attachment 533603 [details] [diff] [review]
patch, part 1

>diff --git a/mobile/themes/core/config.css b/mobile/themes/core/config.css

> #editor-setting .prefbox {
>   border-color: transparent !important;
> }

You missed this "prefbox" usage

otherwise looks good
Attachment #533603 - Flags: review?(mark.finkle) → review+
Comment on attachment 533603 [details] [diff] [review]
patch, part 1

Review of attachment 533603 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/chrome/content/bindings/setting.xml
@@ -357,5 @@
> -
> -      <property name="value" onget="return this.input.value;" onset="return this.input.value=val;"/>
> -    </implementation>
> -  </binding>
> -</bindings>

Looks like this patch has lost the fact that you've just moved the file. Would like that fixed before landing.

::: mobile/chrome/content/config.js
@@ +261,5 @@
>      label.setAttribute("crop", "end");
>      row.appendChild(label);
>  
>      label = document.createElement("label");
>      label.setAttribute("class", "prefvalue");

Missed one, let's use preferences-value here
Attachment #533603 - Flags: review?(dtownsend) → review+
(In reply to comment #33)
> > @@ +2745,5 @@
> > > +          label.className = 'prefdesc';
> > > +          label.textContent = desc;
> > > +          row.appendChild(label);
> > > +          settingsRows.appendChild(row);
> > > +        }
> > 
> > Just append the <label>, no need for the surrounding <row>
> 
> Without the row, the label spans both columns, which is not what we want.

The screenshot makes it look like that is exactly what we want

> > @@ +618,5 @@
> > >                            <label id="detail-reviews" class="text-link"/>
> > >                          </hbox>
> > >                        </row>
> > >                        <row class="detail-row" id="detail-downloads" label="&detail.numberOfDownloads.label;"/>
> > > +                      <rows id="detail-settings" label="settings" />
> > 
> > Nested rows seems odd, could you instead just add the rows to the end of the
> > list.
> 
> A second rows node gets drawn in the same place as the first one. Another
> bug to file.

I don't understand this comment. Appending rows at the end of the rest of the rows certainly shouldn't be a problem
Comment on attachment 533604 [details] [diff] [review]
patch, part 2

Review of attachment 533604 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +153,5 @@
>  
> +const OPTIONS_TYPES = {
> +  window: 1,
> +  inline: 2
> +};

No need for this, we should just have the constants in AddonManager match those we use in install.rdf

@@ +6660,5 @@
> +    if (this.hasResource("options.xul"))
> +      return "inline";
> +
> +    if (this.optionsURL)
> +      return "window";

You haven't defined the constants in AddonManager.jsm like I asked.
Attachment #533604 - Flags: review?(dtownsend) → review-
Comment on attachment 533606 [details] [diff] [review]
tests

Review of attachment 533606 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533606 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 39

6 years ago
Created attachment 534167 [details]
screenshot

(In reply to comment #36)
> The screenshot makes it look like that is exactly what we want

The screenshot is out of date and doesn't match the mockup, which reminds me, I haven't got ui-r yet.
Attachment #531487 - Attachment is obsolete: true
Attachment #531561 - Attachment is obsolete: true
Attachment #531561 - Flags: ui-review?(jboriss)
Attachment #534167 - Flags: ui-review?(jboriss)
(Assignee)

Comment 40

6 years ago
Created attachment 534169 [details] [diff] [review]
patch, part 1

I've moved the theme .css files to the other patch because they make more sense there.
Attachment #533603 - Attachment is obsolete: true
(Assignee)

Comment 41

6 years ago
Created attachment 534171 [details] [diff] [review]
patch, part 2

(In reply to comment #36)
> > > Nested rows seems odd, could you instead just add the rows to the end of the
> > > list.
> > 
> > A second rows node gets drawn in the same place as the first one. Another
> > bug to file.
> 
> I don't understand this comment. Appending rows at the end of the rest of
> the rows certainly shouldn't be a problem

Misunderstood what you asked for. I wanted to keep the settings together inside a <rows> element because it's easier to handle, but ok.

(In reply to comment #37)
> Comment on attachment 533604 [details] [diff] [review] [review]
> patch, part 2
> 
> You haven't defined the constants in AddonManager.jsm like I asked.

Yeah, oops. Sorry.
Attachment #533604 - Attachment is obsolete: true
Attachment #534171 - Flags: review?(dtownsend)
(Assignee)

Comment 42

6 years ago
Created attachment 534173 [details] [diff] [review]
tests

I keep forgetting to update MockProvider to match my changes in XPIProvider, which confuses me until I remember :(
Attachment #533606 - Attachment is obsolete: true
Comment on attachment 534171 [details] [diff] [review]
patch, part 2

Review of attachment 534171 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +624,5 @@
>  
>    // Only read the bootstrapped property for extensions
>    if (addon.type == "extension") {
>      addon.bootstrap = getRDFProperty(ds, root, "bootstrap") == "true";
> +    if (!!addon.optionsType &&

Don't think the !! is necessary here

@@ +627,5 @@
>      addon.bootstrap = getRDFProperty(ds, root, "bootstrap") == "true";
> +    if (!!addon.optionsType &&
> +        addon.optionsType != AddonManager.OPTIONS_TYPE_DIALOG &&
> +        addon.optionsType != AddonManager.OPTIONS_TYPE_INLINE)
> +      throw new Error("Install manifest specifies unknown type: " + addon.optionsType);

Braces around this statement because the condition is multi-line (Our style rules are confusing I know, sorry about that).

::: toolkit/mozapps/extensions/content/extensions.js
@@ +885,4 @@
>            return false;
> +        if (gViewController.currentViewObj == gDetailView &&
> +            aAddon.optionsType == AddonManager.OPTIONS_TYPE_INLINE)
> +          return false;

Braces here too

@@ +2710,5 @@
> +    var xml = xhr.responseXML;
> +    var settings = xml.querySelectorAll(":root > setting");
> +
> +    // This horrible piece of code fixes two problems. 1) The menulist binding doesn't apply
> +    // correctly when it's moved from one document to another, which is solved by manually

Could you file a bug about the menulist issue and reference it here.
Attachment #534171 - Flags: review?(dtownsend) → review+
(In reply to comment #42)
> Created attachment 534173 [details] [diff] [review] [review]
> tests
> 
> I keep forgetting to update MockProvider to match my changes in XPIProvider,
> which confuses me until I remember :(

Hey Geoff, could you attach a screenshot of this patch in action?  Hugely appreciated!
This looks fantastic except for the widgets on the right not being entirely lined up.  A text string, for instance, is far too far to the left.  Text fields are further left than radio buttons.  Is this something that can/should be addressed in this patch, or it that something we should address at a higher level for the entire detailed view UI?
(Assignee)

Comment 46

6 years ago
(In reply to comment #45)
> This looks fantastic except for the widgets on the right not being entirely
> lined up.  A text string, for instance, is far too far to the left.  Text
> fields are further left than radio buttons.  Is this something that
> can/should be addressed in this patch, or it that something we should
> address at a higher level for the entire detailed view UI?

See comment 26, it's a simple fix that I can add in with these patches if you like.
(Assignee)

Comment 47

6 years ago
Created attachment 534584 [details] [diff] [review]
fix for misaligned labels
Attachment #534584 - Flags: review?(jboriss)
(Assignee)

Comment 48

6 years ago
Created attachment 534585 [details]
fixed labels
(Assignee)

Comment 49

6 years ago
(In reply to comment #43)
> @@ +2710,5 @@
> > +    var xml = xhr.responseXML;
> > +    var settings = xml.querySelectorAll(":root > setting");
> > +
> > +    // This horrible piece of code fixes two problems. 1) The menulist binding doesn't apply
> > +    // correctly when it's moved from one document to another, which is solved by manually
> 
> Could you file a bug about the menulist issue and reference it here.

Filed bug 659163.
Comment on attachment 534584 [details] [diff] [review]
fix for misaligned labels

This looks great.  As Geoff describes in the previous comments, there's some alignment issues to work out separately, but this patch is good to go.
Attachment #534584 - Flags: review?(jboriss) → review+
Attachment #534167 - Flags: ui-review?(jboriss) → ui-review+
(Assignee)

Comment 51

6 years ago
Created attachment 534606 [details] [diff] [review]
patch, part 1 [check me in]
Attachment #534169 - Attachment is obsolete: true
(Assignee)

Comment 52

6 years ago
Created attachment 534608 [details] [diff] [review]
patch, part 2 [check me in]
Attachment #534171 - Attachment is obsolete: true
(Assignee)

Comment 53

6 years ago
Created attachment 534610 [details] [diff] [review]
tests [check me in]
Attachment #534173 - Attachment is obsolete: true
(Assignee)

Comment 54

6 years ago
Created attachment 534611 [details] [diff] [review]
fix for misaligned labels [check me in]
Attachment #534584 - Attachment is obsolete: true
(Assignee)

Comment 55

6 years ago
OK, I think I'm done with the bugspam, let's land this sucker.
Keywords: checkin-needed
(Assignee)

Comment 56

6 years ago
Gah. Tests failed on try. *insert bad language here*
Keywords: checkin-needed
Comment on attachment 534606 [details] [diff] [review]
patch, part 1 [check me in]

Geoff - Mobile kinda bitrotted you, but not in a way that breaks the patch. It will break our gingerbread theme though. Can you make the same changes as in:

/mobile/themes/core/browser.css

to this file to:

/mobile/themes/core/gingerbread/browser.css

Feel free to just do it in a separate patch on this bug. I can review it quickly.
(Assignee)

Comment 58

6 years ago
Created attachment 535297 [details] [diff] [review]
Changes to Gingerbread

Thanks for pointing that out Mark.
Attachment #535297 - Flags: review?(mark.finkle)
Attachment #535297 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

6 years ago
Depends on: 521644
(Assignee)

Updated

6 years ago
Depends on: 506069
(Assignee)

Comment 59

6 years ago
Created attachment 535978 [details] [diff] [review]
patch, part 1 [check me in]

Unbitrotted
Attachment #534606 - Attachment is obsolete: true
(Assignee)

Comment 60

6 years ago
Created attachment 535979 [details] [diff] [review]
patch, part 2 [check me in]

Replaced the menulist code with a warning for now, I'll fix that in bug 659163.
Fixed the dodgy Mac styling which was causing the test failures.
Attachment #534608 - Attachment is obsolete: true
(Assignee)

Comment 61

6 years ago
Created attachment 535981 [details] [diff] [review]
tests [check me in]

Changed the tests to not simulate keyboard/mouse events as they seem to be unreliable.
Attachment #534610 - Attachment is obsolete: true
(Assignee)

Comment 62

6 years ago
Created attachment 535982 [details] [diff] [review]
fix for misaligned labels [checked in]

Fixed my own bitrot. :(
Attachment #534611 - Attachment is obsolete: true
(Assignee)

Comment 63

6 years ago
Created attachment 535983 [details] [diff] [review]
Changes to Gingerbread

More bitrot removal.
Attachment #535297 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Depends on: 659163
Any changes you've made since the last review need to be reviewed, if you could provide a diff of just those changes it'd be much easier.
Keywords: checkin-needed
(Assignee)

Comment 65

6 years ago
Created attachment 536037 [details] [diff] [review]
interdiff

So they do.
Attachment #536037 - Flags: review?(dtownsend)

Updated

6 years ago
Attachment #536037 - Attachment is patch: true
Attachment #536037 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 536037 [details] [diff] [review]
interdiff

Review of attachment 536037 [details] [diff] [review]:
-----------------------------------------------------------------

We've never had cases where simulating keyboard/mouse events is unreliable in the past so what is going on here? It really reduces the test coverage to just programmatically do everything.
(Assignee)

Comment 67

6 years ago
Created attachment 536773 [details] [diff] [review]
interdiff

Finally got these tests working properly on try. Can't believe how simple it was really :(
Attachment #536037 - Attachment is obsolete: true
Attachment #536037 - Flags: review?(dtownsend)
Attachment #536773 - Flags: review?(dtownsend)
Attachment #536773 - Attachment is patch: true
Comment on attachment 536773 [details] [diff] [review]
interdiff

Review of attachment 536773 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent. Could you attach the final patches so we can get this landed.
Attachment #536773 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 69

6 years ago
Created attachment 537021 [details] [diff] [review]
patch, part 2 [checked in]
Attachment #535979 - Attachment is obsolete: true
(Assignee)

Comment 70

6 years ago
Created attachment 537022 [details] [diff] [review]
tests [checked in]
Attachment #535981 - Attachment is obsolete: true
(Assignee)

Comment 71

6 years ago
Created attachment 537025 [details] [diff] [review]
patch, part 1 [checked in]
Attachment #535978 - Attachment is obsolete: true
(Assignee)

Comment 72

6 years ago
Created attachment 537026 [details] [diff] [review]
Changes to Gingerbread [checked in]
Attachment #535983 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #535983 - Attachment description: Changes to Gingerbread [check me in] → Changes to Gingerbread
(Assignee)

Comment 73

6 years ago
For whoever does check-in, here's the order: part 1, part 2, tests, fix for labels, changes for gingerbread.
Keywords: checkin-needed
(Assignee)

Comment 74

6 years ago
Created attachment 537045 [details] [diff] [review]
fix for styled button hover state [checked in]

... and then this CSS fix that's just been pointed out to me.
Landed:

http://hg.mozilla.org/mozilla-central/rev/cdfc7faf3822
http://hg.mozilla.org/mozilla-central/rev/e0620f11d5ee
http://hg.mozilla.org/mozilla-central/rev/ddf432ea3249
http://hg.mozilla.org/mozilla-central/rev/e931645d2923
http://hg.mozilla.org/mozilla-central/rev/4a6c8415ae8e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Flags: in-litmus-
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: uiwanted
Geoff, would you be able to write up some docs on MDN on this? There is the docs that were written from Fennec that should serve as a good base and probably only need minor tweaking to cover both, https://wiki.mozilla.org/Mobile/Fennec/Extensions/Options

Maybe at https://developer.mozilla.org/en/Extensions/Inline_Options
(Assignee)

Comment 77

6 years ago
I've created that page and linked to it from the install.rdf page and Firefox 7 for developers.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 78

6 years ago
I've just noticed attachment 535982 [details] [diff] [review] didn't get landed.
Keywords: checkin-needed

Comment 79

6 years ago
How can, we themers, test this?
(In reply to comment #79)
> How can, we themers, test this?

Here's a test extension: http://roosterteethsiteextender.com/rtsefiles/inlineoptions.xpi

Comment 81

6 years ago
is there some way to provide one optionsURL for FF < 7 and another for FF <= 7.0a1 ?

Comment 82

6 years ago
*another for FF >= 7.0a1
chrome.manifest allows you to override your optionsURL to different places depending on the application version.

Updated

6 years ago
Keywords: checkin-needed
Target Milestone: --- → mozilla7

Comment 84

6 years ago
Created attachment 537746 [details]
Button misaligned, and description placed wrongly

Two issues I noticed:
1. The button is missing the title (see specs compared to attachment), causing to appear on the left, instead of in the column of checkboxes and inputs.
2. Why is there both a row with description inserted and a vbox with also a description label?
(Assignee)

Comment 85

6 years ago
1. KWierso's addon has a mistake in the options.xul (should be type="control", not type="button")
2. The extra row is because of a few XUL oddities (and because the same code is shared with mobile). The label inside the vbox isn't used on desktop. If you need to hit the description with CSS, use |row > .preferences-description|.
(In reply to comment #85)
> 1. KWierso's addon has a mistake in the options.xul (should be
> type="control", not type="button")
In my defense, I just stole whatever was being used on this page: https://developer.mozilla.org/en/Extensions/Inline_Options
:)
Attachment #537021 - Attachment description: patch, part 2 [check me in] → patch, part 2 [checked in]
Attachment #537022 - Attachment description: tests [check me in] → tests [checked in]
Attachment #537025 - Attachment description: patch, part 1 [check me in] → patch, part 1 [checked in]
Attachment #537026 - Attachment description: Changes to Gingerbread [check me in] → Changes to Gingerbread [checked in]
Attachment #537045 - Attachment description: fix for styled button hover state → fix for styled button hover state [checked in]
Comment on attachment 535982 [details] [diff] [review]
fix for misaligned labels [checked in]

Boriss technically isn't a code reviewer, but this looks ok to me.

Landed: http://hg.mozilla.org/mozilla-central/rev/7f4c811a19f5
Attachment #535982 - Attachment description: fix for misaligned labels [check me in] → fix for misaligned labels [checked in]
Attachment #535982 - Flags: review+
Depends on: 665515
Marco, could you test this feature from an accessibility stand-point? Thanks.
Geoff, can you provide one or more example add-ons I could use to verify this feature? If you don't have time I will have to create those on my own or I can try to extract those from the patch. Thanks.
Yes, an example add-on that shows this feature would indeed be helpful! :)
(Assignee)

Comment 91

6 years ago
Created attachment 541548 [details]
Example addon

No problem, here's one from the tests with a slight modification to show all the features in one go.
(In reply to comment #91)
> No problem, here's one from the tests with a slight modification to show all
> the features in one go.

Thanks Geoff for the example add-on. Just another short question. Is there a difference between non-restartless and restartless add-ons? Or does it have to be implemented the same way?
Depends on: 669342
Depends on: 669345
Depends on: 669377
Depends on: 669390
Depends on: 669392
No longer depends on: 521644
Depends on: 669507
Verified fixed across branches with builds like Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110705 Firefox/7.0a1

I have filed follow-up bugs for any particular issue I have found during testing. The only bug we have to take care of for Firefox 7 is bug 669342.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Depends on: 675584
Depends on: 666682
Depends on: 681683
Depends on: 680802
Depends on: 695282

Updated

6 years ago
Depends on: 708397

Updated

6 years ago
Depends on: 708987

Updated

6 years ago
Depends on: 708990

Updated

6 years ago
Depends on: 712563

Updated

6 years ago
Depends on: 714583

Updated

5 years ago
Depends on: 719770
(Assignee)

Comment 94

5 years ago
Created attachment 597354 [details]
Updated example addon

I've updated the example addon to include the newer settings types.
Attachment #541548 - Attachment is obsolete: true
please ignore!  
                                                   
   +–––––––––––––––––––––––––+                     
   |                         |                     
   | +–––––+ +–––––+ +–––––+ |                     
   | |  1  | |  2  | |  3  | |                     
   | +–––––+ +–––––+ +–––––+ |                     
   |                         |                     
   | +–––––+ +–––––+ +–––––+ |                     
   | |  4  | |  5  | |  6  | |                     
   | +–––––+ +–––––+ +–––––+ |                     
   |                         |                     
   | +–––––+ +–––––+ +–––––+ |                     
   | |  7  | |  8  | |  9  | |                     
   | +–––––+ +–––––+ +–––––+ |                     
   |                         |                     
   +–––––––––––––––––––––––––+
You need to log in before you can comment on or make changes to this bug.