Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9672 closed defect (fixed)

NodeList-traverse add nextAll/prevAll

Reported by: Les Owned by: James Burke
Priority: high Milestone: 1.4
Component: General Version: 1.3.2
Keywords: NodeList-traverse Cc:
Blocked By: Blocking:

Description

Please add nextAll() and prevAll() to find all siblings after/before the current element.

Change History (16)

comment:1 Changed 10 years ago by Les

...having andSelf() would also be great :)

comment:2 Changed 10 years ago by James Burke

Milestone: tbd1.4
Owner: changed from anonymous to James Burke

Interesting, nextAll and prevAll are not in the jquery tests? Anyway, sounds good. I'll add all three.

comment:3 Changed 10 years ago by Les

Thank you! I noticed that jQuery doesn't support reversing results, but this is feature available in Dojo.

jQuery('div').reverse()

This works in Dojo, which is very nice:

dojo.query('div').reverse()

comment:4 Changed 10 years ago by Les

I looked at the Dojo tests and I don't see that there are tests for the presence of native methods, e.g. sort() or reverse(). IMO it would be a good idea to add these tests.

comment:5 Changed 10 years ago by dante

see #9499 (re: reverse() and sort()) a simple test was added (see 'sort/smoketest' in base/_query tests) but it is currently failing in webkit.

comment:6 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [20347]) Fixes #9672, nextAll, prevAll and andSelf added to dojo.NodeList?-traverse, with docs on campus.

comment:7 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

Nodes returned by prevAll() are returned in reversed DOM order. Is this intentional or required for performance reason? It think this is the only NodeList? method that reverses node order. If it needs to stay this way, I'd mentioned in the inline doc that the node order is reversed.

comment:8 Changed 10 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

(In [20405]) Fixes #9672, documenting that prevAll returns nodes in reverse DOM order. This matches the current jquery 1.3.2 behavior.

comment:9 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

I'll have to reopen this ticket with a different issue.

dojo prevAll() and nextAll() are slow compared to jQuery. This might be related to ticket #9411.

Test setup:

[http://archive.dojotoolkit.org/nightly/dojotoolkit/dojo/tests/NodeList-manipulate.html ]

Run:

dojo.require('dojo.NodeList-traverse');

console.time('dojo_prevAll');
for(var i=0; i<1000; i++) {
dojo.query("form").prevAll();
}
console.timeEnd('dojo_prevAll');

console.time('jQuery_prevAll');
for(var i=0; i<1000; i++) {
jQuery("form").prevAll();
}
console.timeEnd('jQuery_prevAll');

console.time('dojo_nextAll');
for(var i=0; i<1000; i++) {
dojo.query("form").nextAll();
}
console.timeEnd('dojo_nextAll');

console.time('jQuery_nextAll');
for(var i=0; i<1000; i++) {
jQuery("form").nextAll();
}
console.timeEnd('jQuery_nextAll');

Results:

FF 3.5 prevAll dojo --> jQuery: 279ms --> 136ms
FF 3.5 nextAll dojo --> jQuery: 255ms --> 127ms
IE6 prevAll dojo --> jQuery: 2125ms --> 782ms
IE6 nextAll dojo --> jQuery: 2765ms --> 1016ms

comment:10 Changed 10 years ago by Les

Chrome 4 prevAll dojo --> jQuery: 380ms --> 70ms
Chrome 4 nextAll dojo --> jQuery: 337ms --> 78ms

comment:11 Changed 10 years ago by Les

NodeList?.siblings() test (same page):

dojo.require('dojo.NodeList-traverse');

var dform = dojo.query("form");
var jform = jQuery("form")

console.time('dojo');
for(var i=0; i<3000; i++) {
dform.siblings();
}
console.timeEnd('dojo');

console.time('jQuery');
for(var i=0; i<3000; i++) {
jform.siblings();
}
console.timeEnd('jQuery');

Results dojo --> jQuery:

FF 3.5 442ms --> 191ms
IE6 5469ms --> 1766ms
Chrome 4 395ms --> 84ms

comment:12 Changed 10 years ago by Les

NodeList?.parent() test (same loop, same page): dojo --> jQuery

FF 3.5 239ms --> 72ms
IE6 1969ms --> 359ms
Chrome 4 304ms --> 32ms

comment:13 Changed 10 years ago by Les

I wanted to add a test for prev() and next(), but I think these tests are sufficient and the numbers speak for themselves.

comment:14 Changed 10 years ago by James Burke

Milestone: 1.42.0

I did a quick profile using the Firebug profiler on the prevAll() case. It breaks down like so:

dojo._mixin takes 46% of the time. Ouch.

dojo.query() and the function used inside query that is defined via _queryFuncCacheQSA[query] = function(root){} takes 9.57% and 11.58% respectively.

Then tnl() (part of dojo.NodeList?) takes 9.48% of the time.

So around 77% of the time is spent in other things. I would prefer to tackle bigger fish in that perf test, like dojo._mixin before looking at specific enhancements in dojo.NodeList?-traverse.

It does make me wonder about tradeoff of decorating an array for dojo.query calls vs. returning a custom object that has a fixed prototype. That would avoid the _mixin calls, but query results are no longer an array. Maybe store the array as an internal variable on the custom object that can be fetched via a toArray() method? That is a larger discussion for a Dojo 2.0 timeframe.

For now, though I am happy just to have the NodeList?-traverse methods available for 1.4.

comment:15 Changed 10 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

Going to track the _mixin stuff in #10203 and close out this ticket.

comment:16 Changed 10 years ago by Adam Peller

Milestone: 2.01.4
Note: See TracTickets for help on using tickets.