Opened 8 years ago

Closed 4 years ago

Last modified 4 years ago

#14629 closed task (fixed)

use Node.contains() to implement dojo.isDescendant()

Reported by: bill Owned by: dylan
Priority: high Milestone: 1.11
Component: HTML Version: 1.7.1
Keywords: Cc:
Blocked By: Blocking:

Description

Use Node.contains() to implement dojo.isDescendant(). It's a little faster (although on my iPad it's 3x faster), and a little less code than the current implementation.

Unfortunately we need a separate code path for FF<9. (All other browsers support Node.contains().) So the full code becomes something like:

newIsDescendant = document.createElement("div").contains ?
function(node1, node2){
	// summary:
	//		Replacement function for dojo.isDescendant() (FF9+, IE, webkit, opera, iOS)
	node2 = dojo.byId(node2);
	return !!node2 && node2.contains(dojo.byId(node1));
} :
function(node1, node2){
	// Fallback function needed for FF8 and below
	node1 = dojo.byId(node1);
	node2 = dojo.byId(node2);
	return !!(node1 && node2 && (node2.compareDocumentPosition(node1) & 16));
};

See attachment for test case and benchmark.

See also #14570 about dropping FF3.6 support, but it says 1.9.

Attachments (2)

isDescendant.html (42.0 KB) - added by bill 8 years ago.
test case and benchmark
contains.patch (3.3 KB) - added by bill 8 years ago.
patch using has()

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by bill

Attachment: isDescendant.html added

test case and benchmark

comment:1 Changed 8 years ago by bill

PS: could save a few more bytes by inlining the assignments:

 newIsDescendant = document.createElement("div").contains ?
 function(node1, node2){
        // Replacement for dojo.isDescendant() (FF9+, IE, webkit, opera, iOS)     
        return !!( (node2 = dojo.byId(node2)) && node2.contains(dojo.byId(node1)) );
 } :
 function(node1, node2){
        // Fallback function needed for FF8 and below
        return !!( (node1 = dojo.byId(node1)) && (node2 = dojo.byId(node2)) &&
               (node2.compareDocumentPosition(node1) & 16) );
 };

That feature test should also be in a has.add() so that it can be filtered out in a build.

comment:2 in reply to:  1 Changed 8 years ago by Eugene Lazutkin

Replying to bill:

That feature test should also be in a has.add() so that it can be filtered out in a build.

Bill, could you provide a (trivial) patch for that? I am unsure where to put it.

comment:3 Changed 8 years ago by bill

Turns out that unfortunately IE has issues with the new code in the edge cases:

  • document.contains() is undefined. document.documentElement.contains() works though.
  • DispHTMLDomTextNode.contains() is undefined. Clearly a text node cannot contain another node, so we should return false in this case, but it's another condition to check.
  • normalDomNode.contains(DispHTMLDomTextNode) also doesn't work. Not sure how to work around that.

So, I worked up a patch using has() that defaults to the old code for FF <= 8 and IE <= 8, and uses the new code otherwise, but not sure if it's worth it. It's up to you. Pros: runs faster for modern browsers, and reduces code size for mobile builds. Cons: increases code size for non-browser-specific build case.

Changed 8 years ago by bill

Attachment: contains.patch added

patch using has()

comment:4 Changed 7 years ago by bill

PS: We should definitely leave this ticket open though, because once we drop support for IE8 (since FF8 is already desupported), the patch becomes both smaller and faster than the current code.

comment:5 Changed 4 years ago by dylan

Milestone: tbd1.11
Owner: changed from Eugene Lazutkin to dylan
Status: newassigned

We should probably land this anyway, as it's a good improvement and reduces build sizes with the static feature.

comment:6 Changed 4 years ago by Dylan Schiemann <dylan@…>

Resolution: fixed
Status: assignedclosed

In 59b32a5eab1c4f48da9b3d2660900f0564a2bcac/util:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:7 Changed 4 years ago by dylan

Closed via https://github.com/dojo/dojo/commit/13aa0fcc89d4433f376f6b329408b4efdb55ff7b

Not backporting as it's not a huge benefit, and could potentially break someone that's already implemented a similar patch in their own code. Please re-open if you feel that we should backport this.

comment:8 Changed 4 years ago by dylan

Required an additional fix in master, https://github.com/dojo/dojo/commit/d28eb010cce1b4f977520357a4cbc5191da886a6 , as document isn't defined in the default global in all cases obviously.

Note: See TracTickets for help on using tickets.