Closed Bug 372552 Opened 17 years ago Closed 17 years ago

add interface for xul:menu similiar with nsIDOMXULSelectControlElement interface

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha3

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 4 obsolete files)

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?
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.
 
(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?
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.
(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.
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
(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.
(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.
The methods of nsIDOMXULContainerElement don't return the same types so it shouldn't be inherited. Only <menu> should implement nsIDOMXULContainerElement.
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?
Though at this point it's fine with me to have nsIDOMXULContainerElement for xul:menu only.
Assignee: nobody → surkov.alexander
Attached patch patch (obsolete) — Splinter Review
to approve
Attachment #259018 - Flags: first-review?(enndeakin)
Ah, forgot to implement parentContainer. Enn, if the patch approach works for you then I'll add implementation for parentContainer and test the patch.
Status: NEW → ASSIGNED
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.
Attachment #259018 - Flags: first-review?(enndeakin) → first-review-
Attached patch patch2 (obsolete) — Splinter Review
Attachment #259018 - Attachment is obsolete: true
Attachment #259078 - Flags: first-review?(enndeakin)
(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 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))
Attachment #259078 - Flags: first-review?(enndeakin) → first-review+
Attached patch patch3 (obsolete) — Splinter Review
Attachment #259078 - Attachment is obsolete: true
Attachment #259083 - Flags: second-review?(neil)
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.
Attachment #259083 - Flags: second-review?(neil) → second-review+
Attached patch patch4 (obsolete) — Splinter Review
with Neil's comment. Do I need additional r/sr for DOM changes?
Attachment #259083 - Attachment is obsolete: true
Attached patch patch5Splinter Review
for checking in
Attachment #259085 - Attachment is obsolete: true
Summary: problems of xul:menuitem and xul:menu → add interface for xul:menu similiar with nsIDOMXULSelectControlElement interface
Attached file testcase
checked in on trunk
I filed new bug 374610 for an ability to get xul:menu for xul:menuitem.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9alpha3
Version: unspecified → Trunk
Blocks: 389926
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.