Opened 10 years ago

Closed 10 years ago

#9499 closed defect (fixed)

NodeList isn't picking up some native methods (FF3.5)

Reported by: dante Owned by: alex
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0
Keywords: Cc: Eugene Lazutkin
Blocked By: Blocking:

Description (last modified by bill)

This may be a FF3.5 regression, but:

dojo.query("a").sort(function(a,b){ return 1; })

works in FF3.1, but throws errors in FF3.5 final.

This is breaking SortList, but is a generalized regression.

.reverse() displays the same behavior.

Attachments (1)

nodelistsort.html (1.1 KB) - added by James Burke 10 years ago.
Test file, place as a sibling to dojo/dijit/dojox.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by bill

Description: modified (diff)
Milestone: tbd1.3.2
Summary: NodeList isn't picking up some native methodsNodeList isn't picking up some native methods (FF3.5)

Sounds like this should be fixed for the 1.3.2 release? Marking for consideration.

There should also be a unit test for it, if there isn't one already. I saw failures in the dojo.query() tests but I'm not sure if they were from this problem or something else.

comment:2 Changed 10 years ago by James Burke

Looks like document.querySelectorAll("a") also has the same problem. If I force nozip = false for the found nodes inside of query.js, then sort works.

Changed 10 years ago by James Burke

Attachment: nodelistsort.html added

Test file, place as a sibling to dojo/dijit/dojox.

comment:3 Changed 10 years ago by James Burke

Alex suggested testing with forcing useQSA off to see if it is a language vs. a QSA issue. Forcing useQSA to false means the error does not show up.

I attached a test file to make it easier to talk about the same things.

I am fairly ignorant of this area, but here is what it feels like: QSA returns a DOM NodeList? object, which, like getElementsByTagName() do not have array prototype methods (using document.getElementsByTagName("a").sort() also fails in the web page).

It feels like instead of wrapping the code in an array, that we then mixin dojo.NodeList? methods, query is returning a NodeList? object, which has a length and such but is not an actual array.

Alex indicated that nozip = false should just bypass a second scan of an array that query already built. It feels like though we are not converting the QSA NodeList? result into an array though.

For instance, both of these are true in FF 3.5:

document.querySelectorAll("a") instanceof NodeList
dojo.query("a") instanceof NodeList

but both of these return false:

document.querySelectorAll("a") instanceof Array
dojo.query("a") instanceof Array

comment:4 Changed 10 years ago by Eugene Lazutkin

It looks like we may start decorating native NodeList as well. I hope whatever browsers return NodeList form dojo.query() can tolerate non-native methods on it without memory leaks.

comment:5 Changed 10 years ago by alex

First, a couple of notes:

  • dojo.NodeList? instances should ALWAYS be "real" arrays
  • the query system today tries to wrap things in NodeList? dressing if they're arrays, but should pass them to the dojo.NodeList? ctor if they aren't.

I think our issue here is that the NodeList? ctor isn't detecting whatever FF3.5 is returning correctly.

comment:6 Changed 10 years ago by alex

Status: newassigned

comment:7 in reply to:  5 Changed 10 years ago by Eugene Lazutkin

Replying to alex:

I think our issue here is that the NodeList? ctor isn't detecting whatever FF3.5 is returning correctly.

Do you propose to copy elements to new array? Or return the original NodeList decorated with additional methods?

comment:8 Changed 10 years ago by alex

Resolution: fixed
Status: assignedclosed

fixed in [18751]

comment:9 Changed 10 years ago by alex

(In [18752]) missed a spurious change in the last CL. Refs #9499

!strict

comment:10 Changed 10 years ago by alex

(In [18754]) Fixes #9499 on the 1.3 branch.

!strict

comment:11 Changed 10 years ago by dante

Resolution: fixed
Status: closedreopened

Unit test in place in 1.3.2 tag fails in Safari4

[ Error: Result of expression 'i.sort' [undefined] is not a function. ]
ERROR IN: {sort}
FAILED test: sort 1 ms

also been reported broken in FF3.5, but one failing test is enough to reopen

comment:12 Changed 10 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added

comment:13 Changed 10 years ago by James Burke

Resolution: worksforme
Status: reopenedclosed

I do not see this happening with the dojo.tests._base.query tests in trunk. There is a problem with :checked in Safari 4, but I believe that is tracked by another ticket. Similarly, FF 3.5 is not failing on the .sort test.

comment:14 Changed 10 years ago by Irfan Ahmed

Resolution: worksforme
Status: closedreopened

I have been using dojo 1.3.2 with firefox 3.0.5 and this works. However I moved our application for Firefox 3.5.4 and the application is broken because of this bug. The sort method is still undefined in 1.3.2 NodeList?

TestCase?:

Access the following URL in FF3.5.4 and have Firebug installed and open. http://download.dojotoolkit.org/release-1.3.2/dojo-release-1.3.2/dojo/tests/_base/query.html

The output is .. ==================================================== PASSED test: xml_attrs 1 msbootstrap.js (line 641) TypeError?: i.sort is not a function message=i.sort is not a functionbootstrap.js (line 641) ERROR IN: (function sort() {var i = dojo.query("div");i.sort(function (a, b) {return 1;});})bootstrap.js (line 641) FAILED test: sort 1 msbootstrap.js (line 641) Total time for GROUP " t " is 41 msbootstrap.js (line 641) Total time for GROUP " t " is 41 msbootstrap.js (line 641) WOOHOO!!bootstrap.js (line 641)


| TEST SUMMARY:bootstrap.js (line 641)


108 tests in 1 groupsbootstrap.js (line 641) 1 errorsbootstrap.js (line 641) 0 failures

comment:15 Changed 10 years ago by dante

Milestone: 1.3.21.4
Resolution: fixed
Status: reopenedclosed

This was not fixed in the branch, the current reports of fixage are based on trunk/1.4 ... I've confirmed 1.4.0b2 + FF 3.5 | Safari 4 both work with the current code.

There will be no 1.3.3, so no need to back port fix for 1.3 branch.

Note: See TracTickets for help on using tickets.