653637 - Provide a simple way for addons to have preferences UI

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Attachment: 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?

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: screenshot

... and a screenshot for good measure. Note the options.xul file used here isn't quite the same as the example on wikimo.

Comment 2

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

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?

Comment 3

[Geoff Lankow (:darktrojan)]

That really doesn't surprise me. I couldn't find the relevant bug either.

Comment 4

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

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

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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)

Comment 9

[Geoff Lankow (:darktrojan)]

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

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: WIP patch

I've fixed a few minor things and done the CSS for pinstripe and gnomestripe.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: WIP tests

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: patch

Fixes comment 12, also hides the settings for disabled addons.

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: tests

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Attachment: 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.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 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?

Comment 17

[Dave Townsend [:mossop]]

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

Comment 18

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

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.

Comment 19

[Geoff Lankow (:darktrojan)]

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.

Comment 20

[Jennifer Morrow [:Boriss] (UX)]

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

Comment 21

[Jennifer Morrow [:Boriss] (UX)]

Attachment: 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

Comment 22

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 23

[Geoff Lankow (:darktrojan)]

Oh, oops. I've just noticed your second mockup, Jennifer. :|

Comment 24

[Jennifer Morrow [:Boriss] (UX)]

(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!  :)

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 26

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: patch

I think, at last, this is review-worthy.

Comment 28

[Dave Townsend [:mossop]]

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

Comment 29

[Dave Townsend [:mossop]]

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.

Comment 30

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 31

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 2

Comment 32

[Geoff Lankow (:darktrojan)]

Attachment: tests

Comment 33

[Geoff Lankow (:darktrojan)]

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

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 35

[Dave Townsend [:mossop]]

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

Comment 36

[Dave Townsend [:mossop]]

(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 37

[Dave Townsend [:mossop]]

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.

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 533606 [details] [diff] [review]
tests

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

Comment 39

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 40

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 1

I've moved the theme .css files to the other patch because they make more sense there.

Comment 41

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 42

[Geoff Lankow (:darktrojan)]

Attachment: tests

I keep forgetting to update MockProvider to match my changes in XPIProvider, which confuses me until I remember :(

Comment 43

[Dave Townsend [:mossop]]

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.

Comment 44

[Jennifer Morrow [:Boriss] (UX)]

(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!

Comment 45

[Jennifer Morrow [:Boriss] (UX)]

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?

Comment 46

[Geoff Lankow (:darktrojan)]

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

Comment 47

[Geoff Lankow (:darktrojan)]

Attachment: fix for misaligned labels

Comment 48

[Geoff Lankow (:darktrojan)]

Attachment: fixed labels

Comment 49

[Geoff Lankow (:darktrojan)]

(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 50

[Jennifer Morrow [:Boriss] (UX)]

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.

Comment 51

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 1 [check me in]

Comment 52

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 2 [check me in]

Comment 53

[Geoff Lankow (:darktrojan)]

Attachment: tests [check me in]

Comment 54

[Geoff Lankow (:darktrojan)]

Attachment: fix for misaligned labels [check me in]

Comment 55

[Geoff Lankow (:darktrojan)]

OK, I think I'm done with the bugspam, let's land this sucker.

Comment 56

[Geoff Lankow (:darktrojan)]

Gah. Tests failed on try. *insert bad language here*

Comment 57

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 58

[Geoff Lankow (:darktrojan)]

Attachment: Changes to Gingerbread

Thanks for pointing that out Mark.

Comment 59

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 1 [check me in]

Unbitrotted

Comment 60

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 61

[Geoff Lankow (:darktrojan)]

Attachment: tests [check me in]

Changed the tests to not simulate keyboard/mouse events as they seem to be unreliable.

Comment 62

[Geoff Lankow (:darktrojan)]

Attachment: fix for misaligned labels [checked in]

Fixed my own bitrot. :(

Comment 63

[Geoff Lankow (:darktrojan)]

Attachment: Changes to Gingerbread

More bitrot removal.

Comment 64

[Dave Townsend [:mossop]]

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.

Comment 65

[Geoff Lankow (:darktrojan)]

Attachment: interdiff

So they do.

Comment 66

[Dave Townsend [:mossop]]

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.

Comment 67

[Geoff Lankow (:darktrojan)]

Attachment: interdiff

Finally got these tests working properly on try. Can't believe how simple it was really :(

Comment 68

[Dave Townsend [:mossop]]

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.

Comment 69

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 2 [checked in]

Comment 70

[Geoff Lankow (:darktrojan)]

Attachment: tests [checked in]

Comment 71

[Geoff Lankow (:darktrojan)]

Attachment: patch, part 1 [checked in]

Comment 72

[Geoff Lankow (:darktrojan)]

Attachment: Changes to Gingerbread [checked in]

Comment 73

[Geoff Lankow (:darktrojan)]

For whoever does check-in, here's the order: part 1, part 2, tests, fix for labels, changes for gingerbread.

Comment 74

[Geoff Lankow (:darktrojan)]

Attachment: fix for styled button hover state [checked in]

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

Comment 75

[Dave Townsend [:mossop]]

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

Comment 76

[Dave Townsend [:mossop]]

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

Comment 77

[Geoff Lankow (:darktrojan)]

I've created that page and linked to it from the install.rdf page and Firefox 7 for developers.

Comment 78

[Geoff Lankow (:darktrojan)]

I've just noticed attachment 535982 [details] [diff] [review] didn't get landed.

Comment 79

[Alfred Kayser]

How can, we themers, test this?

Comment 80

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

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

Comment 81

[Erik Vold]

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

Comment 82

[Erik Vold]

*another for FF >= 7.0a1

Comment 83

[Dave Townsend [:mossop]]

chrome.manifest allows you to override your optionsURL to different places depending on the application version.

Comment 84

[Alfred Kayser]

Attachment: 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?

Comment 85

[Geoff Lankow (:darktrojan)]

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|.

Comment 86

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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
:)

Comment 87

[Dave Townsend [:mossop]]

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

Comment 88

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

Marco, could you test this feature from an accessibility stand-point? Thanks.

Comment 89

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

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.

Comment 90

[Marco Zehe (:MarcoZ)]

Yes, an example add-on that shows this feature would indeed be helpful! :)

Comment 91

[Geoff Lankow (:darktrojan)]

Attachment: Example addon

No problem, here's one from the tests with a slight modification to show all the features in one go.

Comment 92

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

(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?

Comment 93

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

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.

Comment 94

[Geoff Lankow (:darktrojan)]

Attachment: Updated example addon

I've updated the example addon to include the newer settings types.

Comment 95

[Jennifer Morrow [:Boriss] (UX)]

please ignore!  
                                                   
   +–––––––––––––––––––––––––+                     
   |                         |                     
   | +–––––+ +–––––+ +–––––+ |                     
   | |  1  | |  2  | |  3  | |                     
   | +–––––+ +–––––+ +–––––+ |                     
   |                         |                     
   | +–––––+ +–––––+ +–––––+ |                     
   | |  4  | |  5  | |  6  | |                     
   | +–––––+ +–––––+ +–––––+ |                     
   |                         |                     
   | +–––––+ +–––––+ +–––––+ |                     
   | |  7  | |  8  | |  9  | |                     
   | +–––––+ +–––––+ +–––––+ |                     
   |                         |                     
   +–––––––––––––––––––––––––+