Bug 1508446 - Require that [accesskey] gets set on <xul:label> to enable formatting;r=paolo
☠☠ backed out by 54812dc57a75 ☠ ☠
authorBrian Grinstead <bgrinstead@mozilla.com>
Wed, 21 Nov 2018 22:35:44 +0000
changeset 506844 fab7024e765889bd2ea6d42e735e40b74e3ba166
parent 506843 e70144f3fc89118b48a541daf9206cef292b4a24
child 506845 00e3e4c617a8f22df2e49a428eae8dbde2aea641
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspaolo
bugs1508446
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1508446 - Require that [accesskey] gets set on <xul:label> to enable formatting;r=paolo Previously, if the accesskey attribute was missing then the label would reach up to binding parent to find it's accesskey. In practice, bindings already do [xbl:inherits=accesskey] to send it down to the label anyway. The problem with this is that for controls without accesskeys, the attribute doesn't get set, so the label will access the control from JS. This is fine for XBL, since typically the label XBL will construct at the same time as the control, but when migrating to Custom Elements, the label gets connected even when the control is hidden. Differential Revision: https://phabricator.services.mozilla.com/D12355
browser/components/customizableui/PanelMultiView.jsm
toolkit/content/tests/chrome/test_labelcontrol.xul
toolkit/content/widgets/general.xml
toolkit/content/widgets/text.xml
toolkit/content/widgets/textbox.xml
toolkit/content/widgets/toolbarbutton.xml
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -1327,18 +1327,19 @@ var PanelView = class extends Associated
         "label" + isMultiline,
         "toolbarbutton[wrap]:not([hidden])",
       ].join(",");
       for (let element of this.node.querySelectorAll(selector)) {
         // Ignore items in hidden containers.
         if (element.closest("[hidden]")) {
           continue;
         }
+
         // Take the label for toolbarbuttons; it only exists on those elements.
-        element = element.labelElement || element;
+        element = element.multilineLabel || element;
 
         let bounds = element.getBoundingClientRect();
         let previous = gMultiLineElementsMap.get(element);
         // We don't need to (re-)apply the workaround for invisible elements or
         // on elements we've seen before and haven't changed in the meantime.
         if (!bounds.width || !bounds.height ||
             (previous && element.textContent == previous.textContent &&
                          bounds.width == previous.bounds.width)) {
--- a/toolkit/content/tests/chrome/test_labelcontrol.xul
+++ b/toolkit/content/tests/chrome/test_labelcontrol.xul
@@ -5,39 +5,48 @@
   XUL Widget Test for label control="value"
   -->
 <window title="tabindex" width="500" height="600"
         onload="runTests()"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>  
   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>  
 
-<label id="lab" control="ctl"/>
-<textbox id="ctl" value="Test"/>
-<checkbox id="chk" value="Checkbox"/>
+<label id="textbox-label" control="textbox" />
+<textbox id="textbox" value="Test"/>
+<label id="checkbox-label" control="checkbox" />
+<checkbox id="checkbox" value="Checkbox"/>
 
 <body xmlns="http://www.w3.org/1999/xhtml">
 <p id="display"></p>
 <div id="content" style="display: none">
 </div>
 <pre id="test">
 </pre>
 </body>
 
 <script>
 <![CDATA[
 
 SimpleTest.waitForExplicitFinish();
  
 function runTests()
 {
-  is($("lab").control, "ctl", "control");
-  is($("lab").labeledControlElement, $("ctl"), "labeledControlElement");
-  is($("ctl").labelElement, $("lab"), "labelElement");
-  is($("chk").labelElement.className, "checkbox-label", "labelElement");
+  let textbox = $("textbox");
+  let textboxLabel = $("textbox-label");
+  is(textboxLabel.control, "textbox", "textbox control");
+  is(textboxLabel.labeledControlElement, textbox, "textbox labeledControlElement");
+
+  let checkbox = $("checkbox");
+  let checkboxLabel = $("checkbox-label");
+  is(checkboxLabel.control, "checkbox", "checkbox control");
+  is(checkboxLabel.labeledControlElement, checkbox, "checkbox labeledControlElement");
+  is(checkbox.accessKey, "", "checkbox accessKey not set");
+  checkboxLabel.accessKey = "C";
+  is(checkbox.accessKey, "C", "checkbox accessKey set");
 
   SimpleTest.finish();
 }
 
 ]]>
 
 </script>
 
--- a/toolkit/content/widgets/general.xml
+++ b/toolkit/content/widgets/general.xml
@@ -31,30 +31,29 @@
                                   onget="return this.getAttribute('crop');"/>
       <property name="image"      onset="this.setAttribute('image',val); return val;"
                                   onget="return this.getAttribute('image');"/>
       <property name="command"    onset="this.setAttribute('command',val); return val;"
                                   onget="return this.getAttribute('command');"/>
       <property name="accessKey">
         <getter>
           <![CDATA[
-            return this.labelElement ? this.labelElement.accessKey : this.getAttribute("accesskey");
+            return this.getAttribute("accesskey");
           ]]>
         </getter>
         <setter>
           <![CDATA[
             // Always store on the control
             this.setAttribute("accesskey", val);
             // If there is a label, change the accesskey on the labelElement
             // if it's also set there
-            if (this.labelElement) {
-              this.labelElement.accessKey = val;
+            let labelElement = document.getElementsByAttribute("control", this.id)[0];
+            if (labelElement) {
+              labelElement.setAttribute("accesskey", val);
             }
             return val;
           ]]>
         </setter>
       </property>
-
-      <field name="labelElement"/>
     </implementation>
   </binding>
 
 </bindings>
--- a/toolkit/content/widgets/text.xml
+++ b/toolkit/content/widgets/text.xml
@@ -19,31 +19,16 @@
           this.formatAccessKey(true);
         ]]>
       </constructor>
 
       <method name="formatAccessKey">
         <parameter name="firstTime"/>
         <body>
           <![CDATA[
-            var control = this.labeledControlElement;
-            if (!control) {
-              var bindingParent = document.getBindingParent(this);
-              if ("accessKey" in bindingParent) {
-                control = bindingParent; // For controls that make the <label> an anon child
-              }
-            }
-            if (control) {
-              control.labelElement = this;
-              var controlAccessKey = control.getAttribute("accesskey");
-              if (controlAccessKey) {
-                this.setAttribute("accesskey", controlAccessKey);
-              }
-            }
-
             var accessKey = this.accessKey;
             // No need to remove existing formatting the first time.
             if (firstTime && !accessKey)
               return;
 
             if (this.mInsertSeparator === undefined) {
               try {
                 var prefs = Cc["@mozilla.org/preferences-service;1"].
@@ -83,17 +68,17 @@
 
             var oldHiddenSpan =
               this.getElementsByAttribute("class", "hiddenColon").item(0);
             if (oldHiddenSpan) {
               this.mergeElement(oldHiddenSpan);
             }
 
             var labelText = this.textContent;
-            if (!accessKey || !labelText || !control) {
+            if (!accessKey || !labelText) {
               return;
             }
             var accessKeyIndex = -1;
             if (!this.mAlwaysAppendAccessKey) {
               accessKeyIndex = labelText.indexOf(accessKey);
               if (accessKeyIndex < 0) { // Try again in upper case
                 accessKeyIndex =
                   labelText.toUpperCase().indexOf(accessKey.toUpperCase());
@@ -190,48 +175,39 @@
         !/Mac/.test(navigator.platform)
       </field>
       <field name="mInsertSeparator"/>
       <field name="mAlwaysAppendAccessKey">false</field>
 
       <property name="accessKey">
         <getter>
           <![CDATA[
-            var accessKey = this.getAttribute("accesskey");
-            return accessKey ? accessKey[0] : null;
+            return this.getAttribute("accesskey");
           ]]>
         </getter>
         <setter>
           <![CDATA[
-            // If this label already has an accesskey attribute store it here as well
-            if (this.hasAttribute("accesskey")) {
-              this.setAttribute("accesskey", val);
-            }
+            this.setAttribute("accesskey", val);
             var control = this.labeledControlElement;
             if (control) {
               control.setAttribute("accesskey", val);
             }
             this.formatAccessKey(false);
             return val;
           ]]>
         </setter>
       </property>
 
       <property name="labeledControlElement" readonly="true"
                 onget="var control = this.control; return control ? document.getElementById(control) : null;" />
 
       <property name="control" onget="return this.getAttribute('control');">
         <setter>
           <![CDATA[
-            var control = this.labeledControlElement;
-            if (control) {
-              control.labelElement = null; // No longer pointed to be this label
-            }
             this.setAttribute("control", val);
-            this.formatAccessKey(false);
             return val;
           ]]>
         </setter>
       </property>
     </implementation>
 
     <handlers>
       <handler event="click"><![CDATA[
--- a/toolkit/content/widgets/textbox.xml
+++ b/toolkit/content/widgets/textbox.xml
@@ -47,19 +47,17 @@
         ]]></getter>
       </property>
 
       <property name="value"      onset="this.inputField.value = val; return val;"
                                   onget="return this.inputField.value;"/>
       <property name="defaultValue" onset="this.inputField.defaultValue = val; return val;"
                                   onget="return this.inputField.defaultValue;"/>
       <property name="label"      onset="this.setAttribute('label', val); return val;"
-                                  onget="return this.getAttribute('label') ||
-                                                (this.labelElement ? this.labelElement.value :
-                                                 this.placeholder);"/>
+                                  onget="return this.getAttribute('label') || this.placeholder;" />
       <property name="placeholder" onset="this.inputField.placeholder = val; return val;"
                                   onget="return this.inputField.placeholder;"/>
       <property name="emptyText"  onset="this.placeholder = val; return val;"
                                   onget="return this.placeholder;"/>
       <property name="type"       onset="if (val) this.setAttribute('type', val);
                                          else this.removeAttribute('type'); return val;"
                                   onget="return this.getAttribute('type');"/>
       <property name="maxLength"  onset="this.inputField.maxLength = val; return val;"
--- a/toolkit/content/widgets/toolbarbutton.xml
+++ b/toolkit/content/widgets/toolbarbutton.xml
@@ -6,16 +6,20 @@
 
 <bindings id="toolbarbuttonBindings"
    xmlns="http://www.mozilla.org/xbl"
    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
    xmlns:xbl="http://www.mozilla.org/xbl">
 
   <binding id="toolbarbutton" display="xul:button"
            extends="chrome://global/content/bindings/button.xml#button-base">
+    <implementation>
+      <property name="multilineLabel"
+        onget="return document.getAnonymousElementByAttribute(this, 'class', 'toolbarbutton-multiline-text');" />
+    </implementation>
     <content>
       <children includes="observes|template|menupopup|panel|tooltip"/>
       <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label,type,consumeanchor,triggeringprincipal=iconloadingprincipal"/>
       <xul:label class="toolbarbutton-text" crop="right" flex="1"
                  xbl:inherits="value=label,accesskey,crop,dragover-top,wrap"/>
       <xul:label class="toolbarbutton-multiline-text" flex="1"
                  xbl:inherits="xbl:text=label,accesskey,wrap"/>
       <children includes="box"/>