Closed Bug 454326 Opened 11 years ago Closed 11 years ago

Range.surroundContents doesn't throw BAD_BOUNDARYPOINTS_ERR

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: smaug)

References

()

Details

Attachments

(1 file, 1 obsolete file)

http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html says that Range.surroundContents should throw BAD_BOUNDARYPOINTS_ERR if the range partially selects a non-Text node.  Acid3, test 11, tests for this behavior, but we don't throw.  (Acid3 tests using comments; if I change one to a text node wrapped in a span, I still don't get an exception.)

See http://acid3.acidtests.org/
I think this may be a duplicate of bug 366944 (recall that some of the Acid3 tests were inspired by bug reports in our Bugzilla).
Not a dup of bug 366944, but related.
With bug 302775 this gives one point (11) in ACID3.
Assignee: nobody → Olli.Pettay
Attached patch proposed patch (obsolete) — Splinter Review
This should do it. Text nodes may be partially selected, but only if they
are in the same level.
I added also !mRoot for now, because ExtractContents will return the
right error code for that case (bug 448993).
Attachment #338715 - Flags: superreview?(jonas)
Attachment #338715 - Flags: review?(jonas)
Shouldn't we also not throw if one endpoint is in a textnode and the other endpoint is in the textnodes parent?

I.e.   <foo>|<bar/>te|xt</foo>

Also, shouldn't we throw if mRoot is null? (Not really sure when that can happen still)
(In reply to comment #5)
> Shouldn't we also not throw if one endpoint is in a textnode and the other
> endpoint is in the textnodes parent?
> 
> I.e.   <foo>|<bar/>te|xt</foo>
True.

> 
> Also, shouldn't we throw if mRoot is null? (Not really sure when that can
> happen still)
That is what bug 448993 is about. Calling ExtractContents will handle that.
But will ExtractContents throw the right exception? Arguably the spec is unclear on exception to throw.
Attached patch v2Splinter Review
This way then. Adds also few more tests.
Attachment #338715 - Attachment is obsolete: true
Attachment #338750 - Flags: superreview?(jonas)
Attachment #338750 - Flags: review?(jonas)
Attachment #338715 - Flags: superreview?(jonas)
Attachment #338715 - Flags: review?(jonas)
Attachment #338750 - Flags: superreview?(jonas)
Attachment #338750 - Flags: superreview+
Attachment #338750 - Flags: review?(jonas)
Attachment #338750 - Flags: review+
Comment on attachment 338750 [details] [diff] [review]
v2

>+  if (mStartParent != mEndParent) {
>+    PRBool startIsText = mStartParent->IsNodeOfType(nsINode::eTEXT);
>+    PRBool endIsText = mEndParent->IsNodeOfType(nsINode::eTEXT);
>+    nsINode* startGrandParent = mStartParent->GetNodeParent();
>+    nsINode* endGrandParent = mEndParent->GetNodeParent();
>+    NS_ENSURE_TRUE((startIsText && endIsText &&
>+                    startGrandParent &&
>+                    startGrandParent == endGrandParent) ||
>+                   (startIsText &&
>+                    startGrandParent &&
>+                    startGrandParent == mEndParent) ||
>+                   (endIsText &&
>+                    endGrandParent &&
>+                    endGrandParent == mStartParent),
>+                   NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR);
>+  }

How about simply

  if (mStartParent != mEndParent) {
    nsINode* start = mStartParent->IsNodeOfType(nsINode::eTEXT) ?
                     mStartParent : mStartParent->GetNodeParent();
    nsINode* end = mEndParent->IsNodeOfType(nsINode::eTEXT) ?
                   mEndParent : mEndParent->mEndParent();
    NS_ENSURE_TRUE(start && start == end, NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR);
  }

r/sr=me either way.
That does wrong thing, if mStartParent, nor mEndParent is eTEXT.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Sorry, got it inverted, should be

  if (mStartParent != mEndParent) {
    nsINode* start = mStartParent->IsNodeOfType(nsINode::eTEXT) ?
                     mStartParent->GetNodeParent() : mStartParent;
    nsINode* end = mEndParent->IsNodeOfType(nsINode::eTEXT) ?
                   mEndParent->mEndParent() : mEndParent;
    NS_ENSURE_TRUE(start && start == end,
NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR);
  }

But with that it should work fine as far as i can see
That would work, but is IMO harder to read.
Duplicate of this bug: 354009
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.