Closed
Bug 579638
Opened 14 years ago
Closed 12 years ago
Reinstate intersectsNode on DOM Range
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: timdown, Assigned: Ms2ger)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 3 obsolete files)
202.99 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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
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
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•14 years ago
|
||
This still needs tests.
Assignee | ||
Comment 5•14 years ago
|
||
For the record, the specification is available from <http://bitbucket.org/ms2ger/dom-range/>.
Comment 6•14 years ago
|
||
How do I actually see the draft specification?
Assignee | ||
Comment 7•14 years ago
|
||
$ hg clone https://ms2ger@bitbucket.org/ms2ger/dom-range && firefox dom-range/dom-range
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs tests]
Comment 8•13 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•13 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.
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Whiteboard: [needs tests]
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fb375dbadfa0
Assignee | ||
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 14•12 years ago
|
||
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-
Comment 15•12 years ago
|
||
Spec has been changed.
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #614882 -
Attachment is obsolete: true
Attachment #619383 -
Flags: review?(bugs)
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 619383 [details] [diff] [review] Patch v2 Waiting for a new patch.
Attachment #619383 -
Flags: review?(bugs)
Assignee | ||
Comment 20•12 years ago
|
||
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 21•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db381e1be517
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•11 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
Comment 23•11 years ago
|
||
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.
Description
•