579638 - Reinstate intersectsNode on DOM Range

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Tim Down]

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 (Exited; not receiving bugmail, old account, do not use)]

See Bug 358073

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ms2ger: This sounds like something that's up your street. Wanna write up a patch?

Comment 3

[Tim Down]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

This still needs tests.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

For the record, the specification is available from <http://bitbucket.org/ms2ger/dom-range/>.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

How do I actually see the draft specification?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

$ hg clone https://[e-mail]/ms2ger/dom-range && firefox dom-range/dom-range

Comment 8

[Anne (:annevk)]

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]

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 (:ayg) (no longer with Mozilla)]

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 (:ayg) (no longer with Mozilla)]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=fb375dbadfa0

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Spec has been changed.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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 (:ayg) (no longer with Mozilla)]

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][[e-mail]]]

Comment on attachment 619383 [details] [diff] [review]
Patch v2

Waiting for a new patch.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Updated to match the current spec and to use the now-imported tests.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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 (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/db381e1be517

Comment 23

[Jean-Yves Perrier [:teoli]]

Documentation updated:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/17
https://developer.mozilla.org/en-US/docs/Web/API/range
and
https://developer.mozilla.org/en-US/docs/Web/API/Range.intersectsNode