Changeset 81374 in webkit

Timestamp:

Mar 17, 2011 12:02:50 PM (13 years ago)

Author:

rniwa@webkit.org

Message:

2011-03-17 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Justin Garcia.

Assert that editing does not ignore position's anchorNode if position is an offset in anchor
​https://bugs.webkit.org/show_bug.cgi?id=56027

Added a test to ensure WebKit lets users edit contents inside a button element properly.

• editing/execCommand/button-expected.txt: Added.
• editing/execCommand/button.html: Added.

2011-03-17 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Justin Garcia.

Assert that editing does not ignore position's anchorNode if position is an offset in anchor
​https://bugs.webkit.org/show_bug.cgi?id=56027

Added the assertion in Position::Position and Position::moveToPosition. This assertion catches
places where we instantiate positions inside a node on which editingIgnoresContent returns true.

Test: editing/execCommand/button.html

• dom/Position.cpp: (WebCore::Position::Position): Added an assertion. (WebCore::Position::moveToPosition): Ditto.
• dom/PositionIterator.cpp: (WebCore::PositionIterator::operator Position): Avoid creating a position immediately below a node whose content is ignored by editing. While this does not avoid creation of positions inside ignored contents completely, it works in most cases. Filed the bug 56027 to resolve the underlying problem. Without this change, the assertion hits in existing layout tests. cannot be tested directly.
• editing/ApplyStyleCommand.cpp: (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded): Call firstPositionInOrBeforeNode instead of firstPositionInNode because startNode may as well be a br element. Without this change, the assertion hits in existing layout tests.
• editing/htmlediting.cpp: (WebCore::canHaveChildrenForEditing): button is editable so content is not ignored. Added a test for this.
• editing/visible_units.cpp: (WebCore::previousBoundary): Added a FIXME. (WebCore::startPositionForLine): Because br can also have an inline text box, checking that startBox is an inline text box isn't an adequate to instantiate a position inside startNode. Call startNode->isTextNode() instead. Without this change, the assertion hits in existing layout tests.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/execCommand/button-expected.txt (added)
• LayoutTests/editing/execCommand/button.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Position.cpp (modified) (2 diffs)
• Source/WebCore/dom/PositionIterator.cpp (modified) (1 diff)
• Source/WebCore/editing/ApplyStyleCommand.cpp (modified) (1 diff)
• Source/WebCore/editing/htmlediting.cpp (modified) (1 diff)
• Source/WebCore/editing/visible_units.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-03-17  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Justin Garcia.
+
+        Assert that editing does not ignore position's anchorNode if position is an offset in anchor
+        https://bugs.webkit.org/show_bug.cgi?id=56027
+
+        Added a test to ensure WebKit lets users edit contents inside a button element properly.
+
+        * editing/execCommand/button-expected.txt: Added.
+        * editing/execCommand/button.html: Added.
+
 2011-03-17  Adam Roben  <aroben@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-03-17  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Justin Garcia.
+
+        Assert that editing does not ignore position's anchorNode if position is an offset in anchor
+        https://bugs.webkit.org/show_bug.cgi?id=56027
+
+        Added the assertion in Position::Position and Position::moveToPosition. This assertion catches
+        places where we instantiate positions inside a node on which editingIgnoresContent returns true.
+
+        Test: editing/execCommand/button.html
+
+        * dom/Position.cpp:
+        (WebCore::Position::Position): Added an assertion.
+        (WebCore::Position::moveToPosition): Ditto.
+        * dom/PositionIterator.cpp:
+        (WebCore::PositionIterator::operator Position): Avoid creating a position immediately below
+        a node whose content is ignored by editing. While this does not avoid creation of positions
+        inside ignored contents completely, it works in most cases. Filed the bug 56027 to resolve
+        the underlying problem. Without this change, the assertion hits in existing layout tests.
+        cannot be tested directly.
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded): Call firstPositionInOrBeforeNode
+        instead of firstPositionInNode because startNode may as well be a br element. Without this change,
+        the assertion hits in existing layout tests.
+        * editing/htmlediting.cpp:
+        (WebCore::canHaveChildrenForEditing): button is editable so content is not ignored. Added a test
+        for this.
+        * editing/visible_units.cpp:
+        (WebCore::previousBoundary): Added a FIXME.
+        (WebCore::startPositionForLine): Because br can also have an inline text box, checking that
+        startBox is an inline text box isn't an adequate to instantiate a position inside startNode.
+        Call startNode->isTextNode() instead. Without this change, the assertion hits in existing layout
+        tests.
+
 2011-03-17  Pavel Podivilov  <podivilov@chromium.org>
 

trunk/Source/WebCore/dom/Position.cpp

@@ -95 +95 @@
     , m_isLegacyEditingPosition(false)
 {
+    ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get()));
     ASSERT(anchorType == PositionIsOffsetInAnchor);
 }
@@ -100 +101 @@
 void Position::moveToPosition(PassRefPtr<Node> node, int offset)
 {
+    ASSERT(!editingIgnoresContent(node.get()));
     ASSERT(anchorType() == PositionIsOffsetInAnchor || m_isLegacyEditingPosition);
     m_anchorNode = node;

trunk/Source/WebCore/dom/PositionIterator.cpp

@@ -39 +39 @@
     if (m_nodeAfterPositionInAnchor) {
         ASSERT(m_nodeAfterPositionInAnchor->parentNode() == m_anchorNode);
+        // FIXME: This check is inadaquete because any ancestor could be ignored by editing
+        if (editingIgnoresContent(m_nodeAfterPositionInAnchor->parentNode()))
+            return positionBeforeNode(m_anchorNode);
         return positionInParentBeforeNode(m_nodeAfterPositionInAnchor);
     }

trunk/Source/WebCore/editing/ApplyStyleCommand.cpp

@@ -1697 +1697 @@
         positionForStyleComparison = positionBeforeNode(dummyElement.get());
     } else
-        positionForStyleComparison = firstPositionInNode(startNode.get());
+        positionForStyleComparison = firstPositionInOrBeforeNode(startNode.get());
 
     StyleChange styleChange(style, positionForStyleComparison);

trunk/Source/WebCore/editing/htmlediting.cpp

@@ -77 +77 @@
         && !node->hasTagName(brTag)
         && !node->hasTagName(imgTag)
-        && !node->hasTagName(buttonTag)
         && !node->hasTagName(inputTag)
         && !node->hasTagName(textareaTag)

trunk/Source/WebCore/editing/visible_units.cpp

@@ -127 +127 @@
     BackwardsCharacterIterator charIt(searchRange.get());
     charIt.advance(string.size() - suffixLength - next);
+    // FIXME: charIt can get out of shadow host.
     return VisiblePosition(charIt.range()->endPosition(), DOWNSTREAM);
 }
@@ -378 +379 @@
     }
     
-    VisiblePosition visPos = startBox->isInlineTextBox() ? VisiblePosition(Position(startNode, static_cast<InlineTextBox *>(startBox)->start(), Position::PositionIsOffsetInAnchor), DOWNSTREAM)
-                                                         : VisiblePosition(positionBeforeNode(startNode), DOWNSTREAM);
+    VisiblePosition visPos = startNode->isTextNode() ? VisiblePosition(Position(startNode, static_cast<InlineTextBox *>(startBox)->start(), Position::PositionIsOffsetInAnchor), DOWNSTREAM)
+                                                     : VisiblePosition(positionBeforeNode(startNode), DOWNSTREAM);
     return positionAvoidingFirstPositionInTable(visPos);
 }