Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8612 closed enhancement (fixed)

add some sugar to dojo.NodeList

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

Description (last modified by Eugene Lazutkin)

as discussed with pete, need to have some more sugar for plugd in NodeList in Base

Attachments (2)

testSplice.html (874 bytes) - added by jpdutoit 10 years ago.
Splice test case
testSpliceNodeList.js (29.6 KB) - added by jpdutoit 10 years ago.
Splice test cast Nodelist.js

Download all attachments as: .zip

Change History (41)

comment:1 Changed 10 years ago by dante

Cc: dante added
Milestone: 1.31.4

I'm perfectly fine with this not being an 11th hour thing, and targeting 1.4 for this vast NodeList? refactor. Exposing _mapIn was huge, and makes this pattern easily enumerable. We should not do this for 1.3, and polish up these Dom APIs at once concisely. We're overdue on enhancements for 1.3b as it is.

comment:2 Changed 10 years ago by dante

Milestone: 1.41.3
Resolution: fixed
Status: newclosed

this landed in [16642], one change in [16643] but overall the commit is questionable.

comment:3 Changed 10 years ago by James Burke

(In [16645]) Refs #8612 and #8613. Reverts changesets [16641] [16642] [16643]. dojo.create not accepting html strings has been explicitly discussed as not being included in Dojo 1.3 multiple times now. We can talk about it more for 1.4 after we have had time to let the current create bake a bit in the wild. It is easier to add things later than take away later too. Similarly with NodeList?, while I want an end() in general, i want to be sure we think through the use cases for it, and adding more API. and it was done without adequate notice. \!strict

comment:4 Changed 10 years ago by James Burke

Milestone: 1.31.4
Resolution: fixed
Status: closedreopened

comment:5 Changed 10 years ago by James Burke

(In [17568]) Refs #8612: re-introduces a _stash and end() method for dojo.NodeList?. Also, methods that return new NodeLists? also call _stash() so end() can be used properly with them. dojo._queryListCtor renamed to dojo._NodeListCtor since it is a more accurate description and also set by dojo.NodeList?. This allows other dojo.NodeList? delegates/variants to be used by our query and NodeList? operations. Introduces traversing methods for dojo.NodeList? via dojo.NodeList?-traverse: children, closest, parent, parennts, sibling, next, prev, first, last, odd, even. Inline documentation is there, but still need to get feedback on the change and do wiki docs so leaving the ticket open. Also, create() and wrap() are not part of this update. \!strict for query.js

comment:6 Changed 10 years ago by Eugene Lazutkin

Very nice set of extensions! I see them immediately useful in some of my things.

My notes on NodeList-traverse (I used http://bugs.dojotoolkit.org/browser/dojo/trunk/NodeList-traverse.js?rev=17568 as a reference):

  1. getUniqueAsNodeList() line 36: the call to dojo.indexOf() makes this function quadratic in the computational complexity. It will bite us with big lists. Can we come up with something better than that?
  2. children() line 69: do we really need _toArray() here? I think we can safely skip it in this place.
  3. parent() line 99: there is no guard against parent being null. It is frequently encountered when working with fragments.
  4. parents(): is it safe to put non-nodes in NodeList? Some existing NodeList operations may fail, if not all items support certain APIs.
  5. siblings() line 132: no guard for parent being null in the case of a stand-alone node. I would assume that in this case it should return an empty list (no siblings).

Both parent() and parents() return a list of unique nodes. In some cases it is beneficial to keep the result in direct correspondence to the original items:

var nodeList = ...;
// imagine that parent() doesn't check for uniqueness
var parentList = nodeList.parent(someQuery);
// now parentList[i] is the found parent for nodeList[i]

I see the place for both techniques, but I am not ready to call if we need both, or only one (and which one).

comment:7 Changed 10 years ago by James Burke

(In [17572]) Refs #8612: extra checks for null parents (suggested by uhop). parent() really just needs to get immediate parent, and from what I can tell, element nodes are not embedded in anything hazardous except document nodes, but those are weeded out in _getUniqueAsNodeList. Also added a doc clarification to clarify name of function.

comment:8 Changed 10 years ago by James Burke

elazutkin:

  1. Yeah it is really bad. Any ideas? I didn't like the idea of attaching unique IDs to the nodes, but maybe adding a custom attribute with an ID on it would allow us to keep a hash/dictionary to weed out duplicates. I didn't like all that DOM twiddling, but it may be better than dojo.indexOf. The dojo.query _zip function might be instructional, but I have not fully digested it.
  1. Yes, we need the toArray because childNodes is array.concated to the other NodeList?'s nodes' childNodes. At least Firefox pukes if we try to use a NodeList? directly in the array concat call.
  1. Fixed, thanks for the catch. From what I can tell of node types, my test was way too complicated: if we have a node, the likely parent will be an element or a document, or document fragment. But document and fragment nodes will be weeded out in _getUniqueAsNodeList.
  1. _getUniqueAsNodeList should weed out any non-elements, and parents I believe are restricted to elements, documents and document fragments?
  1. Fixed that one thanks.

I'm not sure it makes sense to keep duplicates in a node list. There are many node list operations that could be dangerous to do twice on a node? Doubling up on animations or event handlers seems problematic for example, as well as addContent. I think the most common case is to use NodeLists? as part of dojo.query() calls, so I think it should follow the conceptual model of the DOM, and duplicates in that DOM model seem weird.

Thanks for your review. I'm sure you can find ways to further optimize the code!

comment:9 Changed 10 years ago by dante

the inline docs are disappointing. I don't see any information as to how to actually use _stash/end and it is currently incompatible with plugd's api. All NodeList?.js functions use the private 'tnl()' and pass a parent, but that doesn't help anyone outside of NodeList?. perhaps I'm missing something in the code, but the lack of inline doc examples discouraged me for investigating. I think I understand why the current implementation is "better", but the lack of a migration path is frustrating, to say the least.

comment:10 Changed 10 years ago by James Burke

(In [17573]) Refs #8612: better docs for _stash and end, code reduction in NodeList?-traverse methods. For NodeList?.end, throw descriptive error if no parent NodeList?. This is experimental though/needs more input. Pete for one is not sold on the idea, but I feel we need to fail as fast as we can instead of silently introducing bugs into a new user's code.

comment:11 in reply to:  8 ; Changed 10 years ago by Eugene Lazutkin

Description: modified (diff)

Replying to jburke:

  1. Yeah it is really bad. Any ideas? I didn't like the idea of attaching unique IDs to the nodes, but maybe adding a custom attribute with an ID on it would allow us to keep a hash/dictionary to weed out duplicates. I didn't like all that DOM twiddling, but it may be better than dojo.indexOf. The dojo.query _zip function might be instructional, but I have not fully digested it.

Yes, I think that Alex' solution is the most practical one. _zip looks complicated because Alex deals with IE XML. If we are to restrict the code to HTML it'll be simple. A sketch:

//private variables
var expandoName = "_dojoMarker", expandoValue = 0;

// the function
var _getUniqueAsNodeList = function(nodes){
  var ret = [], i = 0, node;
  ++expandoValue;
  // collect unique nodes using expando properties
  while(node = nodes[i++]){
    if(node.nodeType == 1 /*NODE*/){
      if(node[expandoName] !== expandoValue){
        node[expandoName] = expandoValue;
        ret.push(node);
      }
    }
  }
  // optional: cleanup
  for(i = 0; node = ret[i++];){
    delete node[expandoName]; // does it work on all browsers?
    // alternative: node[expandoName] = "";
  }
  return ret;
};

Alex doesn't clean up in his code opting for the smaller code base, and the faster execution at a price of a small baggage left on nodes. I think like usual he did the right choice.

In my code I decided to use the expando name, which tells that it came from Dojo. We can reuse his "_zipIdx".

comment:12 Changed 10 years ago by Mark Wubben

For some reason r17568 breaks dijit.Menu, on line 24 this.getChildren is undefined. r17567 works fine. I have no idea how it relates to NodeList? changes, but this is what I'm seeing.

comment:13 Changed 10 years ago by Mark Wubben

OK, this seems to have been a conflict with Plugd. Still unsure how that happened, but its all working after updating to Plugd r116.

comment:14 in reply to:  11 ; Changed 10 years ago by James Burke

Replying to elazutkin:

Yes, I think that Alex' solution is the most practical one. _zip looks complicated because Alex deals with IE XML. If we are to restrict the code to HTML it'll be simple.

I am leaning towards just exposing zip as a private method on dojo.query, or off of dojo like _filterQueryResult is done. At first glance it looks like _nodeUID() in query.js could also be consolidated to use the same node attribute as the zip stuff, and I would prefer to use a dojo._scopeName-based attribute name to allow multiversion dojo situations. I will likely proceed with that path unless Alex indicates otherwise (particularly if _uid really needs to be a separate attribute).

comment:15 in reply to:  14 Changed 10 years ago by Eugene Lazutkin

Replying to jburke:

I am leaning towards just exposing zip as a private method on dojo.query, or off of dojo like _filterQueryResult is done. At first glance it looks like _nodeUID() in query.js could also be consolidated to use the same node attribute as the zip stuff, and I would prefer to use a dojo._scopeName-based attribute name to allow multiversion dojo situations.

Hmm.

  1. Exposing private dojo.query() stuff precludes us from bundling Sizzle, which doesn't have these functions defined, or adding them to Sizzle ourselves in the adapter.
  1. Moving parts of dojo.query() to Dojo makes Acme more dependent on Dojo, or leads to implementing them twice in both places.

Most probably the first option is the best, but we need to be careful.

comment:16 Changed 10 years ago by James Burke

(In [17600]) Refs #8612. More NodeList? sugar in the form of manipulation APIs for placing nodes in different positions. This one is a bit interesting because it will define an html() method. So does dojo.NodeList?-html. The one in -manipulate will defer to the one in dojo.NodeList?-html if it is loaded. html() in -manipulate is just an alias for -manipulate's innerHTML method, so that one can always be used if you want to be totally safe. I did not want to use -html's version of html(): I feel it is too cumbersome/needs more work and does not fit with the rest of the methods in -manipulate. I want to see how I can plug in widget parsing and template application via -manipulate, but those are future changes.

comment:17 Changed 10 years ago by jpdutoit

It seems that r17568 breaks NodeList?.slice. It returns the full NodeList? every time.

comment:18 Changed 10 years ago by James Burke

jpdutoit: Can you give some more info? When I try the unit tests for dojo.NodeList? via this URL, they all pass: http://archive.dojotoolkit.org/nightly/dojotoolkit/util/doh/runner.html?testModule=dojo.tests._base.query

What browser are you using, and if you have a simple test case, that would be appreciated.

Changed 10 years ago by jpdutoit

Attachment: testSplice.html added

Splice test case

Changed 10 years ago by jpdutoit

Attachment: testSpliceNodeList.js added

Splice test cast Nodelist.js

comment:19 in reply to:  18 Changed 10 years ago by jpdutoit

Replying to jburke: I've attached a test case which fails for me. I'm using a built version of dojo, and have also attached the version of NodeList?.js that was used in this build.

Updating to the r17568 broke all my uses of NodeList?.splice (later i found NodeList?.concat did not work either). Reverting to a previous trunk version fixed it again.

comment:20 Changed 10 years ago by jpdutoit

I forgot to mention I'm using Firefox 3.0.10. But had the same problem using webkit in a custom app.

comment:21 Changed 10 years ago by jpdutoit

In the above posts and filenames, when i mention splice i mean slice. The test case has it right. Guess it's getting a bit late here, sorry. :)

comment:22 Changed 10 years ago by James Burke

jpdutoit: I put up the test case here, using the latest code from http://archive.dojotoolkit.org/nightly:

http://tagneto.org/temp/testSplice.html

and the test passes for me. Are you doing a custom build with other stuff in the dojo.js layer?

comment:23 in reply to:  22 Changed 10 years ago by jpdutoit

Replying to jburke: It seems revisions of plugd before r116 is not compatible with the current dojo, updating to plugd's latest revision fixed the problem. Thanks.

comment:24 Changed 10 years ago by dante

sorry - I should have noticed that. Yes, plugd base had to be switched/reduced some to accomodate the new internals happening in NodeList?. (._stash() is 'backwards' if you are using it) but otherise should work fine. I fixed plugd the day after this initial checkin. Sorry for the confusion, not making a warning in plugd's docs is my bad.

comment:25 Changed 10 years ago by James Burke

(In [17640]) Refs #8612, now addContent can accept HTML string templates and parse for widgets, if dojo.string and/or dojo.parser have been separately dojo.required. This is a bit experimental. If the feedback from folks is good, it will stay in and get more docs on dojocampus. These changes along with dojo.cache allow for some neat NodeList?-based HTML construction from html templates. It is tempting to put in support for dojoAttachEvent and dojoAttachPoint, but I think with the nodelist operations, it makes more sense to just use another .query and the NodeList? event binding methods, particularly now that .end() is supported. I could almost fold the NodeList?-manipulate's placeMulti functionality into NodeList?.js's place method, but the default place method assumes only the first queried node should be use to receive all the nodes instead of allowing all nodes to be matched across all queried elements. If folks felt like this would not be too much of a backward compat issue, we could fold in the changes.

comment:26 Changed 10 years ago by James Burke

(In [17642]) Refs #8612: updating docs to mention other position options that became available as of Dojo 1.3

comment:27 Changed 10 years ago by James Burke

(In [17927]) Refs #8612. Inline doc examples for dojo.NodeList?-manipulate

comment:28 Changed 10 years ago by James Burke

(In [17928]) Refs #8612. Inline doc examples for dojo.NodeList?-traverse

comment:29 Changed 10 years ago by James Burke

(In [17929]) Refs #8612: allow for widgets that change their DOM node.

comment:30 Changed 10 years ago by dante

Priority: normalhigh

[17640] breaks existing usage of NodeList?.instantiate -- why are we tracking instantiated methods now? There is an instantiate test in dijit, perhaps I should move it into base unit tests, where this would have been spotted.

the error is "this._construct is not a function", coming from declare._makeCtor

comment:31 Changed 10 years ago by James Burke

(In [17972]) Refs #8612, fixes instantiate() error and makes sure test passes on all browsers.

comment:32 Changed 10 years ago by James Burke

Priority: highnormal

The instantiate error is fixed now, thanks for the unit test. I completely missed that there was no instantiate unit test. :(

As for tracking instantiated, that grew out of the new functionality to do something like this:

dojo.require("dojo.string");
dojo.require("dojo.parser");
dojo.require("dijit.form.Button");
dojo.requireLocalization("custom", "buttonText");

//Some time later...
var bundle = dojo.i18n.getLocalization("custom", "buttonText");

var emptyDivs = dojo.query(".emptyDiv").addContent({
    template: '<button dojoType="dijit.form.Button">${i18n.send}</button>',
    parse: true, //signifies that parser should be run
    i18n: bundle
});

//Get list of dijits created to store for later
var dijits = emptyDivs.instantiated;

That kind of code works now in trunk. However, I am not sure if it is useful in practice. I also am not sure we want to support plain, non-function properties like instantiated on a NodeList?. In general, I wanted to find a way where we can get at the artifacts that we create via the NodeList? operations, like the dojo.connect handles, the widgets created, mainly so folks can do cleanup appropriately. But maybe that is not something we should support in NodeList?.

Maybe the way to do that is to make sure you can get the connect handles and instantiated dijits by giving dojo a node. This will work for dijits via dijit.getEnclosingWidget(). We would need to modify the connect handling to allow lookups by an ID on the node. Something we may want anyway for things like cloning event connections when nodes are cloned, supporting jquery-like trigger() calls.

I'm not sure of the right answer yet, so feedback is greatly appreciated. I am not tied to the instantiated feature and I can remove it. I still like the ability to use dojo.string templates as part of inserted HTML though. Maybe just cut tracking of the declared instances in instantiated.

comment:33 Changed 10 years ago by James Burke

Thinking more on it, getting the instantiated widgets might work by doing:

var dijits = emptyDivs.query("[dojoType]").map(function(node){
    return dijit.getEnclosingWidget(node);
});

So it is discoverable. I'll look at removing the instantiated property.

comment:34 Changed 10 years ago by James Burke

(In [18112]) Refs #8612. Remove instantiated from NodeList?. Getting dijits works via code snippet in the ticket. it does not work for generically declared things, but it is not clear instantiated is useful in NodeList? type use cases.

comment:35 Changed 10 years ago by James Burke

(In [18344]) Refs #8612. Be sure the NodeList?-manipulate methods that use _placeMultiple can handle NodeLists? and nodes as arguments.

comment:36 Changed 10 years ago by James Burke

(In [19021]) Refs #8612: NodeList? sugar: make it easier for dojo.NodeList? delegates to use their construcors for any NodeLists? created while part of chained methods.

comment:37 Changed 10 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

Good enough for 1.4, any bugs can be opened as separate tickets.

comment:38 Changed 10 years ago by James Burke

(In [20360]) Refs #8612, forgot to remove a line when I removed the instantiated stuff.

comment:39 Changed 10 years ago by James Burke

(In [20361]) Refs #8612, update end() docs to match code.

Note: See TracTickets for help on using tickets.