Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#14555 closed defect (fixed)

[regression] dojo.NodeList.concat([]) is broken

Reported by: MaxMotovilov Owned by: Kris Zyp
Priority: high Milestone: 1.8
Component: Query Version: 1.7.0
Keywords: Cc:
Blocked By: Blocking:

Description

In Firebug console: (new dojo.NodeList()).concat([]) returns [ [ ] ]

This has worked properly (i.e. returned [], the way Array.concat does) in 1.6.x and ever before. Very unsettling issue to say the least.

Attachments (2)

NodeList_test_patch.patch (687 bytes) - added by bill 8 years ago.
update test script to test for this bug
query_js_patch.patch (1.7 KB) - added by Zakk Acreman 8 years ago.
Patch for concat() in query.js. Passes all of the concat tests, including bill's.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by bill

Summary: dojo.NodeList.concat is broken[regression] dojo.NodeList.concat([]) is broken

dojo/tests/_base/NodeList.html has a number of tests for concat(), but perhaps not this one.

comment:2 Changed 8 years ago by bill

Kris, this broke in your [25889] check in.

Changed 8 years ago by bill

Attachment: NodeList_test_patch.patch added

update test script to test for this bug

comment:3 Changed 8 years ago by Karl Tiedt

#15085 is a duplicate of this ticket.

comment:4 Changed 8 years ago by bill

Version: 1.7.11.7.0

Looks like this is broken in 1.7.

comment:5 Changed 8 years ago by Zakk Acreman

It's not just concat([]) that produces unexpected behavior. NodeList?.concat() is completely broken in Firefox 12, Chrome 19, and IE 9.

Instead of a unioned NodeList? object, concat() produces an array of separate NodeList? objects, which is useful for exactly nothing and, furthermore, breaks chaining.

This problem also affects NodeList? methods that use concat(), specifically andSelf(). See: http://jsfiddle.net/zjacreman/5gSvS/

comment:6 Changed 8 years ago by Zakk Acreman

I've been poking at this bug some more.

http://jsfiddle.net/zjacreman/c4YRT/1/

Long story short, it looks like Dojo NodeLists? are not Array-like enough for Array.prototype.concat(). So whether a NodeList? is Dojo or built-in, it always needs to be spliced/converted into a plain array and then re-_wrap()-ped as a Dojo NodeList?.

Changed 8 years ago by Zakk Acreman

Attachment: query_js_patch.patch added

Patch for concat() in query.js. Passes all of the concat tests, including bill's.

comment:7 Changed 7 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

In [29141]:

Fix NodeList#concat?(), copying as an array so native concat can function properly, fixes #14555 !strict

comment:8 Changed 7 years ago by Kris Zyp

Milestone: tbd1.8
Note: See TracTickets for help on using tickets.