Last Comment Bug 579638 - Reinstate intersectsNode on DOM Range
: Reinstate intersectsNode on DOM Range
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: :Ms2ger
:
Mentors:
http://dvcs.w3.org/hg/domcore/raw-fil...
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (4.30 KB, patch)
2010-10-22 13:52 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
Patch v1 (40.56 KB, patch)
2012-04-13 12:36 PDT, :Ms2ger
bugs: review-
Details | Diff | Splinter Review
Patch v2 (40.60 KB, patch)
2012-04-29 04:45 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
Patch v3 (202.99 KB, patch)
2012-07-15 12:40 PDT, :Ms2ger
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:1.9.2.6) 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: https://bugs.webkit.org/show_bug.cgi?id=20738

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] (khuey@mozilla.com) 2010-07-17 09:51:56 PDT
See Bug 358073
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 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 2010-10-22 13:52:57 PDT
Created attachment 485397 [details] [diff] [review]
WIP

This still needs tests.
Comment 5 :Ms2ger 2010-10-22 13:54:02 PDT
For the record, the specification is available from <http://bitbucket.org/ms2ger/dom-range/>.
Comment 6 Olli Pettay [:smaug] 2010-10-23 02:35:28 PDT
How do I actually see the draft specification?
Comment 7 :Ms2ger 2010-10-23 10:37:19 PDT
$ hg clone https://ms2ger@bitbucket.org/ms2ger/dom-range && 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 (http://code.google.com/p/rangy), which pre-dates most of the new Range spec, throws for a detached range.
Comment 10 :Aryeh Gregor (away until August 15) 2011-12-30 08:04:01 PST
New spec I just wrote, tested against WebKit and Opera:

http://dvcs.w3.org/hg/domcore/raw-file/tip/dom-core.html#dom-range-intersectsnode

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 (away until August 15) 2012-01-01 09:17:05 PST
I just noted "[needs tests]" in the whiteboard.  My tests are here, usable under CC0:

http://w3c-test.org/webapps/DOMCore/tests/submissions/AryehGregor/Range-intersectsNode.html

(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 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 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;
>+    }
>+
>+    return NS_ERROR_DOM_NOT_FOUND_ERR;
>+  }
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 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


>+NS_IMETHODIMP
>+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) {
>+    return NS_ERROR_DOM_INVALID_STATE_ERR;
>+  }
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 (away until August 15) 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) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> +  }

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 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

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