Last Comment Bug 333023 - Menulist/menu cleanup
: Menulist/menu cleanup
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
Depends on: 354805
Blocks: 336863 344487 324230
  Show dependency treegraph
 
Reported: 2006-04-06 10:39 PDT by Neil Deakin
Modified: 2008-07-31 03:20 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix various issues (47.90 KB, patch)
2006-04-06 10:41 PDT, Neil Deakin
neil: superreview-
Details | Diff | Review
Address review comments (47.32 KB, patch)
2006-04-18 10:43 PDT, Neil Deakin
neil: superreview+
Details | Diff | Review
Updated patch addressing Neil's review comments (43.59 KB, patch)
2006-09-22 13:02 PDT, Neil Deakin
asaf: review+
Details | Diff | Review

Description Neil Deakin 2006-04-06 10:39:30 PDT
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 2006-04-06 10:41:47 PDT
Created attachment 217447 [details] [diff] [review]
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 2006-04-06 11:19:31 PDT
Good idea, valuechange should be fired when it changes to null as well.
Comment 3 neil@parkwaycc.co.uk 2006-04-07 02:14:57 PDT
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] 2006-04-10 09:18:16 PDT
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 2006-04-18 10:43:52 PDT
Created attachment 218847 [details] [diff] [review]
Address review comments
Comment 6 neil@parkwaycc.co.uk 2006-04-18 15:59:17 PDT
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 2006-04-19 08:30:13 PDT
(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 2006-09-22 13:02:43 PDT
Created attachment 239697 [details] [diff] [review]
Updated patch addressing Neil's review comments
Comment 9 Sebastian Winkler 2006-09-25 08:24:26 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-27 16:12:36 PDT
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 2006-09-28 07:37:39 PDT
> 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: gavin@gavinsharp.com] 2006-09-28 08:19:31 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-28 08:30:57 PDT
The implementation is, of course, but IIRC the implements attribute isn't inherited once you extend the implementation.
Comment 14 Neil Deakin 2006-09-28 08:58:00 PDT
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 2006-09-28 10:22:50 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.