John Gaunt (redfive) Reporter
	Description • 23 years ago

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 )

	David Bradley
	Comment 1 • 23 years ago

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.

	John Gaunt (redfive) Reporter
	Comment 2 • 23 years ago

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. ;-)

	David Bradley
	Comment 3 • 23 years ago

So it sounds more like a DOM issue than an XPConnect issue correct, and probably
should be reassigned?

	Fabian Guisset
	Comment 4 • 23 years ago

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?

	Phil Schwartau
	Comment 5 • 23 years ago

But this has nothing to do with XPConnect, does it? 
Shouldn't this be reassigned to DOM Level 0?

	Fabian Guisset
	Comment 6 • 23 years ago

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 :(

	David Bradley
	Comment 7 • 23 years ago

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]; }

	Fabian Guisset
	Comment 8 • 23 years ago

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.

	Phil Schwartau
	Comment 9 • 23 years ago

cc'ing jband for advice -

	John Bandhauer
	Comment 10 • 23 years ago

I don't see a reason to hack xpconnect here.

	David Bradley
	Comment 11 • 22 years ago

Over to widgets

	Phil Schwartau
	Comment 12 • 22 years ago

Setting default QA -

	rods (gone)
	Comment 13 • 22 years ago

seems form controls related

	John Keiser (jkeiser)
	Comment 14 • 22 years ago

-> 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).

	jag (Peter Annema)
	Comment 15 • 22 years ago

jgaunt, if it's fairly easy for you, could you test this fix with the code you
wanted to use?

	John Gaunt (redfive) Reporter
	Comment 16 • 22 years ago

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.

	(no longer active)
	Comment 17 • 10 years ago

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.

	(no longer active)
	Comment 18 • 10 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 19 • 10 years ago

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 ;)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 20 • 9 years ago

Make it possible to create NodeList objects from JS
(I think having just ctor is enough. no methods for modifying the list)

	:Gijs (he/him)
	Comment 21 • 9 years ago

(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...

	:Gijs (he/him)
	Comment 22 • 9 years ago

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...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 23 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 25 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 26 • 9 years ago

(that patch obviously is missing all the listbox changes)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 27 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #26)
> (that patch obviously is missing all the listbox changes)

...which Gijs will do

	:Gijs (he/him)
	Comment 28 • 9 years ago

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.

	:Gijs (he/him)
	Comment 29 • 9 years ago

I believe this works, but considering the hacks I already saw, I guess only try and time will tell...

	Andrea Marchesini [:baku]
	Comment 30 • 9 years ago

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::

	Pulsebot
	Comment 33 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3212ad29e2

	Pulsebot
	Comment 34 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c3036239c2

	Carsten Book [:Tomcat]
	Comment 35 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/bb3212ad29e2
https://hg.mozilla.org/mozilla-central/rev/57c3036239c2

	Trevor Saunders (:tbsaunde)
	Comment 36 • 9 years ago

(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.

	:Gijs (he/him)
	Comment 37 • 9 years ago

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?

	:Gijs (he/him)
	Comment 38 • 9 years ago

(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.)

	Dão Gottwald [:dao]
	Comment 39 • 9 years ago

(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?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 40 • 9 years ago

C++ Accessibility code.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 41 • 9 years ago

(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?

	Pulsebot
	Comment 42 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/fb998e8c0b94

	:Gijs (he/him)
	Comment 43 • 9 years ago

(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.

	Pulsebot
	Comment 44 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/2661970e2de2

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 45 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fb998e8c0b94
https://hg.mozilla.org/mozilla-central/rev/2661970e2de2

	Carsten Book [:Tomcat]
	Comment 46 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fb998e8c0b94
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2661970e2de2

	Carsten Book [:Tomcat]
	Comment 47 • 9 years ago

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

	aleth [:aleth]
	Comment 48 • 9 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 49 • 9 years ago

(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.