isEditablePosition and related functions shouldn't move position out of table
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Feb 2015 05:51:31 +0000 (05:51 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Feb 2015 05:51:31 +0000 (05:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=129200

Reviewed by Darin Adler.

Source/WebCore:

This patch removes the legacy editing position for elements display: table in its computed style.
Previously, we used (table, 0) and (table, !0) to denote positions immediately before and after
such an element for historical reasons. This forced us to update the style tree before computing
the editability of a position because we have to check the editability of the position outside
the element with display: table if the position was using such a legacy editing position.
e.g. if a table was not editable (contenteditable=false), the position before the table (table, 0)
should still be considered editable if the parent node of the table was editable.

This patch replaces such a legacy editing position by using modern position types:
PositionIsBeforeAnchor and PositionIsAfterAnchor.

No new tests since there should be no change in the user perceived editing operations.

* dom/Position.cpp:
(WebCore::Position::previous): Setup the node and the offset correctly when the original position's
type is PositionIsBeforeAnchor. Also return a position before or after node when the node we found
is "atomic" (e.g. input, img, br, etc...) or it's a table. This avoids creating a legacy editing
position inside a table.
(WebCore::Position::next): Ditto.
(WebCore::Position::atStartOfTree): Use atFirstEditingPositionForNode, which takes care of all types
of positions.
(WebCore::Position::atEndOfTree): Ditto.
(WebCore::Position::downstream): Return a position before a node instead of a legacy editing position
for an atomic element or a table element as done in the equivalent code in Position::upstream.
(WebCore::Position::isCandidate): Don't treat a position inside a table to be a candidate. e.g.
(table, 1) when there are more than two children of the table.

* dom/PositionIterator.cpp:
(WebCore::PositionIterator::operator Position): PositionIterator internally uses legacy editing
positions. So convert it to a modern position by returning a position before or after a table here.
* editing/ApplyBlockElementCommand.cpp:
(WebCore::ApplyBlockElementCommand::formatSelection): Check that the unsplittable element we found
is actually empty before executing the simple code path for an empty unsplittable element. Without
this check, block formatting a table element will fail.

* editing/htmlediting.cpp:
(WebCore::isEditablePosition): Use containerNode instead of deprecatedNode because the editability
of a position before or after an element is determined by its parent, not the element itself.
(WebCore::isAtUnsplittableElement): Ditto.
(WebCore::isRichlyEditablePosition): Ditto. Removed the code that moved the starting node out of
an element with display: table. This is the code removal for which this patch was made.
(WebCore::editableRootForPosition): Ditto.

LayoutTests:

Rebaselined a test. There is no visual difference.

* platform/mac/editing/inserting/5058163-1-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@180726 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/mac/editing/inserting/5058163-1-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/dom/Position.cpp
Source/WebCore/dom/PositionIterator.cpp
Source/WebCore/editing/ApplyBlockElementCommand.cpp
Source/WebCore/editing/htmlediting.cpp
Source/WebCore/editing/htmlediting.h

index 47bc0098706552b23962abef0532bf05fbf8e24c..96829d88cba210a98c2bcb19c4c5c2428e7ccdbb 100644 (file)
@@ -1,3 +1,14 @@
+2015-02-26  Ryosuke Niwa  <rniwa@webkit.org>
+
+        isEditablePosition and related functions shouldn't move position out of table
+        https://bugs.webkit.org/show_bug.cgi?id=129200
+
+        Reviewed by Darin Adler.
+
+        Rebaselined a test. There is no visual difference.
+
+        * platform/mac/editing/inserting/5058163-1-expected.txt:
+
 2015-02-26  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] More test expectation updates.
index 8eb6473e50598aacc1eb0eb48e00afe9b9f1d712..fe7b323bb6c8abac03e126a8d13a82a0e3e28d67 100644 (file)
@@ -13,9 +13,10 @@ layer at (0,0) size 800x600
               RenderTableCell {TD} at (2,2) size 478x20 [r=0 c=0 rs=1 cs=1]
                 RenderText {#text} at (1,1) size 476x18
                   text run at (1,1) width 476: "There should be two empty paragraphs after this table and before the next."
-        RenderBlock (anonymous) at (0,26) size 784x36
+        RenderBlock {DIV} at (0,26) size 784x18
+          RenderBR {BR} at (0,0) size 0x18
+        RenderBlock (anonymous) at (0,44) size 784x18
           RenderBR {BR} at (0,0) size 0x18
-          RenderBR {BR} at (0,18) size 0x18
         RenderTable {TABLE} at (0,62) size 280x26 [border: (1px solid #AAAAAA)]
           RenderTableSection {TBODY} at (1,1) size 278x24
             RenderTableRow {TR} at (0,2) size 278x20
index 03ea2a08751d6d14069f77b3f56a658c6665f38e..fc8174696ad70d3140c9b01d5e04c47ad7fd7c2b 100644 (file)
@@ -1,3 +1,53 @@
+2015-02-26  Ryosuke Niwa  <rniwa@webkit.org>
+
+        isEditablePosition and related functions shouldn't move position out of table
+        https://bugs.webkit.org/show_bug.cgi?id=129200
+
+        Reviewed by Darin Adler.
+
+        This patch removes the legacy editing position for elements display: table in its computed style.
+        Previously, we used (table, 0) and (table, !0) to denote positions immediately before and after
+        such an element for historical reasons. This forced us to update the style tree before computing
+        the editability of a position because we have to check the editability of the position outside
+        the element with display: table if the position was using such a legacy editing position.
+        e.g. if a table was not editable (contenteditable=false), the position before the table (table, 0)
+        should still be considered editable if the parent node of the table was editable.
+
+        This patch replaces such a legacy editing position by using modern position types:
+        PositionIsBeforeAnchor and PositionIsAfterAnchor.
+
+        No new tests since there should be no change in the user perceived editing operations.
+
+        * dom/Position.cpp:
+        (WebCore::Position::previous): Setup the node and the offset correctly when the original position's
+        type is PositionIsBeforeAnchor. Also return a position before or after node when the node we found
+        is "atomic" (e.g. input, img, br, etc...) or it's a table. This avoids creating a legacy editing
+        position inside a table.
+        (WebCore::Position::next): Ditto.
+        (WebCore::Position::atStartOfTree): Use atFirstEditingPositionForNode, which takes care of all types
+        of positions.
+        (WebCore::Position::atEndOfTree): Ditto.
+        (WebCore::Position::downstream): Return a position before a node instead of a legacy editing position
+        for an atomic element or a table element as done in the equivalent code in Position::upstream.
+        (WebCore::Position::isCandidate): Don't treat a position inside a table to be a candidate. e.g.
+        (table, 1) when there are more than two children of the table.
+
+        * dom/PositionIterator.cpp:
+        (WebCore::PositionIterator::operator Position): PositionIterator internally uses legacy editing
+        positions. So convert it to a modern position by returning a position before or after a table here.
+        * editing/ApplyBlockElementCommand.cpp:
+        (WebCore::ApplyBlockElementCommand::formatSelection): Check that the unsplittable element we found
+        is actually empty before executing the simple code path for an empty unsplittable element. Without
+        this check, block formatting a table element will fail.
+
+        * editing/htmlediting.cpp:
+        (WebCore::isEditablePosition): Use containerNode instead of deprecatedNode because the editability
+        of a position before or after an element is determined by its parent, not the element itself.
+        (WebCore::isAtUnsplittableElement): Ditto.
+        (WebCore::isRichlyEditablePosition): Ditto. Removed the code that moved the starting node out of
+        an element with display: table. This is the code removal for which this patch was made.
+        (WebCore::editableRootForPosition): Ditto.
+
 2015-02-26  Timothy Horton  <timothy_horton@apple.com>
 
         Implement <attachment> element appearance on Mac
index 962bb0e899d5e7b8849eef5dae988f29922d55a3..1fd42652a3fda85167a76a91d2dc64fca15304d2 100644 (file)
@@ -308,6 +308,11 @@ Position Position::previous(PositionMoveType moveType) const
     // FIXME: Negative offsets shouldn't be allowed. We should catch this earlier.
     ASSERT(offset >= 0);
 
+    if (anchorType() == PositionIsBeforeAnchor) {
+        node = containerNode();
+        offset = computeOffsetInContainerNode();
+    }
+
     if (offset > 0) {
         if (Node* child = node->traverseToChildAt(offset - 1))
             return lastPositionInOrAfterNode(child);
@@ -331,6 +336,13 @@ Position Position::previous(PositionMoveType moveType) const
     if (!parent)
         return *this;
 
+    if (positionBeforeOrAfterNodeIsCandidate(node))
+        return positionBeforeNode(node);
+
+    Node* previousSibling = node->previousSibling();
+    if (previousSibling && positionBeforeOrAfterNodeIsCandidate(previousSibling))
+        return positionAfterNode(previousSibling);
+
     return createLegacyEditingPosition(parent, node->computeNodeIndex());
 }
 
@@ -346,6 +358,11 @@ Position Position::next(PositionMoveType moveType) const
     // FIXME: Negative offsets shouldn't be allowed. We should catch this earlier.
     ASSERT(offset >= 0);
 
+    if (anchorType() == PositionIsAfterAnchor) {
+        node = containerNode();
+        offset = computeOffsetInContainerNode();
+    }
+
     Node* child = node->traverseToChildAt(offset);
     if (child || (!node->hasChildNodes() && offset < lastOffsetForEditing(node))) {
         if (child)
@@ -363,6 +380,13 @@ Position Position::next(PositionMoveType moveType) const
     if (!parent)
         return *this;
 
+    if (isRenderedTable(node) || editingIgnoresContent(node))
+        return positionAfterNode(node);
+
+    Node* nextSibling = node->nextSibling();
+    if (nextSibling && positionBeforeOrAfterNodeIsCandidate(nextSibling))
+        return positionBeforeNode(nextSibling);
+
     return createLegacyEditingPosition(parent, node->computeNodeIndex() + 1);
 }
 
@@ -452,14 +476,14 @@ bool Position::atStartOfTree() const
 {
     if (isNull())
         return true;
-    return !findParent(deprecatedNode()) && m_offset <= 0;
+    return !findParent(containerNode()) && atFirstEditingPositionForNode();
 }
 
 bool Position::atEndOfTree() const
 {
     if (isNull())
         return true;
-    return !findParent(deprecatedNode()) && m_offset >= lastOffsetForEditing(deprecatedNode());
+    return !findParent(containerNode()) && atLastEditingPositionForNode();
 }
 
 // return first preceding DOM position rendered at a different location, or "this"
@@ -754,7 +778,7 @@ Position Position::downstream(EditingBoundaryCrossingRule rule) const
         // Return position before tables and nodes which have content that can be ignored.
         if (editingIgnoresContent(currentNode) || isRenderedTable(currentNode)) {
             if (currentPos.offsetInLeafNode() <= renderer->caretMinOffset())
-                return createLegacyEditingPosition(currentNode, renderer->caretMinOffset());
+                return positionBeforeNode(currentNode);
             continue;
         }
 
@@ -931,8 +955,11 @@ bool Position::isCandidate() const
     if (is<RenderText>(*renderer))
         return !nodeIsUserSelectNone(deprecatedNode()) && downcast<RenderText>(*renderer).containsCaretOffset(m_offset);
 
-    if (isRenderedTable(deprecatedNode()) || editingIgnoresContent(deprecatedNode()))
-        return (atFirstEditingPositionForNode() || atLastEditingPositionForNode()) && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
+    if (positionBeforeOrAfterNodeIsCandidate(deprecatedNode())) {
+        return ((atFirstEditingPositionForNode() && m_anchorType == PositionIsBeforeAnchor)
+            || (atLastEditingPositionForNode() && m_anchorType == PositionIsAfterAnchor))
+            && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
+    }
 
     if (m_anchorNode->hasTagName(htmlTag))
         return false;
index c8de033d0e7d07265cb01d94a845fb1984561ae6..1f009670f2acdc9aa771368415751cad75852265 100644 (file)
@@ -43,10 +43,12 @@ PositionIterator::operator Position() const
     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()))
+        if (positionBeforeOrAfterNodeIsCandidate(m_anchorNode))
             return positionBeforeNode(m_anchorNode);
         return positionInParentBeforeNode(m_nodeAfterPositionInAnchor);
     }
+    if (positionBeforeOrAfterNodeIsCandidate(m_anchorNode))
+        return atStartOfNode() ? positionBeforeNode(m_anchorNode) : positionAfterNode(m_anchorNode);
     if (m_anchorNode->hasChildNodes())
         return lastPositionInOrAfterNode(m_anchorNode);
     return createLegacyEditingPosition(m_anchorNode, m_offsetInAnchor);
index f740ec3ac647ed0c69f19839aa034dc17acc5034..c6de1ddf33c1eb06f381ef551233c8633ebd6a18 100644 (file)
@@ -103,7 +103,7 @@ void ApplyBlockElementCommand::formatSelection(const VisiblePosition& startOfSel
     // Special case empty unsplittable elements because there's nothing to split
     // and there's nothing to move.
     Position start = startOfSelection.deepEquivalent().downstream();
-    if (isAtUnsplittableElement(start)) {
+    if (isAtUnsplittableElement(start) && startOfParagraph(start) == endOfParagraph(endOfSelection)) {
         RefPtr<Element> blockquote = createBlockElement();
         insertNodeAt(blockquote, start);
         RefPtr<Element> placeholder = createBreakElement(document());
index 87c9de96818eb300a88ad96b69040b2117de6b74..23ec90bed08fd16e753cc2cbd12c4dbce6bf6798 100644 (file)
@@ -143,7 +143,7 @@ Node* lowestEditableAncestor(Node* node)
 
 bool isEditablePosition(const Position& p, EditableType editableType, EUpdateStyle updateStyle)
 {
-    Node* node = p.deprecatedNode();
+    Node* node = p.containerNode();
     if (!node)
         return false;
     if (updateStyle == UpdateStyle)
@@ -151,28 +151,22 @@ bool isEditablePosition(const Position& p, EditableType editableType, EUpdateSty
     else
         ASSERT(updateStyle == DoNotUpdateStyle);
 
-    if (node->renderer() && node->renderer()->isTable())
-        node = node->parentNode();
-
     return node->hasEditableStyle(editableType);
 }
 
 bool isAtUnsplittableElement(const Position& pos)
 {
-    Node* node = pos.deprecatedNode();
+    Node* node = pos.containerNode();
     return (node == editableRootForPosition(pos) || node == enclosingNodeOfType(pos, &isTableCell));
 }
     
     
 bool isRichlyEditablePosition(const Position& p, EditableType editableType)
 {
-    Node* node = p.deprecatedNode();
+    Node* node = p.containerNode();
     if (!node)
         return false;
-        
-    if (node->renderer() && node->renderer()->isTable())
-        node = node->parentNode();
-    
+
     return node->hasRichlyEditableStyle(editableType);
 }
 
@@ -181,10 +175,7 @@ Element* editableRootForPosition(const Position& p, EditableType editableType)
     Node* node = p.containerNode();
     if (!node)
         return 0;
-        
-    if (node->renderer() && node->renderer()->isTable())
-        node = node->parentNode();
-    
+
     return node->rootEditableElement(editableType);
 }
 
index ed415766e06c5baf8f01f2a45241afe60c523e51..ce895e32734c17fe9ea7483fd6998cd4c6292bdf 100644 (file)
@@ -120,6 +120,11 @@ bool isRenderedAsNonInlineTableImageOrHR(const Node*);
 bool areIdenticalElements(const Node*, const Node*);
 bool isNonTableCellHTMLBlockElement(const Node*);
 
+inline bool positionBeforeOrAfterNodeIsCandidate(Node* node)
+{
+    return isRenderedTable(node) || editingIgnoresContent(node);
+}
+
 WEBCORE_EXPORT TextDirection directionOfEnclosingBlock(const Position&);
 
 // -------------------------------------------------------------------------