454326 - Range.surroundContents doesn't throw BAD_BOUNDARYPOINTS_ERR

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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/

Comment 1

[Alex Vincent [:WeirdAl]]

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

Comment 2

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

Not a dup of bug 366944, but related.

Comment 3

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

With bug 302775 this gives one point (11) in ACID3.

Comment 4

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

Attachment: proposed patch

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

Comment 5

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

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)

Comment 6

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

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

Comment 7

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

But will ExtractContents throw the right exception? Arguably the spec is unclear on exception to throw.

Comment 8

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

Attachment: v2

This way then. Adds also few more tests.

Comment 9

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

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.

Comment 10

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

That does wrong thing, if mStartParent, nor mEndParent is eTEXT.

Comment 11

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

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

Comment 12

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

That would work, but is IMO harder to read.