333023 - Menulist/menu cleanup

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Various bits of menulist/menu cleanup

I have some testcases I've made for menulists, I'll figure out a place to check them in.

Aaron, not sure about the ValueChange event, should it be fired only when changing the menulist? Right now it doesn't fire when setting it to null, the patch changes it to do that.

Comment 1

[Neil Deakin]

Attachment: Fix various issues

- switch to use image attribute instead of src attribute consistently
- fire select event when selecting an item from a menulist
- add inputField and editable properties to menulist
- add select and control properties to menu, menuitem and menuseparator
- make menu, menuitem and menuseparator inherit from general.xml#control-item
- changed selectionInternal to mSelectionInternal
- in selectedIndex setter, don't change selection if the index is too high
- fire ValueChange attribute when setting selection to null
- prevent setting current item to an item which isn't in the menulist
- use label attribute when setting initial value for editable menulist
- just use inherited constructor for editable menulist

Comment 2

[Aaron Leventhal]

Good idea, valuechange should be fired when it changes to null as well.

Comment 3

[[e-mail]]

Comment on attachment 217447 [details] [diff] [review]
Fix various issues

>+      <property name="selected" readonly="true"
>+                onget="return this.getAttribute('_moz-menuactive') == 'true' ||
>+                              this.getAttribute('selected') == 'true';"/>
_moz-menuactive does not mean the same as selected.

>+      <property name="control" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            var parent = this.parentNode;
>+            while (parent) {
>+              if (parent instanceof Components.interfaces.nsIDOMXULSelectControlElement)
>+                return parent;
>+              parent = parent.parentNode;
>+            }
>+            return null;
Do we really need to walk all the way up? Or are you planning on fixing support for menulists with nested menuitems?

>-  <binding id="menulist" display="xul:menu"
>+  <binding id="menulist" display="xul:menu" 
The jst-review simulacrum, although not completely reliable, is good at spotting whitespace like this.

>-        <xul:image class="menulist-icon" xbl:inherits="src"/>
>+        <xul:image class="menulist-icon" xbl:inherits="src=image"/>
Elsewhere you changed this to src=image,src (I haven't decided which is correct but they can't both be right).

>+              var editable = this.editable;
>+              var value = this.value;
>+              if (!arr.item(0) && value)
>+                arr = popup.getElementsByAttribute(editable ? 'label' : 'value', value);
> 
>               if (arr.item(0))
>                 this.selectedItem = arr[0];
>-              else
>+              else if (!editable || !value)
>                 this.selectedIndex = 0;
That's definitely an improvement on the current code, although I'm pondering that editable menulists should never enforce an initial selection.

>+      <property name="editable"  onset="this.setAttribute('editable',val); return val;"
>+                                 onget="return this.getAttribute('editable') == 'true';"/>
Personally I'd prefer type="editable" and a readonly property, but at least we ought to sync xul.css with this definition?

>-            if (popup && 0 <= val && val < popup.childNodes.length)
tabbox.xml also checks for 0 .. length; are you proposing to change that too?

>+            if (val) {
>+              if ("control" in val) {
>+                if (val.control != this)
>+                  return oldval;
>+              }
Are you going to be adding these checks to all widgets? And I'm not convinced that you should define the property if you can't reliably use it here (e.g. trying to set the selected item to an element not in the document).

>+            if (oldval != val) {
We know oldval != val here. Interestingly radio groups always fire select events when someone calls the selected item setter.

>+          if (!this.mSelectedInternal)
>+            this.selectedItem = item;
Is this wise? I'm sure it will break existing code.

>-          if (popup)
>-            this.removeChild(popup);
>+          if (popup) {
>+            while (popup.firstChild)
>+              popup.removeChild(popup.firstChild);
>+          }
Why this change?

>-                    xbl:inherits="value=label,disabled,tabindex"/>
>+                    xbl:inherits="value=label,value,disabled,tabindex,readonly"/>
Heh, a readonly editable menulist - makes sense :-P

>+            if (val && val.control != this)
>+              return oldval;
Not the extensive check above, then?

>             this.setSelectionInternal(val);
>-
>-            if (!this.selectedInternal) {
>+            if (this.mSelectedInternal) {
You changed this to if (val) above.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 217447 [details] [diff] [review]
Fix various issues

I'll take a look once Neil's comments are addressed

Comment 5

[Neil Deakin]

Attachment: Address review comments

Comment 6

[[e-mail]]

Comment on attachment 218847 [details] [diff] [review]
Address review comments

>+      <property name="control" readonly="true">
Is there some grand design that explains this new property?

>+            var parent = this.parentNode;
>+            if (parent &&
>+                parent.parentNode instanceof Components.interfaces.nsIDOMXULSelectControlElement)
>+              return parent;
Surely this should return parent.parentNode;

>+              else if (!editable || !value)
>                 this.selectedIndex = 0;
Still worried that this should be just !editable

>+      <method name="contains">
>+        <parameter name="item"/>
>+        <body>
>+          <![CDATA[
>+            if (!item)
>+              return false;
Nit: both callers already null-check

>-            var popup = this.menupopup;
>-            if (popup && 0 <= val && val < popup.childNodes.length)
>-              this.selectedItem = popup.childNodes[val];
>-            else
>+            if (val < 0) {
>               this.selectedItem = null;
>+            }
>+            else {
>+              var popup = this.menupopup;
>+              if (popup && val < popup.childNodes.length)
>+                this.selectedItem = popup.childNodes[val];
>+            }
I'd still like to know the justification for this change too.

>+              return oldval;
Nit: aren't setters supposed to always return val; ?

Comment 7

[Neil Deakin]

(In reply to comment #6)
> (From update of attachment 218847 [details] [diff] [review] [edit])
> >+      <property name="control" readonly="true">
> Is there some grand design that explains this new property?
> 
It's declared in the nsIDOMXULSelectControlItemElement interface which this element implements.

> >+              return parent;
> Surely this should return parent.parentNode;

Yes, it should.

> >+              else if (!editable || !value)
> >                 this.selectedIndex = 0;
> Still worried that this should be just !editable

Makes sense. I'll change it.

> >+      <method name="contains">
> >+        <parameter name="item"/>
> >+        <body>
> >+          <![CDATA[
> >+            if (!item)
> >+              return false;
> Nit: both callers already null-check

Someone else might call it though.

> >-            var popup = this.menupopup;
> >-            if (popup && 0 <= val && val < popup.childNodes.length)
> >-              this.selectedItem = popup.childNodes[val];
> >-            else
> >+            if (val < 0) {
> >               this.selectedItem = null;
> >+            }
> >+            else {
> >+              var popup = this.menupopup;
> >+              if (popup && val < popup.childNodes.length)
> >+                this.selectedItem = popup.childNodes[val];
> >+            }
> I'd still like to know the justification for this change too.

None, really. I can change it back.

> Nit: aren't setters supposed to always return val; ?

Fixed.

Comment 8

[Neil Deakin]

Attachment: Updated patch addressing Neil's review comments

Comment 9

[Sebastian Winkler]

(In reply to comment #2)
> Good idea, valuechange should be fired when it changes to null as well.

But shouldn't it only by fired by actual user input?
Right now, unlike a textbox's change event for example, it fires also as a result of manipulation through scripting.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 239697 [details] [diff] [review]
Updated patch addressing Neil's review comments


>   <binding id="menulist-editable" extends="chrome://global/content/bindings/menulist.xml#menulist">
...
>-    <implementation>
>-      <constructor><![CDATA[
>-        this.setInitialSelection();
>-      ]]></constructor>
> 
>+    <implementation>

Shouldn't you set the implements attribute again here (to "nsIDOMXULMenuListElement, nsIAccessibleProvider")?

r=mano with this addressed.

Comment 11

[Neil Deakin]

> Shouldn't you set the implements attribute again here (to
> "nsIDOMXULMenuListElement, nsIAccessibleProvider")?
> 
> r=mano with this addressed.
> 

It inherits from the base binding.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> > Shouldn't you set the implements attribute again here (to
> > "nsIDOMXULMenuListElement, nsIAccessibleProvider")?
> > 
> > r=mano with this addressed.
> 
> It inherits from the base binding.

Are you sure? I vaguely recall that not being the case, and bug 347219 comment 19 seems to back that up.

Comment 13

[Asaf Romano (gone)]

The implementation is, of course, but IIRC the implements attribute isn't inherited once you extend the implementation.

Comment 14

[Neil Deakin]

No, as http://xulplanet.com/ndeakin/tests/xts/menulist/menulist-interfaces.xul demonstrates. The issue in bug 347219 was because control-item doesn't even implement nsIDOMXULLabeledControlElement, which is probably should.

Comment 15

[Aaron Leventhal]

(In reply to comment #14)
> No, as http://xulplanet.com/ndeakin/tests/xts/menulist/menulist-interfaces.xul
> demonstrates. The issue in bug 347219 was because control-item doesn't even
> implement nsIDOMXULLabeledControlElement, which is probably should.

Right, I was thinking that too. Not sure why it doesn't. The things that inherit from it have to, which seems odd.