120684 - Make listbox.selectedItems be an nsIDOMNodeList

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[John Gaunt (redfive)]

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 )

Comment 1

[David Bradley]

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.

Comment 2

[John Gaunt (redfive)]

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

Comment 3

[David Bradley]

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

Comment 4

[Fabian Guisset]

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?

Comment 5

[Phil Schwartau]

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

Comment 6

[Fabian Guisset]

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

Comment 7

[David Bradley]

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

Comment 8

[Fabian Guisset]

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.

Comment 9

[Phil Schwartau]

cc'ing jband for advice -

Comment 10

[John Bandhauer]

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

Comment 11

[David Bradley]

Over to widgets

Comment 12

[Phil Schwartau]

Setting default QA -

Comment 13

[rods (gone)]

seems form controls related

Comment 14

[John Keiser (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).

Comment 15

[jag (Peter Annema)]

Attachment: This should fix it.

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

Comment 16

[John Gaunt (redfive)]

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.

Comment 17

[(no longer active)]

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.

Comment 18

[(no longer active)]

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.

Comment 19

[Trevor Saunders (:tbsaunde)]

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

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 21

[:Gijs (out for now; he/him)]

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

Comment 22

[:Gijs (out for now; he/him)]

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

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: chromenodelist.diff

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: chromenodelist.diff (some more tests)

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.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(that patch obviously is missing all the listbox changes)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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

...which Gijs will do

Comment 28

[:Gijs (out for now; he/him)]

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.

Comment 29

[:Gijs (out for now; he/him)]

Attachment: use ChromeNodeList in toolkit's listboxes' selectedItems implementation,

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

Comment 30

[Andrea Marchesini [:baku]]

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

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Attachment: less namespace use

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: less namespace use

Comment 33

[Pulsebot]

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

Comment 34

[Pulsebot]

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

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

[Trevor Saunders (:tbsaunde)]

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

Comment 37

[:Gijs (out for now; he/him)]

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?

Comment 38

[:Gijs (out for now; he/him)]

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

Comment 39

[Dão Gottwald [:dao]]

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

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

C++ Accessibility code.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 42

[Pulsebot]

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

Comment 43

[:Gijs (out for now; he/him)]

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

Comment 44

[Pulsebot]

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

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 46

[Carsten Book [:Tomcat]]

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

Comment 47

[Carsten Book [:Tomcat]]

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

Comment 48

[aleth [:aleth]]

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?

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

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