Bug 16749 – DOMRange.surroundContents throws wrong exception (Acid3 bug)

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric Seidel (no email)]

DOMRange.surroundContents throws wrong exception (Acid3 bug)

This one should be easy to fix. :)

    function () {
      // test 15: Ranges and Comments
      var doc = getTestDocument();
      var c1 = doc.createComment("aaaaa");
      doc.appendChild(c1);
      var c2 = doc.createComment("bbbbb");
      doc.appendChild(c2);
      var r = doc.createRange();
      r.setStart(c1, 2);
      r.setEnd(c2, 3);
      var msg = 'wrong exception raised';
      try {
        r.surroundContents(doc.createElement('a'));
        msg = 'no exception raised';
      } catch (e) {
        if ('code' in e)
          msg += '; code = ' + e.code;
        if (e.code == e.BAD_BOUNDARYPOINTS_ERR)
          msg = '';
      }
      assert(msg == '', msg);
      assert(r.toString() == "", "comments returned text");
      return 1;
    },
    function () {
      return 1;
    },

[Eric Seidel (no email)]

The failure text:
Test 15: FAIL (wrong exception raised; code = 3)

[Alexey Proskuryakov]

"BAD_BOUNDARYPOINTS_ERR: Raised if the Range partially selects a non-text node."

I'm wondering why the spec says that - why are Text nodes different from Comment and CDATASection ones in this regard?

[Andrew Wellington]

Created attachment 18294 [details]
Proposed patch

Ensure that we throw for all non-text nodes not just those that aren't character indexed for offsets.

Also moved the code for this exception up the method so it'll throw before HIERARCHY_REQUEST_ERR, which will let this Acid3 test pass.

[Eric Seidel (no email)]

Comment on attachment 18294 [details]
Proposed patch

A couple things.  I'm not yet sure we really have enough test coverage for this change.  Perhaps I'm just not thinking clear enough at this hour, or perhaps you can just convince me over IRC.

Also, our style guidelines forbid c-style casts in c++ code:
(int)m_endOffset

[Eric Seidel (no email)]

Ian, I think it's unclear as to why this is the "wrong exception".

[Ian 'Hixie' Hickson]

woops, i meant to put the comment nodes in an element, not in a document. my bad. will fix.

[Eric Seidel (no email)]

FYI the fail message is now:
Test 15: FAIL (when trying to surround two halves of comment: wrong exception raised; code = 3)

[Alexey Proskuryakov]

As Andrew has discovered, this case meets conditions for both exceptions.

[Darin Adler]

Comment on attachment 18294 [details]
Proposed patch

Looks good!

+    } else if (m_startContainer->nodeType() != Node::TEXT_NODE) {

I would just have used isTextNode() here rather than nodeType().

+        if (m_startOffset > 0) {
+            ec = RangeException::BAD_BOUNDARYPOINTS_ERR;
+            return;
+        }

This seems wrong. If m_startOffset is >= m_startContainer->maxCharacterOffset, I think we're fine here.

review- because of that incorrect case. We need to add a test where the start container is a comment node and the start offset is the end of the comment node.

[Ian 'Hixie' Hickson]

(please do let me know if you disagree with what the test currently does)

[Darin Adler]

(In reply to comment #10)
> (please do let me know if you disagree with what the test currently does)

I don't think the specification is clear enough about which exception will be raised when there are multiple things wrong. It's easy to make WebKit match Acid3's expectations and we'll do that. But the specification should be fixed to be specific enough; there might be additional WebKit fixes needed then. Where would we do that? In HTML 5?

[Darin Adler]

Committed revision 30123.

[Ian 'Hixie' Hickson]

This would be a WebAPI working group thing. We need a volunteer to edit the spec.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.