Last Comment Bug 372552 - add interface for xul:menu similiar with nsIDOMXULSelectControlElement interface
: add interface for xul:menu similiar with nsIDOMXULSelectControlElement interface
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha3
Assigned To: alexander :surkov
:
Mentors:
Depends on: 415060
Blocks: 389926
  Show dependency treegraph
 
Reported: 2007-03-03 21:16 PST by alexander :surkov
Modified: 2008-02-25 07:56 PST (History)
4 users (show)
asqueella: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.86 KB, patch)
2007-03-19 11:45 PDT, alexander :surkov
enndeakin: first‑review-
Details | Diff | Splinter Review
patch2 (11.70 KB, patch)
2007-03-20 01:09 PDT, alexander :surkov
enndeakin: first‑review+
Details | Diff | Splinter Review
patch3 (11.70 KB, patch)
2007-03-20 04:39 PDT, alexander :surkov
neil: second‑review+
Details | Diff | Splinter Review
patch4 (11.63 KB, patch)
2007-03-20 05:27 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch5 (11.73 KB, patch)
2007-03-20 09:57 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
testcase (1.85 KB, application/vnd.mozilla.xul+xml)
2007-03-20 10:03 PDT, alexander :surkov
no flags Details

Description alexander :surkov 2007-03-03 21:16:46 PST
nsIDOMXULSelectControlItemElement::control for xul:menuitem returns null. xul:menu imlements nsIDOMXULSelectControlItemElement but control property retunrs null too. Probably xul:menu should implement nsIDOMXULSelectControlElement additionally to avoid these problems. Should it?
Comment 1 Neil Deakin (away until Oct 3) 2007-03-04 06:54:28 PST
Not clear what you're asking here. For <menuitems> in a <menulist>, the control property should return the menulist. For items not in a menulist, it should be null.

For <menu> elements, it should really return null all the time, although technically someone could put one in a <menulist> and .control would return the menulist.
 
Comment 2 alexander :surkov 2007-03-04 19:18:29 PST
(In reply to comment #1)
> For <menu> elements, it should really return null all the time.

That is the bug is for. Accessibility needs to know level of hierarchy, number of children on certain level of hierarchy, position of menuitem. I don't like to be depended on structure or tag names to find these things because often custom XBL can easy change structure or tag names. Therefore nsIDOMXULSelectControlElement and nsIDOMXULSelectControlItemElement interfaces are excellent thing to get all I want.

So I guess xul:menu may implement both nsIDOMXULSelectControlElement and nsIDOMXULSelectControlItemElement. It can implement nsIDOMXULSelectControlElement because it is container and methods of the interface makes sense for it. Am I right?
Comment 3 Neil Deakin (away until Oct 3) 2007-03-05 04:31:24 PST
No, menu should not implement nsIDOMXULSelectControlElement. That interface should only be used for elements where one of its items is selected.

XBLService::ResolveTag should be used to compare tags. You can get the menupopup for a menu using code like in nsContentUtils::FindFirstChildWithResolvedTag.
Comment 4 alexander :surkov 2007-03-05 06:04:19 PST
(In reply to comment #3)
> No, menu should not implement nsIDOMXULSelectControlElement. That interface
> should only be used for elements where one of its items is selected.

Ah, clear. So, probably we can introduce new interface for containers (and this interface I guess can be implemented by xul:menu)?

interface nsIDOMXULContainerControlElement : public nsIDOMXULControlElement {
  nsIDOMXULContainerControlElement appendItem(in DOMString label, in DOMString value);
  nsIDOMXULContainerControlElement insertItemAt(in long index, in DOMString label, in DOMString value);
  nsIDOMXULContainerControlElement removeItemAt(in long index);

  readonly attribute unsigned long itemCount;
  long getIndexOfItem(in nsIDOMXULSelectControlItemElement item);
  nsIDOMXULContainerControlElement getItemAtIndex(in long index);
}

interface nsIDOMXULSelectControlElement : public nsIDOMXULContainerControlElement {
}

I really don't like to depend in XUL on tag names. Interface is fine thing to require it.
Comment 5 Neil Deakin (away until Oct 3) 2007-03-17 06:06:21 PDT
Having an interface for menus sounds ok, but it should be:

interface nsIDOMXULContainerElement : public nsIDOMXULElement {
  nsIDOMXULElement appendItem(in DOMString label, in DOMString value);
  nsIDOMXULElement insertItemAt(in long index, in DOMString label, in DOMString value);
  nsIDOMXULElement removeItemAt(in long index);

  readonly attribute unsigned long itemCount;
  long getIndexOfItem(in nsIDOMXULElement item);
  nsIDOMXULElement getItemAtIndex(in long index);
}

A 'parentContainer' property might be useful too.

> 
> I really don't like to depend in XUL on tag names.
> 

Why not? It won't behave like a menu in xul unless the tag is xul:menu or the binding has an extends/display of xul:menu
Comment 6 alexander :surkov 2007-03-17 07:58:40 PDT
(In reply to comment #5)

> > I really don't like to depend in XUL on tag names.
> > 
> 
> Why not? It won't behave like a menu in xul unless the tag is xul:menu or the
> binding has an extends/display of xul:menu
> 

Yes, that's how xul is implemented mostly, but it brings problems when I like to have certain xul's element behaviour for another element like for XForms elements that are hosted in XUL document. Therefore I don't like to spread this approach everywhere where we can go without it.
Comment 7 alexander :surkov 2007-03-17 11:57:32 PDT
(In reply to comment #5)
> Having an interface for menus sounds ok, but it should be:
> 
> interface nsIDOMXULContainerElement : public nsIDOMXULElement {
>   nsIDOMXULElement appendItem(in DOMString label, in DOMString value);
>   nsIDOMXULElement insertItemAt(in long index, in DOMString label, in DOMString
> value);
>   nsIDOMXULElement removeItemAt(in long index);
> 
>   readonly attribute unsigned long itemCount;
>   long getIndexOfItem(in nsIDOMXULElement item);
>   nsIDOMXULElement getItemAtIndex(in long index);
> }

Is multiple inheritance for interfaces supported? It would be fine if

interface nsIDOMXULSelectControlElement : public nsIDOMXULControlElement
                                          public nsIDOMXULContainerElement 
{
};

it helps to keep changes easier (we don't need to change a lot binding for XUL elements).

> A 'parentContainer' property might be useful too.

Sounds good.
Comment 8 Neil Deakin (away until Oct 3) 2007-03-19 10:50:12 PDT
The methods of nsIDOMXULContainerElement don't return the same types so it shouldn't be inherited. Only <menu> should implement nsIDOMXULContainerElement.
Comment 9 alexander :surkov 2007-03-19 10:57:27 PDT
But returned object may be queried to needed type. Otherwise we have two similar interfaces, and isn't it too luxurious for xul:menu to have own interface that is called ContainerElement?
Comment 10 alexander :surkov 2007-03-19 10:59:01 PDT
Though at this point it's fine with me to have nsIDOMXULContainerElement for xul:menu only.
Comment 11 alexander :surkov 2007-03-19 11:45:22 PDT
Created attachment 259018 [details] [diff] [review]
patch

to approve
Comment 12 alexander :surkov 2007-03-19 11:47:59 PDT
Ah, forgot to implement parentContainer. Enn, if the patch approach works for you then I'll add implementation for parentContainer and test the patch.
Comment 13 Neil Deakin (away until Oct 3) 2007-03-19 12:28:39 PDT
Comment on attachment 259018 [details] [diff] [review]
patch

+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Foundation.

Not technically correct, but no big deal.

I'm assuming English isn't your first language, but there should be some prepositions (a/an/the)
in the comments, so that generated documentation will read better. I've put the changes in brackets below:

+  /**
+   * Creates [an] item for the given label and value and appends it to the container.
+   *
+   * @param aLabel - the [label] for [the] new item
+   * @param aValue - the value of [the] new item
+   */
+  nsIDOMXULElement appendItem(in DOMString aLabel, in DOMString aValue);
+
+  /**
+   * Creates [an] item for the given label and value and inserts it [into] the container
+   * at the specified position.
+   *
+   * @param aIndex - the index where [the] new item will be inserted
+   * @param aLabel - the [label] for [the] new item
+   * @param aValue - the value of [the] new item
+   */
+  nsIDOMXULElement insertItemAt(in long aIndex, in DOMString aLabel,
+                                in DOMString aValue);
+
+  /**
+   * Removes [an] item from the container.
+   *
+   * @param aIndex - the [index of the item to remove]
+   */
+  nsIDOMXULElement removeItemAt(in long aIndex);
+
+  /**
+   * Returns [a] count of items in the container.
+   */
+  readonly attribute unsigned long itemCount;
+
+  /**
+   * Returns [the] index of [an] item, [or -1 if the item is not in the container]
+   *
+   * @param aItem - the [item to determine the index of]
+   */
+  long getIndexOfItem(in nsIDOMXULElement aItem);
+
+  /**
+   * Returns [the] item [at a given] index [or null if the item is not is the container].
+   *
+   * @param aIndex - the index [of the item to return]
+   */
+  nsIDOMXULElement getItemAtIndex(in long aIndex);
+
+  /**
+   * Returns [the] parent container if any.
+   */
+  readonly attribute nsIDOMXULElement parentContainer;
+};

Index: toolkit/content/widgets/menu.xml
+      <method name="insertItemAt">
+        <parameter name="aIndex"/>
+        <parameter name="aLabel"/>
+        <parameter name="aValue"/>
+        <body>
+          if (!this.menupopup)
+            return;

Could you make this append a new menupopup as menulist does?

+
+          const XUL_NS =
+            "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
+
+          var menuitem = this.ownerDocument.createElementNS(XUL_NS, "menuitem");
+          menuitem.setAttribute("label", aLabel);
+          menuitem.setAttribute("value", aValue);
+
+          var item = this.getItemOfIndex(aIndex);
+          if (!item)
+            this.menupopup.appendChild(menuitem);
+          else
+            this.menupopup.insertBefore(menuitem, item);

OK, but maybe reverse the conditions so the code is similar to tabs.insertItemAt

+      <method name="removeItemAt">
+        <parameter name="aIndex"/>
+        <body>
+        <![CDATA[
+          var item = this.getItemAtIndex(aIndex);
+          if (item && this.menupopup)
+            this.menupopup.removeChild(item);
+        ]]>

removeItemAt should return the removed item.

+        </body>
+      </method>
+
+      <property name="itemsCount" readonly="true">

itemCount, not itemsCount

+      <method name="getItemAtIndex">
+        <parameter name="aIndex"/>
+        <body>
+          return this.menupopup ? this.menupopup.childNodes[aIndex] : -1;

This needs to do a bounds check and return -1 if index is < 0 or >= length.
You should be able to just copy the code for getIndexOfItem/getItemAtIndex from menulist.

+    <!-- private -->
+      <property name="menupopup" readonly="true">

There shouldn't a be a private comment there.

+        <getter>
+        <![CDATA[
+          if (!this._menupopup) {

Make sure to add a <field> for _menupopup.

The only other issue is that the code for the menupopup property won't catch menupopups defined inside a derived binding, but that isn't a big deal. The derived binding could always override .menupopup anyways.

Otherwise, it looks good.
Comment 14 alexander :surkov 2007-03-20 01:09:08 PDT
Created attachment 259078 [details] [diff] [review]
patch2
Comment 15 alexander :surkov 2007-03-20 01:13:41 PDT
(In reply to comment #13)
> (From update of attachment 259018 [details] [diff] [review])
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Foundation.
> 
> Not technically correct, but no big deal.

I'm not sure what should I write here. I guess I should ask Frank Hecker. But always I used this one where I didn't used 'Alexander Surkov'.

> 
> I'm assuming English isn't your first language, but there should be some
> prepositions (a/an/the)
> in the comments, so that generated documentation will read better. I've put the
> changes in brackets below:

Yes, thank you for changes.
> 
> The only other issue is that the code for the menupopup property won't catch
> menupopups defined inside a derived binding, but that isn't a big deal. The
> derived binding could always override .menupopup anyways.

Right now I can't see usecase for this. But I am agree anyone who creates own bidning can handle this himself.
Comment 16 Neil Deakin (away until Oct 3) 2007-03-20 04:34:04 PDT
Comment on attachment 259078 [details] [diff] [review]
patch2


>+      <property name="parentContainer" readonly="true">
>+        <getter>
>+          var parent = this;
>+          while (parent = parent.parentNode) {
>+            if (parent instanceOf Components.intefaces.nsIDOMXULContainerElement)
>+              return parent;

You've spelled 'interfaces' incorrectly and it should be 'instanceof'

Also, the condition should have double parantheses, otherwise a strict js warning will be given.

while ((parent = parent.parentNode))
Comment 17 alexander :surkov 2007-03-20 04:39:30 PDT
Created attachment 259083 [details] [diff] [review]
patch3
Comment 18 neil@parkwaycc.co.uk 2007-03-20 05:06:04 PDT
Comment on attachment 259083 [details] [diff] [review]
patch3

>+          var before = this.getItemOfIndex(aIndex);
getItemAtIndex, no?

>+          var item = this.getItemAtIndex(aIndex);
>+          if (item && this.menupopup)
item is hardly likely to exist if menupopup doesn't ;-)

>+          var parent = this;
>+          while ((parent = parent.parentNode)) {
My preference is for
for (var parent = this.parentNode; parent; parent = parent.parentNode)

>+      <field name="_menupopup">null</field>
I'm not sure caching the menupopup is a good idea.
Comment 19 alexander :surkov 2007-03-20 05:27:41 PDT
Created attachment 259085 [details] [diff] [review]
patch4

with Neil's comment. Do I need additional r/sr for DOM changes?
Comment 20 alexander :surkov 2007-03-20 09:57:08 PDT
Created attachment 259099 [details] [diff] [review]
patch5

for checking in
Comment 21 alexander :surkov 2007-03-20 10:03:38 PDT
Created attachment 259102 [details]
testcase
Comment 22 alexander :surkov 2007-03-20 10:04:48 PDT
checked in on trunk
Comment 23 alexander :surkov 2007-03-20 10:21:35 PDT
I filed new bug 374610 for an ability to get xul:menu for xul:menuitem.

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