Bug 659163 - Make menulists usable and useful in addon manager inline settings; r=dtownsend
authorGeoff Lankow <geoff@darktrojan.net>
Sat, 11 Jun 2011 15:01:15 +1200
changeset 71427 bfcc94f3fd0a854fb0769b1611aaddbfee2847a6
parent 71426 1b97769e6549e5053c817ed932e1bed08df524c3
child 71428 e802d263c27ec5b91df1a154e8e9327115332d43
push id159
push usereakhgari@mozilla.com
push dateTue, 16 Aug 2011 17:53:11 +0000
treeherdermozilla-beta@8786e3e49240 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdtownsend
bugs659163
milestone7.0a1
first release with
nightly win64
bfcc94f3fd0a / 7.0a1 / 20110611030228 / files
nightly linux32
nightly linux64
nightly mac
nightly win32
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly win64
Bug 659163 - Make menulists usable and useful in addon manager inline settings; r=dtownsend
mobile/chrome/content/bindings/extensions.xml
toolkit/mozapps/extensions/content/extensions.js
toolkit/mozapps/extensions/test/browser/addons/browser_inlinesettings1/options.xul
toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
toolkit/mozapps/extensions/test/browser/options.xul
toolkit/themes/pinstripe/mozapps/extensions/extensions.css
toolkit/themes/winstripe/mozapps/extensions/extensions.css
--- a/mobile/chrome/content/bindings/extensions.xml
+++ b/mobile/chrome/content/bindings/extensions.xml
@@ -133,16 +133,20 @@
             for (let i = 0; i < prefs.length; i++)
               box.appendChild(prefs.item(i));
 
             // Send an event so add-ons can prepopulate any non-preference based
             // settings
             let event = document.createEvent("Events");
             event.initEvent("AddonOptionsLoad", true, false);
             this.dispatchEvent(event);
+
+            // Also send a notification to match the behavior of desktop Firefox
+            let id = this.id.substring(17); // length of |urn:mozilla:item:|
+            Services.obs.notifyObservers(document, "addon-options-displayed", id);
           ]]>
         </body>
       </method>
     </implementation>
   </binding>
 
   <binding id="extension-searchplugin" extends="chrome://browser/content/bindings.xml#richlistitem">
     <content orient="vertical">
--- a/toolkit/mozapps/extensions/content/extensions.js
+++ b/toolkit/mozapps/extensions/content/extensions.js
@@ -2816,67 +2816,67 @@ var gDetailView = {
       rows.removeChild(rows.lastChild);
   },
 
   fillSettingsRows: function () {
     this.emptySettingsRows();
     if (this._addon.optionsType != AddonManager.OPTIONS_TYPE_INLINE)
       return;
 
+    // This function removes and returns the text content of aNode without
+    // removing any child elements. Removing the text nodes ensures any XBL
+    // bindings apply properly.
+    function stripTextNodes(aNode) {
+      var text = '';
+      for (var i = 0; i < aNode.childNodes.length; i++) {
+        if (aNode.childNodes[i].nodeType != document.ELEMENT_NODE) {
+          text += aNode.childNodes[i].textContent;
+          aNode.removeChild(aNode.childNodes[i--]);
+        } else {
+          text += stripTextNodes(aNode.childNodes[i]);
+        }
+      }
+      return text;
+    }
+
     var rows = document.getElementById("detail-downloads").parentNode;
 
     var xhr = new XMLHttpRequest();
     xhr.open("GET", this._addon.optionsURL, false);
     xhr.send();
 
     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 (bug 659163), which is solved 
-    // by manually cloning the menulist. 2) Labels and controls aligned to the top of a row 
-    // looks really bad, so the description is put on a new row to preserve alignment.
     for (var i = 0; i < settings.length; i++) {
       var setting = settings[i];
       if (i == 0)
         setting.setAttribute("first-row", true);
 
-      // remove menulist controls for replacement later
-      var control = setting.firstElementChild;
-      if (setting.getAttribute("type") == "control" && control && control.localName == "menulist") {
-        setting.removeChild(control);
-        var consoleMessage = Cc["@mozilla.org/scripterror;1"].
-                             createInstance(Ci.nsIScriptError);
-        consoleMessage.init("Menulist is not available in the addons-manager yet, due to bug 659163",
-                            this._addon.optionsURL, null, null, 0, Ci.nsIScriptError.warningFlag, null);
-        Services.console.logMessage(consoleMessage);
-        continue;
-      }
-
-      // remove setting description, for replacement later
-      var desc = setting.textContent.trim();
-      if (desc)
-        setting.textContent = "";
+      // Remove setting description, for replacement later
+      var desc = stripTextNodes(setting).trim();
       if (setting.hasAttribute("desc")) {
-        desc = setting.getAttribute("desc");
+        desc = setting.getAttribute("desc").trim();
         setting.removeAttribute("desc");
       }
 
       rows.appendChild(setting);
 
-      // add a new row containing the description
+      // Add a new row containing the description
       if (desc) {
         var row = document.createElement("row");
         var label = document.createElement("label");
         label.className = "preferences-description";
         label.textContent = desc;
         row.appendChild(label);
         rows.appendChild(row);
       }
     }
+
+    Services.obs.notifyObservers(document, "addon-options-displayed", this._addon.id);
   },
 
   getSelectedAddon: function() {
     return this._addon;
   },
 
   onEnabling: function() {
     this.updateState();
--- a/toolkit/mozapps/extensions/test/browser/addons/browser_inlinesettings1/options.xul
+++ b/toolkit/mozapps/extensions/test/browser/addons/browser_inlinesettings1/options.xul
@@ -1,16 +1,16 @@
 <?xml version="1.0" ?>
 <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"/>
   <setting type="control" title="Menulist">
-    <menulist sizetopopup="always">
+    <menulist sizetopopup="always" oncommand="window._testValue = this.value;">
       <menupopup>
-        <menuitem label="Test 1" value="1" />
-        <menuitem label="Test 2" value="2" />
-        <menuitem label="Test 3" value="3" />
+        <menuitem label="Alpha" value="1" />
+        <menuitem label="Bravo" value="2" />
+        <menuitem label="Charlie" value="3" />
       </menupopup>
     </menulist>
   </setting>
 </vbox>
--- a/toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
+++ b/toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
@@ -3,16 +3,26 @@
  */
 
 // Tests various aspects of the details view
 
 var gManagerWindow;
 var gCategoryUtilities;
 var gProvider;
 
+const SETTINGS_ROWS = 5;
+
+var observer = {
+  lastData: null,
+  observe: function(aSubject, aTopic, aData) {
+    if (aTopic == "addon-options-displayed")
+      this.lastData = aData;
+  }
+};
+
 function installAddon(aCallback) {
   AddonManager.getInstallForURL(TESTROOT + "addons/browser_inlinesettings1.xpi",
                                 function(aInstall) {
     aInstall.addListener({
       onInstallEnded: function() {
         executeSoon(aCallback);
       }
     });
@@ -38,22 +48,26 @@ function test() {
     optionsURL: CHROMEROOT + "addon_prefs.xul"
   }]);
 
   installAddon(function () {
     open_manager("addons://list/extension", function(aWindow) {
       gManagerWindow = aWindow;
       gCategoryUtilities = new CategoryUtilities(gManagerWindow);
 
+      Services.obs.addObserver(observer, "addon-options-displayed", false);
+
       run_next_test();
     });
   });
 }
 
 function end_test() {
+  Services.obs.removeObserver(observer, "addon-options-displayed");
+
   close_manager(gManagerWindow, function() {
     AddonManager.getAddonByID("inlinesettings1@tests.mozilla.org", function(aAddon) {
       aAddon.uninstall();
       finish();
     });
   });
 }
 
@@ -97,19 +111,21 @@ add_test(function() {
 add_test(function() {
   var addon = get_addon_element(gManagerWindow, "inlinesettings1@tests.mozilla.org");
   addon.parentNode.ensureElementIsVisible(addon);
 
   var button = gManagerWindow.document.getAnonymousElementByAttribute(addon, "anonid", "preferences-btn");
   EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
   wait_for_view_load(gManagerWindow, function() {
+    is(observer.lastData, "inlinesettings1@tests.mozilla.org", "Observer notification should have fired");
+
     var grid = gManagerWindow.document.getElementById("detail-grid");
     var settings = grid.querySelectorAll("rows > setting");
-    is(settings.length, 4, "Grid should have settings children");
+    is(settings.length, SETTINGS_ROWS, "Grid should have settings children");
 
     // Force bindings to apply
     settings[0].clientTop;
 
     Services.prefs.setBoolPref("extensions.inlinesettings1.bool", false);
     var input = gManagerWindow.document.getAnonymousElementByAttribute(settings[0], "anonid", "input");
     isnot(input.checked, true, "Checkbox should have initial value");
     EventUtils.synthesizeMouseAtCenter(input, { clickCount: 1 }, gManagerWindow);
@@ -146,35 +162,51 @@ add_test(function() {
     is(input.value, "foo", "Text box should have initial value");
     input.select();
     EventUtils.synthesizeKey("b", {}, gManagerWindow);
     EventUtils.synthesizeKey("a", {}, gManagerWindow);
     EventUtils.synthesizeKey("r", {}, gManagerWindow);
     is(input.value, "bar", "Text box should have updated value");
     is(Services.prefs.getCharPref("extensions.inlinesettings1.string"), "bar", "String pref should have been updated");
 
-    var button = gManagerWindow.document.getElementById("detail-prefs-btn");
-    is_element_hidden(button, "Preferences button should not be visible");
+    var input = settings[4].firstElementChild;
+    is(input.value, "1", "Menulist should have initial value");
+    input.focus();
+    EventUtils.synthesizeKey("b", {}, gManagerWindow);
+    is(input.value, "2", "Menulist should have updated value");
+    is(gManagerWindow._testValue, "2", "Menulist oncommand handler should've updated the test value");
 
-    gCategoryUtilities.openType("extension", run_next_test);
+    setTimeout(function () {
+      EventUtils.synthesizeKey("c", {}, gManagerWindow);
+      is(input.value, "3", "Menulist should have updated value");
+      is(gManagerWindow._testValue, "3", "Menulist oncommand handler should've updated the test value");
+      delete gManagerWindow._testValue;
+
+      var button = gManagerWindow.document.getElementById("detail-prefs-btn");
+      is_element_hidden(button, "Preferences button should not be visible");
+
+      gCategoryUtilities.openType("extension", run_next_test);
+    }, 1200); // Timeout value from toolkit/content/tests/widgets/test_menulist_keynav.xul
   });
 });
 
 // Addon with inline preferences as optionsURL
 add_test(function() {
   var addon = get_addon_element(gManagerWindow, "inlinesettings2@tests.mozilla.org");
   addon.parentNode.ensureElementIsVisible(addon);
 
   var button = gManagerWindow.document.getAnonymousElementByAttribute(addon, "anonid", "preferences-btn");
   EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
   wait_for_view_load(gManagerWindow, function() {
+    is(observer.lastData, "inlinesettings2@tests.mozilla.org", "Observer notification should have fired");
+
     var grid = gManagerWindow.document.getElementById("detail-grid");
     var settings = grid.querySelectorAll("rows > setting");
-    is(settings.length, 2, "Grid should have settings children");
+    is(settings.length, 3, "Grid should have settings children");
 
     // Force bindings to apply
     settings[0].clientTop;
 
     var node = settings[0];
     is(node.nodeName, "setting", "Should be a setting node");
     var description = gManagerWindow.document.getAnonymousElementByAttribute(node, "class", "preferences-description");
     is(description.textContent.trim(), "", "Description node should be empty");
@@ -187,32 +219,45 @@ add_test(function() {
     is(node.nodeName, "setting", "Should be a setting node");
     description = gManagerWindow.document.getAnonymousElementByAttribute(node, "class", "preferences-description");
     is(description.textContent.trim(), "", "Description node should be empty");
 
     node = node.nextSibling;
     is(node.nodeName, "row", "Setting should be followed by a row node");
     is(node.textContent, "Description Text Node", "Description should be in this row");
 
+    node = settings[2];
+    is(node.nodeName, "setting", "Should be a setting node");
+    description = gManagerWindow.document.getAnonymousElementByAttribute(node, "class", "preferences-description");
+    is(description.textContent.trim(), "", "Description node should be empty");
+    var button = node.firstElementChild;
+    isnot(button, null, "There should be a button");
+
+    node = node.nextSibling;
+    is(node.nodeName, "row", "Setting should be followed by a row node");
+    is(node.textContent.trim(), "This is a test, all this text should be visible", "Description should be in this row");
+    
     var button = gManagerWindow.document.getElementById("detail-prefs-btn");
     is_element_hidden(button, "Preferences button should not be visible");
 
     gCategoryUtilities.openType("extension", run_next_test);
   });
 });
 
 // Addon with non-inline preferences as optionsURL
 add_test(function() {
   var addon = get_addon_element(gManagerWindow, "noninlinesettings@tests.mozilla.org");
   addon.parentNode.ensureElementIsVisible(addon);
 
   var button = gManagerWindow.document.getAnonymousElementByAttribute(addon, "anonid", "details-btn");
   EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
   wait_for_view_load(gManagerWindow, function() {
+    is(observer.lastData, "inlinesettings2@tests.mozilla.org", "Observer notification should not have fired");
+
     var grid = gManagerWindow.document.getElementById("detail-grid");
     var settings = grid.querySelectorAll("rows > setting");
     is(settings.length, 0, "Grid should not have settings children");
 
     var button = gManagerWindow.document.getElementById("detail-prefs-btn");
     is_element_visible(button, "Preferences button should be visible");
 
     gCategoryUtilities.openType("extension", run_next_test);
@@ -225,17 +270,17 @@ add_test(function() {
   addon.parentNode.ensureElementIsVisible(addon);
 
   var button = gManagerWindow.document.getAnonymousElementByAttribute(addon, "anonid", "preferences-btn");
   EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
   wait_for_view_load(gManagerWindow, function() {
     var grid = gManagerWindow.document.getElementById("detail-grid");
     var settings = grid.querySelectorAll("rows > setting");
-    is(settings.length, 4, "Grid should have settings children");
+    is(settings.length, SETTINGS_ROWS, "Grid should have settings children");
 
     // disable
     var button = gManagerWindow.document.getElementById("detail-disable-btn");
     EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
     settings = grid.querySelectorAll("rows > setting");
     is(settings.length, 0, "Grid should not have settings children");
 
@@ -254,15 +299,15 @@ add_test(function() {
         var settings = grid.querySelectorAll("rows > setting");
         is(settings.length, 0, "Grid should not have settings children");
 
         // enable
         var button = gManagerWindow.document.getElementById("detail-enable-btn");
         EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
         settings = grid.querySelectorAll("rows > setting");
-        is(settings.length, 4, "Grid should have settings children");
+        is(settings.length, SETTINGS_ROWS, "Grid should have settings children");
 
         gCategoryUtilities.openType("extension", run_next_test);
       });
     });
   });
 });
--- a/toolkit/mozapps/extensions/test/browser/options.xul
+++ b/toolkit/mozapps/extensions/test/browser/options.xul
@@ -1,5 +1,8 @@
 <?xml version="1.0" ?>
 <vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
   <setting pref="extensions.inlinesettings2.bool1" type="bool" title="Bool 1" desc="Description Attribute"/>
   <setting pref="extensions.inlinesettings2.bool2" type="bool" title="Bool 2">Description Text Node</setting>
+  <setting type="control" title="Button">
+    This is a test, <button label="button" />all this text should be visible
+  </setting>
 </vbox>
--- a/toolkit/themes/pinstripe/mozapps/extensions/extensions.css
+++ b/toolkit/themes/pinstripe/mozapps/extensions/extensions.css
@@ -1119,17 +1119,17 @@ setting[type="control"] menulist {
 }
 
 .addon-control[disabled="true"] {
   display: none;
 }
 
 .addon-control:active:hover,
 setting[type="control"] button:active:hover,
-setting[type="control"] menulist:hover {
+setting[type="control"] menulist:active:hover {
   box-shadow: inset 0 1px 3px rgba(0,0,0,.2), 0 1px rgba(255,255,255,0.25);
   background-image: -moz-linear-gradient(rgba(45,54,71,0.3), rgba(45,54,71,0.1));
   border-color: rgba(60,73,97,0.7);
 }
 
 .button-link {
   -moz-appearance: none;
   background: transparent;
--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css
@@ -1163,17 +1163,17 @@ setting[type="control"] menulist {
   border: 1px solid rgba(31, 64, 100, 0.4);
   border-top-color: rgba(31, 64, 100, 0.3);
   box-shadow: 0 0 0 1px rgba(255, 255, 255, 0.25) inset,
               0 0 2px 1px rgba(255, 255, 255, 0.25) inset;
 }
 
 .addon-control:active:hover,
 setting[type="control"] button:active:hover,
-setting[type="control"] menulist:hover {
+setting[type="control"] menulist:active:hover {
   background-color: rgba(61, 76, 92, 0.2);
   border-color: rgba(39, 53, 68, 0.5);
   box-shadow: 0 0 3px 1px rgba(39, 53, 68, 0.2) inset;
 }
 
 .addon-control > .button-box,
 setting[type="control"] button > .button-box {
   padding: 1px;