Reinstate intersectsNode on DOM Range

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Core & HTML
--
enhancement
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Tim Down, Assigned: Ms2ger)

Tracking

({dev-doc-complete})

Trunk
mozilla17
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Version: unspecified → Trunk
See Bug 358073
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ms2ger: This sounds like something that's up your street. Wanna write up a patch?
Assignee: nobody → Ms2ger
(Reporter)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 485397 [details] [diff] [review]
WIP

This still needs tests.
(Assignee)

Comment 5

6 years ago
For the record, the specification is available from <http://bitbucket.org/ms2ger/dom-range/>.
How do I actually see the draft specification?
(Assignee)

Comment 7

6 years ago
$ hg clone https://ms2ger@bitbucket.org/ms2ger/dom-range && firefox dom-range/dom-range
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Whiteboard: [needs tests]

Comment 8

6 years ago
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?

Updated

6 years ago
(Reporter)

Comment 9

6 years ago
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.
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.
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).
(Assignee)

Updated

5 years ago
Whiteboard: [needs tests]
(Assignee)

Comment 12

5 years ago
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.
Attachment #485397 - Attachment is obsolete: true
Attachment #614882 - Flags: review?(bugs)
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fb375dbadfa0
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
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
Attachment #614882 - Flags: review?(bugs) → review-
Spec has been changed.
(Assignee)

Comment 16

5 years ago
Created attachment 619383 [details] [diff] [review]
Patch v2
Attachment #614882 - Attachment is obsolete: true
Attachment #619383 - Flags: review?(bugs)
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 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 on attachment 619383 [details] [diff] [review]
Patch v2

Waiting for a new patch.
Attachment #619383 - Flags: review?(bugs)
(Assignee)

Comment 20

5 years ago
Created attachment 642404 [details] [diff] [review]
Patch v3

Updated to match the current spec and to use the now-imported tests.
Attachment #619383 - Attachment is obsolete: true
Attachment #642404 - Flags: review?(bugs)
Comment on attachment 642404 [details] [diff] [review]
Patch v3

>+++ b/dom/interfaces/range/nsIDOMRange.idl
>@@ -60,13 +60,18 @@ interface nsIDOMRange : nsISupports
update uuid
Attachment #642404 - Flags: review?(bugs) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/mozilla-central/rev/db381e1be517
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.