Closed Bug 333023 Opened 18 years ago Closed 18 years ago

Menulist/menu cleanup

Categories

(Core :: XUL, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Fix various issues (obsolete) — Splinter Review
- 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
Attachment #217447 - Flags: superreview?(neil)
Attachment #217447 - Flags: review?
Attachment #217447 - Flags: review? → review?(mconnor)
Good idea, valuechange should be fired when it changes to null as well.
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.
Attachment #217447 - Flags: superreview?(neil) → superreview-
Comment on attachment 217447 [details] [diff] [review]
Fix various issues

I'll take a look once Neil's comments are addressed
Attachment #217447 - Flags: review?(mconnor)
Attached patch Address review comments (obsolete) — Splinter Review
Attachment #217447 - Attachment is obsolete: true
Attachment #218847 - Flags: superreview?(neil)
Attachment #218847 - Flags: review?(mconnor)
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; ?
Attachment #218847 - Flags: superreview?(neil) → superreview+
(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.
Blocks: 344487
Blocks: 324230
Attachment #218847 - Attachment is obsolete: true
Attachment #239697 - Flags: review?(mano)
Attachment #218847 - Flags: review?(mconnor)
(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 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.
Attachment #239697 - Flags: review?(mano) → review+
> Shouldn't you set the implements attribute again here (to
> "nsIDOMXULMenuListElement, nsIAccessibleProvider")?
> 
> r=mano with this addressed.
> 

It inherits from the base binding.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(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.
The implementation is, of course, but IIRC the implements attribute isn't inherited once you extend the implementation.
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.
(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.
Depends on: 354805
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.