Make listbox.selectedItems be an nsIDOMNodeList

RESOLVED FIXED in mozilla45

Status

()

defect
RESOLVED FIXED
18 years ago
2 years ago

People

(Reporter: mozilla, Assigned: smaug)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla45
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

The following code  results in a JS error:

  nsCOMPtr<nsIDOMNodeList> selectedItems;
  nsCOMPtr<nsIDOMXULMultiSelectControlElement> listbox
(do_QueryInterface(mDOMNode));
  if (listbox) {
    listbox->GetSelectedItems(getter_AddRefs(selectedItems));
    if (selectedItems) {
      PRUint32 length = 0;
      selectedItems->GetLength(&length);
      for ( PRInt32 i = 0 ; i < length ; i++ ) {
        nsCOMPtr<nsIAccessible> tempAccessible;
        nsCOMPtr<nsIDOMNode> tempNode;
        selectedItems->Item(i, getter_AddRefs(tempNode));


The call to Item gets a JS error complaining that the method does not exist in
that object. The if() passes and the call to Length() succeeds too.

There is a comment in the nsIDOMXULMultSelectCntlEl.idl about waiting to get
nsIDOMNodeList scriptable and mutable for the SelectedItems property. So I guess
the problem lies in the nsIDOMNodeList impl. 

filing on xpconnect because I can get an nsIDOMNodeList back from a straight C++
call and it works fine. Seems like a problem in the JS-to-C++ conversion
somewhere ( even it is just in the way nsIDOMNodeList is put together )
From what I can tell, selectedItems is represented by a JS Array. Thus it
doesn't have an Item method, and doesn't truly implement an nsIDOMNodeList.
In the code where I exposed this we have a workaround. So the priority I guess
is pretty low. Not exactly sure what should get fixed here. IIRC someone had
said the nsIDOMNodeList just wasn't completely implemented from the JS
side...but I probably don't remember correctly. ;-)
So it sounds more like a DOM issue than an XPConnect issue correct, and probably
should be reassigned?
The problem is that selectedItems is implemented in XBL:
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/l
istbox.xml#40
It is a pure JS array, thus it has a length method, but no item() method. It is 
possible to have a NodeList represented as a JS array (using DOMClassInfo), but 
I'm not sure it is possible to have a JS array implemented as a NodeList.
A solution would be to declare the selectedItems property of the listbox 
binding as its own binding, which could then implement a custom .item() method, 
thus emulating a NodeList.
Thoughts?
But this has nothing to do with XPConnect, does it? 
Shouldn't this be reassigned to DOM Level 0?
I don't know actually... perhaps XPConnect could internally convert the call to
.item() to a js array call. I know next to nothing about how XBL interacts with
XPConnect/DOM, sorry :(
If someone wants to make a case for adding exposing attributes of an array as
XPCOM methods that could be mapped to interfaces, then another bug would
probably be in order.

Couldn't you add a JavaScript function named item to the array?

anArray.item = function(index) { return this[index]; }
dbradley, yes that's another solution.
It seems reasonable to me to expect a complete implementation of the interface
from the XBL implementor (in this case, the listbox binding), just like a c++
implementation has to provide a complete implementation of the interface.
(I don't think it's reasonable to expect XPConnect to "complete" an incomplete
XBL/JS implementation)
So I guess this bug should go to whoever is in charge of listbox.xml/tree.xml,
if my reasoning is correct.
cc'ing jband for advice -
I don't see a reason to hack xpconnect here.
Over to widgets
Assignee: dbradley → rods
Component: XPConnect → XP Toolkit/Widgets
Setting default QA -
QA Contact: pschwartau → jrgm
seems form controls related
Assignee: rods → jkeiser
-> default owner.  This is not HTML form controls at least.

Changing XPConnect for this case seems unnecessary and even bad.  Therefore
there are two options:
(1) have the XBL implement nsIDOMNodeList functions (this seems like a wonderful
idea)
(2) have the caller not use it as an nsIDOMNodeList since it *isn't* one.

I don't much care; but I would vote for solution (1).
Assignee: jkeiser → jaggernaut
Summary: js wrapped nsIDOMNodeList missing the Item method → Make listbox.selectedItems be an nsIDOMNodeList
jgaunt, if it's fairly easy for you, could you test this fix with the code you
wanted to use?
Uh, the code has already been changed and checked in for some time. I wouldn't
say it would be easy. And I really don't have the time immediately to test this,
but the exact code I was using is in the bug.
Assignee: jag → nobody
This bug breaks the code in XULListboxAccessible::SelectedRowIndices as explained in bug 994964 comment 73-79.  Specifically, |item| here <http://mxr.mozilla.org/mozilla-central/source/accessible/src/xul/XULListboxAccessible.cpp#471> is *always* going to be null, so effectively this method never returns correct results.  XULListboxAccessible::SelectedCells and XULListboxAccessible::SelectedCellIndices are similarly affected.

accessible/tests/mochitest/table/test_sels_listbox.xul exercises this code but I didn't bother checking why it passes.  CCing you guys in case you care enough to continue to investigate.
Actually I did end up investigating a bit more, and the caller of SelectedRowIndices (xpcAccessibleTable::GetSelectedRowIndices) just ends up creating and returning a zero-length array.  The reason that the test doesn't catch this is a bug in the test: <http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_sels_listbox.xul#122>.  Here when we try to compare the array entries with what we expect, we loop over based on selCols.length, which of course would be 0 in this case so we don't end up examining any array elements.  If I change that loop condition to use aSelIndexesArray.length which is what it should use, the test would fail as I expected.
yzen, eejay, Marco want to take it? you just need to make the listbox.xml thing return something implementing nsIDOMNodeList, and there aren't' many 1xxxxx bugs left ;)
Flags: needinfo?(gijskruitbosch+bugs)
Posted patch chrome_ctor_for_nodelist.diff (obsolete) — Splinter Review
Make it possible to create NodeList objects from JS
(I think having just ctor is enough. no methods for modifying the list)
(In reply to Olli Pettay [:smaug] from comment #20)
> Created attachment 8677771 [details] [diff] [review]
> chrome_ctor_for_nodelist.diff
> 
> Make it possible to create NodeList objects from JS
> (I think having just ctor is enough. no methods for modifying the list)

Why not? This will mean that every time the selection changes in a multiple-select listbox, we have to construct a new NodeList on the JS side. That's not going to be fun if the number of selected items is high...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)
Actually, maybe this is a dumb question, but: can't the JS side of this just implement something that's an nsIDOMNodeList in JS? It's not a particularly large interface, so it wouldn't be that much overhead...
We want to make nsIDOMNodeList a builtinclass (and eventually drop nsIDOMNodeList and have just the webidl NodeList). Anyhow, I'm actually adding ChromeNodeList which has a ctor and way to append and remove nodes so that "live" nodelist is possible. That is that listbox seems to require after all.
Flags: needinfo?(bugs)
Attachment #8677771 - Attachment is obsolete: true
This adds a ChromeOnly ChromeNodeList so that chrome js can create and modify a proper NodeList object. xul:listbox uses currently a JS Array for that, but obviously that doesn't work with C++ a11y code which expects the interface to return a proper nodelist.
The implementation is pretty much the smallest possible to fulfill the requirements listbox has.
Attachment #8678163 - Flags: review?(amarchesini)
(that patch obviously is missing all the listbox changes)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Olli Pettay [:smaug] from comment #26)
> (that patch obviously is missing all the listbox changes)

...which Gijs will do
Not sure what to do with this:

https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/xul/nsIDOMXULMultSelectCntrlEl.idl#29

Trevor, can you advise?

The basic implementation in the XUL thing isn't difficult, but updating callers is "interesting" and might have add-on impact.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(tbsaunde+mozbugs)
I believe this works, but considering the hacks I already saw, I guess only try and time will tell...
Attachment #8678917 - Flags: review?(dao)
Comment on attachment 8678163 [details] [diff] [review]
chromenodelist.diff (some more tests)

Review of attachment 8678163 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ChromeNodeList.cpp
@@ +10,5 @@
> +using namespace mozilla;
> +using namespace mozilla::dom;
> +
> +already_AddRefed<ChromeNodeList>
> +ChromeNodeList::Constructor(const mozilla::dom::GlobalObject& aGlobal,

drop mozilla::dom::

@@ +11,5 @@
> +using namespace mozilla::dom;
> +
> +already_AddRefed<ChromeNodeList>
> +ChromeNodeList::Constructor(const mozilla::dom::GlobalObject& aGlobal,
> +                            mozilla::ErrorResult& aRv)

mozilla::

::: dom/base/ChromeNodeList.h
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class ChromeNodeList : public nsSimpleContentList

final?

@@ +18,5 @@
> +  {
> +  }
> +
> +  static already_AddRefed<ChromeNodeList>
> +  Constructor(const mozilla::dom::GlobalObject& aGlobal,

1. drop mozilla::dom::
2. I think you must declare/forward declare GlobalObject and ErrorResult here.

@@ +19,5 @@
> +  }
> +
> +  static already_AddRefed<ChromeNodeList>
> +  Constructor(const mozilla::dom::GlobalObject& aGlobal,
> +              mozilla::ErrorResult& aRv);

and mozilla::
Attachment #8678163 - Flags: review?(amarchesini) → review+
Keywords: leave-open
(In reply to :Gijs Kruitbosch from comment #28)
> Not sure what to do with this:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/xul/
> nsIDOMXULMultSelectCntrlEl.idl#29
> 
> Trevor, can you advise?
> 
> The basic implementation in the XUL thing isn't difficult, but updating
> callers is "interesting" and might have add-on impact.

personally I'd just drop the comment.  The methods might not be strictly required anymore (I wasn't even aware they existed), but they don't really hurt right? so I don't see any reason to bother clean them up.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8678917 - Flags: review?(dao) → review+
So unfortunately the trypush here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da7cab5ee779

has reftest orange.

When run locally, I see:

 0:02.95 JavaScript error: chrome://global/content/bindings/listbox.xml, line 177: ReferenceError: ChromeNodeList is not defined


That makes sense because the constructor is ChromeOnly, and reftests don't run with chrome privileges.

Unfortunately, that means that the binding is not guaranteed access to it in its enclosing window. We don't currently use listboxes in content-privileged code, that I can tell, but it's not easy to be 100% sure about that, and it could change...

I see a number of options:
1) find some way to expose the constructor to XBL
2) disable the test and move on
3) somehow make the test run with chrome privileges (rewrite as a mochitest, essentially, AIUI, because reftests never have chrome privileges, from what I can tell on #developers)
4) add fallback code to the binding that uses an array with append() and remove() polyfilled, if ChromeNodeList is not available (thus regressing the thing this bug aims to fix in those circumstances).

Olli/Dão, what makes the most sense to you?
Flags: needinfo?(dao)
Flags: needinfo?(bugs)
(FWIW, I actually think it would make more sense to have XUL tests run with chrome privileges, because now that we have chrome-only CSS properties this problem is only going to get worse.)
(In reply to :Gijs Kruitbosch from comment #37)
> 4) add fallback code to the binding that uses an array with append() and
> remove() polyfilled, if ChromeNodeList is not available (thus regressing the
> thing this bug aims to fix in those circumstances).

What motivated the recent activity in this bug in the first place? What code requires selectedItems to be an nsIDOMNodeList rather than a JS array?
Flags: needinfo?(dao)
C++ Accessibility code.
Flags: needinfo?(bugs)
(In reply to :Gijs Kruitbosch from comment #37)
 > I see a number of options:
> 1) find some way to expose the constructor to XBL
I thought I wrote a comment about [Func="IsChromeOrXBL"]

So, does replacing ChromeOnly with [Func="IsChromeOrXBL"] help?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Olli Pettay [:smaug] from comment #41)
> (In reply to :Gijs Kruitbosch from comment #37)
>  > I see a number of options:
> > 1) find some way to expose the constructor to XBL
> I thought I wrote a comment about [Func="IsChromeOrXBL"]
> 
> So, does replacing ChromeOnly with [Func="IsChromeOrXBL"] help?

Yes. Landed it with that updated.
Flags: needinfo?(gijskruitbosch+bugs)
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Depends on: 1221962
Looks like some mdn pages should be updated. In particular,

* https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/listbox#p-selectedItems

* NodeList.append() and NodeList.remove() isn't there at present (https://developer.mozilla.org/en-US/docs/Web/API/NodeList). I'm not sure how standard these methods are intended to be.

Should this change be flagged for mentioning to addon authors?

It might have been nice if .append() had been called .push(), for extra backwards Array compatibility?
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: dev-doc-needed
(In reply to aleth [:aleth] from comment #48)
> Looks like some mdn pages should be updated. In particular,
> 
> *
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/listbox#p-
> selectedItems
indeed. looks like that doc has been wrong, given that selectedItems is about
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/xul/nsIDOMXULMultSelectCntrlEl.idl
where the attribute is defined to be 'readonly attribute nsIDOMNodeList selectedItems;'

> 
> * NodeList.append() and NodeList.remove() isn't there at present
> (https://developer.mozilla.org/en-US/docs/Web/API/NodeList). I'm not sure
> how standard these methods are intended to be.
append or remove were not added to NodeList. They are methods of ChromeNodeList, which is totally unstandard, just like the whole listbox is.
https://developer.mozilla.org/en-US/docs/Web/API/NodeList must not be updated because of the changes in this bud.


> 
> Should this change be flagged for mentioning to addon authors?
Hmm, possibly

> It might have been nice if .append() had been called .push(), for extra
> backwards Array compatibility?
There is still slight chance that NodeList will become ArrayClass, http://heycam.github.io/webidl/#LegacyArrayClass , so I wouldn't add any Array-like methods to NodeList for now.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1222797
Keywords: addon-compat
Assignee: nobody → bugs
You need to log in before you can comment on or make changes to this bug.