Changeset 30123 in webkit

Timestamp:

Feb 10, 2008 12:13:51 PM (16 years ago)

Author:

Darin Adler

Message:

WebCore:

Reviewed and tweaked by Darin.

DOMRange.surroundContents throws wrong exception (Acid3 bug)
​http://bugs.webkit.org/show_bug.cgi?id=16749

Throw BAD_BOUNDARYPOINTS_ERR if attempting to split a non-text node that
has offsets in characters.

• dom/Range.cpp: (WebCore::Range::surroundContents):

LayoutTests:

Reviewed and tweaked a bit by Darin.

DOMRange.surroundContents throws wrong exception (Acid3 bug)
​http://bugs.webkit.org/show_bug.cgi?id=16749

Throw BAD_BOUNDARYPOINTS_ERR if attempting to split a non-text node that
has offsets in characters

• fast/dom/Range/range-exceptions-expected.txt: Added.
• fast/dom/Range/resources/range-exceptions.js: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/dom/Range/range-exceptions-expected.txt (modified) (2 diffs)
• LayoutTests/fast/dom/Range/resources/range-exceptions.js (modified) (2 diffs)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/Range.cpp (modified) (8 diffs)
• WebCore/dom/Range.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2008-02-10  Andrew Wellington  <proton@wiretapped.net>
+
+        Reviewed and tweaked a bit by Darin.
+        
+        DOMRange.surroundContents throws wrong exception (Acid3 bug)
+        http://bugs.webkit.org/show_bug.cgi?id=16749
+        
+        Throw BAD_BOUNDARYPOINTS_ERR if attempting to split a non-text node that
+        has offsets in characters
+
+        * fast/dom/Range/range-exceptions-expected.txt: Added.
+        * fast/dom/Range/resources/range-exceptions.js: Added.
+
 2008-02-09  David Hyatt  <hyatt@apple.com>
 

trunk/LayoutTests/fast/dom/Range/range-exceptions-expected.txt

@@ -1 @@
-This test checks a few DOM range exceptions.
+This test checks some DOM Range exceptions.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
@@ -7 +7 @@
 PASS foo.outerHTML is '<foo></foo>'
 PASS range.surroundContents(foo) threw exception Error: BAD_BOUNDARYPOINTS_ERR: DOM Range Exception 1.
+PASS r.surroundContents(document.createElement('a')) threw exception Error: BAD_BOUNDARYPOINTS_ERR: DOM Range Exception 1.
+PASS r.surroundContents(document.createElement('a')) threw exception Error: HIERARCHY_REQUEST_ERR: DOM Exception 3.
 PASS successfullyParsed is true
 

trunk/LayoutTests/fast/dom/Range/resources/range-exceptions.js

@@ -1 +1 @@
 description(
-"This test checks a few DOM range exceptions."
+"This test checks some DOM Range exceptions."
 );
 
@@ -14 +14 @@
 shouldThrow("range.surroundContents(foo)");
 
+// Ensure that we throw BAD_BOUNDARYPOINTS_ERR when trying to split a comment
+// (non-text but character-offset node). (Test adapted from Acid3.)
+var c1 = document.createComment("aaaaa");
+node.appendChild(c1);
+var c2 = document.createComment("bbbbb");
+node.appendChild(c2);
+var r = document.createRange();
+r.setStart(c1, 2);
+r.setEnd(c2, 3);
+shouldThrow("r.surroundContents(document.createElement('a'))", '"Error: BAD_BOUNDARYPOINTS_ERR: DOM Range Exception 1"');
+
+// But not when we don't try to split the comment.
+r.setStart(c1, 0);
+r.setEnd(c1, 5);
+shouldThrow("r.surroundContents(document.createElement('a'))", '"Error: HIERARCHY_REQUEST_ERR: DOM Exception 3"');
+
 var successfullyParsed = true;

trunk/WebCore/ChangeLog

@@ +1 @@
+2008-02-10  Andrew Wellington  <proton@wiretapped.net>
+
+        Reviewed and tweaked by Darin.
+        
+        DOMRange.surroundContents throws wrong exception (Acid3 bug)
+        http://bugs.webkit.org/show_bug.cgi?id=16749
+        
+        Throw BAD_BOUNDARYPOINTS_ERR if attempting to split a non-text node that
+        has offsets in characters.
+
+        * dom/Range.cpp:
+        (WebCore::Range::surroundContents):
+
 2008-02-10  Darin Adler  <darin@apple.com>
 

trunk/WebCore/dom/Range.cpp

@@ -365 +365 @@
 
     // compare to end, and point comes after
-    else if (compareBoundaryPoints(refNode, offset, m_endContainer.get(), m_endOffset) == 1)
+    if (compareBoundaryPoints(refNode, offset, m_endContainer.get(), m_endOffset) == 1)
         return 1;
-    
+
     // point is in the middle of this range, or on the boundary points
-    else
-        return 0;
+    return 0;
 }
 
@@ -1169 +1168 @@
 void Range::checkNodeWOffset(Node* n, int offset, ExceptionCode& ec) const
 {
-    if (offset < 0)
-        ec = INDEX_SIZE_ERR;
-        // no return here
-
     switch (n->nodeType()) {
+        case Node::DOCUMENT_TYPE_NODE:
         case Node::ENTITY_NODE:
         case Node::NOTATION_NODE:
-        case Node::DOCUMENT_TYPE_NODE:
             ec = RangeException::INVALID_NODE_TYPE_ERR;
-            break;
+            return;
+        case Node::CDATA_SECTION_NODE:
+        case Node::COMMENT_NODE:
         case Node::TEXT_NODE:
-        case Node::COMMENT_NODE:
-        case Node::CDATA_SECTION_NODE:
-            if ((unsigned)offset > static_cast<CharacterData*>(n)->length())
+            if (static_cast<unsigned>(offset) > static_cast<CharacterData*>(n)->length())
                 ec = INDEX_SIZE_ERR;
-            break;
+            return;
         case Node::PROCESSING_INSTRUCTION_NODE:
-            if ((unsigned)offset > static_cast<ProcessingInstruction*>(n)->data().length())
+            if (static_cast<unsigned>(offset) > static_cast<ProcessingInstruction*>(n)->data().length())
                 ec = INDEX_SIZE_ERR;
-            break;
-        default:
-            if ((unsigned)offset > n->childNodeCount())
+            return;
+        case Node::ATTRIBUTE_NODE:
+        case Node::DOCUMENT_FRAGMENT_NODE:
+        case Node::DOCUMENT_NODE:
+        case Node::ELEMENT_NODE:
+        case Node::ENTITY_REFERENCE_NODE:
+        case Node::XPATH_NAMESPACE_NODE:
+            if (static_cast<unsigned>(offset) > n->childNodeCount())
                 ec = INDEX_SIZE_ERR;
-            break;
-    }
-}
-
-void Range::checkNodeBA( Node *n, ExceptionCode& ec) const
+            return;
+    }
+    ASSERT_NOT_REACHED();
+}
+
+void Range::checkNodeBA(Node* n, ExceptionCode& ec) const
 {
     // INVALID_NODE_TYPE_ERR: Raised if the root container of refNode is not an
@@ -1204 +1205 @@
     while (root->parentNode())
         root = root->parentNode();
+
     if (!(root->nodeType() == Node::ATTRIBUTE_NODE ||
           root->nodeType() == Node::DOCUMENT_NODE ||
@@ -1378 +1380 @@
     m_startOffset = 0;
     m_endContainer = refNode;
-    m_endOffset = refNode->offsetInCharacters() ? refNode->maxCharacterOffset() : refNode->childNodeCount();
+    m_endOffset = maxEndOffset();
 }
 
@@ -1397 +1399 @@
     // INVALID_NODE_TYPE_ERR: Raised if node is an Attr, Entity, DocumentType, Notation,
     // Document, or DocumentFragment node.
-    if( newParent->nodeType() == Node::ATTRIBUTE_NODE ||
-        newParent->nodeType() == Node::ENTITY_NODE ||
-        newParent->nodeType() == Node::NOTATION_NODE ||
-        newParent->nodeType() == Node::DOCUMENT_TYPE_NODE ||
-        newParent->nodeType() == Node::DOCUMENT_NODE ||
-        newParent->nodeType() == Node::DOCUMENT_FRAGMENT_NODE) {
-        ec = RangeException::INVALID_NODE_TYPE_ERR;
-        return;
+    switch (newParent->nodeType()) {
+        case Node::ATTRIBUTE_NODE:
+        case Node::DOCUMENT_FRAGMENT_NODE:
+        case Node::DOCUMENT_NODE:
+        case Node::DOCUMENT_TYPE_NODE:
+        case Node::ENTITY_NODE:
+        case Node::NOTATION_NODE:
+            ec = RangeException::INVALID_NODE_TYPE_ERR;
+            return;
+        case Node::CDATA_SECTION_NODE:
+        case Node::COMMENT_NODE:
+        case Node::ELEMENT_NODE:
+        case Node::ENTITY_REFERENCE_NODE:
+        case Node::PROCESSING_INSTRUCTION_NODE:
+        case Node::TEXT_NODE:
+        case Node::XPATH_NAMESPACE_NODE:
+            break;
     }
 
@@ -1420 +1431 @@
         return;
     }
+
+    // BAD_BOUNDARYPOINTS_ERR: Raised if the Range partially selects a non-Text node.
+    // FIXME: The specification says non-Text. What does that mean about CDataSection, which
+    // is a kind of Text node? Use isTextNode() instead of nodeType() to include CDataSection?
+    if (m_startContainer->nodeType() != Node::TEXT_NODE) {
+        if (m_startOffset > 0 && m_startOffset < maxStartOffset()) {
+            ec = RangeException::BAD_BOUNDARYPOINTS_ERR;
+            return;
+        }
+    }
+    if (m_endContainer->nodeType() != Node::TEXT_NODE) {
+        if (m_endOffset > 0 && m_endOffset < maxEndOffset()) {
+            ec = RangeException::BAD_BOUNDARYPOINTS_ERR;
+            return;
+        }
+    }    
 
     // Raise a HIERARCHY_REQUEST_ERR if m_startContainer doesn't accept children like newParent.
@@ -1437 +1464 @@
     }
 
-    // ### check if node would end up with a child node of a type not allowed by the type of node
-
-    // BAD_BOUNDARYPOINTS_ERR: Raised if the Range partially selects a non-text node.
-    if (!m_startContainer->offsetInCharacters()) {
-        if (m_startOffset > 0 && m_startOffset < m_startContainer->childNodeCount()) {
-            ec = RangeException::BAD_BOUNDARYPOINTS_ERR;
-            return;
-        }
-    }
-    if (!m_endContainer->offsetInCharacters()) {
-        if (m_endOffset > 0 && m_endOffset < m_endContainer->childNodeCount()) {
-            ec = RangeException::BAD_BOUNDARYPOINTS_ERR;
-            return;
-        }
-    }
+    // FIXME: Do we need a check if the node would end up with a child node of a type not
+    // allowed by the type of node?
 
     ec = 0;
@@ -1684 +1698 @@
 }
 
-}
+unsigned Range::maxStartOffset() const
+{
+    if (!m_startContainer)
+        return 0;
+    if (!m_startContainer->offsetInCharacters())
+        return m_startContainer->childNodeCount();
+    return m_startContainer->maxCharacterOffset();
+}
+
+unsigned Range::maxEndOffset() const
+{
+    if (!m_endContainer)
+        return 0;
+    if (!m_endContainer->offsetInCharacters())
+        return m_endContainer->childNodeCount();
+    return m_endContainer->maxCharacterOffset();
+}
+
+}

trunk/WebCore/dom/Range.h

@@ -132 +132 @@
     void checkDeleteExtract(ExceptionCode&);
     bool containedByReadOnly() const;
+    unsigned maxStartOffset() const;
+    unsigned maxEndOffset() const;
 };