Bug 16749 - DOMRange.surroundContents throws wrong exception (Acid3 bug)
Summary: DOMRange.surroundContents throws wrong exception (Acid3 bug)
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
Depends on:
Blocks: Acid3
  Show dependency treegraph
Reported: 2008-01-05 14:15 PST by Eric Seidel (no email)
Modified: 2019-02-06 09:04 PST (History)
3 users (show)

See Also:

Proposed patch (5.25 KB, patch)
2008-01-05 20:27 PST, Andrew Wellington
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-05 14:15:14 PST
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");
      var c2 = doc.createComment("bbbbb");
      var r = doc.createRange();
      r.setStart(c1, 2);
      r.setEnd(c2, 3);
      var msg = 'wrong exception raised';
      try {
        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;
Comment 1 Eric Seidel (no email) 2008-01-05 14:15:35 PST
The failure text:
Test 15: FAIL (wrong exception raised; code = 3)

Comment 2 Alexey Proskuryakov 2008-01-05 15:56:13 PST
"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?
Comment 3 Andrew Wellington 2008-01-05 20:27:41 PST
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.
Comment 4 Eric Seidel (no email) 2008-01-05 22:20:35 PST
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:
Comment 5 Eric Seidel (no email) 2008-01-05 22:39:58 PST
Ian, I think it's unclear as to why this is the "wrong exception".
Comment 6 Ian 'Hixie' Hickson 2008-01-05 22:56:03 PST
woops, i meant to put the comment nodes in an element, not in a document. my bad. will fix.
Comment 7 Eric Seidel (no email) 2008-01-06 00:16:16 PST
FYI the fail message is now:
Test 15: FAIL (when trying to surround two halves of comment: wrong exception raised; code = 3)
Comment 8 Alexey Proskuryakov 2008-01-06 00:24:10 PST
As Andrew has discovered, this case meets conditions for both exceptions.
Comment 9 Darin Adler 2008-01-06 01:05:35 PST
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.
Comment 10 Ian 'Hixie' Hickson 2008-01-08 19:37:34 PST
(please do let me know if you disagree with what the test currently does)
Comment 11 Darin Adler 2008-02-10 12:10:17 PST
(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?
Comment 12 Darin Adler 2008-02-10 12:14:25 PST
Committed revision 30123.
Comment 13 Ian 'Hixie' Hickson 2008-02-10 13:17:37 PST
This would be a WebAPI working group thing. We need a volunteer to edit the spec.
Comment 14 Lucas Forschler 2019-02-06 09:04:09 PST
Mass moving XML DOM bugs to the "DOM" Component.