Status

()

Core
XUL
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Neil Deakin (mostly unavailable until September), Assigned: Neil Deakin (mostly unavailable until September))

Tracking

(Blocks: 2 bugs)

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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
Attachment #217447 - Flags: superreview?(neil)
Attachment #217447 - Flags: review?
Attachment #217447 - Flags: review? → review?(mconnor)

Comment 2

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

Comment 3

12 years ago
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)
Created attachment 218847 [details] [diff] [review]
Address review comments
Attachment #217447 - Attachment is obsolete: true
Attachment #218847 - Flags: superreview?(neil)
Attachment #218847 - Flags: review?(mconnor)

Comment 6

12 years ago
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: 336863
Blocks: 344487

Updated

11 years ago
Blocks: 324230
Created attachment 239697 [details] [diff] [review]
Updated patch addressing Neil's review comments
Attachment #218847 - Attachment is obsolete: true
Attachment #239697 - Flags: review?(mano)
Attachment #218847 - Flags: review?(mconnor)

Comment 9

11 years ago
(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
Last Resolved: 11 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.

Comment 15

11 years ago
(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

Updated

9 years ago
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.