Last Comment Bug 579638 - Reinstate intersectsNode on DOM Range
: Reinstate intersectsNode on DOM Range
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: :Ms2ger (⌚ UTC+1/+2)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2010-07-17 05:57 PDT by Tim Down
Modified: 2013-08-04 01:26 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP (4.30 KB, patch)
2010-10-22 13:52 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v1 (40.56 KB, patch)
2012-04-13 12:36 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (40.60 KB, patch)
2012-04-29 04:45 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v3 (202.99 KB, patch)
2012-07-15 12:40 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review

Description Tim Down 2010-07-17 05:57:03 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100625 Firefox/3.6.6
Build Identifier: 

I'm requesting that the intersectsNode method of Range (removed along with compareNode in Gecko 1.9: see #358073) be reinstated.

I personally found it useful. For example, it enables one to write a simple function to iterate over nodes within a range by using a TreeWalker on the range's commonAncestorContainer and filtering using intersectsNode. The published workaround on MDC is slightly fiddly and doesn't work in older WebKit because they had a bug in compareBoundaryPoints:

You've kept comparePoint, which is non-standard, so I don't really see the
argument for axing intersectsNode, which is a convenient, well-named and useful
function, and WebKit and Opera still have it (haven't had a chance to check IE
9 preview yet).

Reproducible: Always
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-07-17 09:51:56 PDT
See Bug 358073
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-19 02:04:14 PDT
Ms2ger: This sounds like something that's up your street. Wanna write up a patch?
Comment 3 Tim Down 2010-07-20 11:01:53 PDT
A little further research reveals that the previous Mozilla implementation and its JavaScript replacement on MDC differ from the WebKit and Opera implementation: in Mozilla, a Range adjacent to a node (ie whose end boundary is immediately before the node or start boundary immediately after) is NOT considered to intersect with it while the opposite is true in WebKit and Opera.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2010-10-22 13:52:57 PDT
Created attachment 485397 [details] [diff] [review]

This still needs tests.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2010-10-22 13:54:02 PDT
For the record, the specification is available from <>.
Comment 6 Olli Pettay [:smaug] 2010-10-23 02:35:28 PDT
How do I actually see the draft specification?
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2010-10-23 10:37:19 PDT
$ hg clone && firefox dom-range/dom-range
Comment 8 Anne (:annevk) 2011-09-04 05:28:24 PDT
This is the only method that currently does not throw when the range is detached as far as I can tell. Should we change that?
Comment 9 Tim Down 2011-09-04 06:24:57 PDT
I can't see why it would be different from other methods and it makes no sense to call it on a detached range, so I'd favour changing it. For what it's worth, the implementation of intersectsNode in my Rangy library (, which pre-dates most of the new Range spec, throws for a detached range.
Comment 10 Aryeh Gregor (:ayg) (next working March 28-April 26) 2011-12-30 08:04:01 PST
New spec I just wrote, tested against WebKit and Opera:

There's one XXX there that I'd be interested in hearing feedback on.  If you do intersectsNode() on the range's root, WebKit and Opera both throw, but it would make more sense to just return true -- the range is fully contained in the node, so clearly it intersects it.

Currently the spec matches Chrome 17 dev in all my tests, except that it throws if the range is detached.
Comment 11 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-01-01 09:17:05 PST
I just noted "[needs tests]" in the whiteboard.  My tests are here, usable under CC0:

(When they're approved, they'll move from submissions/AryehGregor/ to approved/, probably with the rest of the URL the same.)  They should be easily adaptable to mochitests.  Chrome 17 dev passes all of them except the detached range ones (and see bug 702948).
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-04-13 12:36:51 PDT
Created attachment 614882 [details] [diff] [review]
Patch v1

Now with Aryeh's test. I imported just the one test, because the full test suite hit timeouts when I last tried to import it.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-04-13 13:09:57 PDT
Comment 14 Olli Pettay [:smaug] 2012-04-17 03:51:57 PDT
Comment on attachment 614882 [details] [diff] [review]
Patch v1

>+  nsINode* parent = node->GetNodeParent();
>+  if (!parent) {
>+    // |node|'s root is |node| itself.
>+    if (GetRoot() != node) {
>+      return NS_OK;
>+    }
>+  }
We should get spec changed. Throwing in this case is just wrong.

>+++ b/dom/interfaces/range/nsIDOMRange.idl
>@@ -94,13 +94,18 @@ interface nsIDOMRange : nsISupports
Update the uuid of nsIDOMRange
Comment 15 Olli Pettay [:smaug] 2012-04-17 06:31:12 PDT
Spec has been changed.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-04-29 04:45:38 PDT
Created attachment 619383 [details] [diff] [review]
Patch v2
Comment 17 Olli Pettay [:smaug] 2012-05-03 04:27:11 PDT
Comment on attachment 619383 [details] [diff] [review]
Patch v2

>+nsRange::IntersectsNode(nsIDOMNode* aNode, bool* aResult)
>+  *aResult = false;
>+  nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
>+  // TODO: This should throw a TypeError.
>+  NS_ENSURE_ARG(node);
>+  // Step 1.
>+  if (mIsDetached) {
>+  }
Remove mIsDetached thing. I know it is still in the spec, but
AFAIK we're trying to remove it.

>+++ b/dom/interfaces/range/nsIDOMRange.idl
>@@ -94,13 +94,18 @@ interface nsIDOMRange : nsISupports
update uuid

r for the patch. I'll look at the tests still.
Comment 18 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-05-03 04:41:05 PDT
Comment on attachment 619383 [details] [diff] [review]
Patch v2

Review of attachment 619383 [details] [diff] [review]:

::: content/base/src/nsRange.cpp
@@ +698,5 @@
> +
> +  // Step 1.
> +  if (mIsDetached) {
> +  }

Smaug pointed out this is outdated now because of bug 702948.
Comment 19 Olli Pettay [:smaug] 2012-05-07 07:32:58 PDT
Comment on attachment 619383 [details] [diff] [review]
Patch v2

Waiting for a new patch.
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-07-15 12:40:52 PDT
Created attachment 642404 [details] [diff] [review]
Patch v3

Updated to match the current spec and to use the now-imported tests.
Comment 21 Olli Pettay [:smaug] 2012-07-16 09:34:36 PDT
Comment on attachment 642404 [details] [diff] [review]
Patch v3

>+++ b/dom/interfaces/range/nsIDOMRange.idl
>@@ -60,13 +60,18 @@ interface nsIDOMRange : nsISupports
update uuid
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2012-07-18 06:14:42 PDT

Note You need to log in before you can comment on or make changes to this bug.