#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)
Change History (10)
Changed 8 years ago by
Attachment: | isDescendant.html added |
---|
comment:1 follow-up: 2 Changed 8 years ago by
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 Changed 8 years ago by
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
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.
comment:4 Changed 7 years ago by
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
Milestone: | tbd → 1.11 |
---|---|
Owner: | changed from Eugene Lazutkin to dylan |
Status: | new → assigned |
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
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:7 Changed 4 years ago by
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
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.
test case and benchmark