Indenting an indented image element resulted in an extra indentation
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Feb 2014 02:23:43 +0000 (02:23 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Feb 2014 02:23:43 +0000 (02:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=129201

Reviewed by Enrica Casucci.

Source/WebCore:

The bug was caused by endOfParagraph returning a position at the beginning of a block when the position
passed into the function was at the beginning of the block. Consider the following DOM:
<blockquote><img></blockquote>

When endOfParagraph is called on (blockquote, 0), the condition r->isBR() || isBlock(n) in endOfParagraph
matches immediately on startNode and it returns (blockquote, 0) again.

This resulted in moveParagraphWithClones invoked by indentIntoBlockquote to erroneously clone the inner
blockquote. Worked around this bug in ApplyBlockElementCommand::formatSelection by checking this specific
condition and moving the position to the end of the block. Unfortunately, a lot of existing code depends
on the current behavior of endOfParagraph so fixing the function itself was not possible.

There was another bug in indentIntoBlockquote to incorrectly insert a new blockquote into the existing
blockquote due to the code introduced in r99594 to avoid inserting before the root editable element.
Since this happens only if outerBlock is the root editable element, which is nodeToSplitTo or an ancestor
of nodeToSplitTo, explicitly look for this condition.

Test: editing/execCommand/indent-img-twice.html

* editing/ApplyBlockElementCommand.cpp:
(WebCore::ApplyBlockElementCommand::formatSelection):
(WebCore::isNewLineAtPosition):
* editing/IndentOutdentCommand.cpp:
(WebCore::IndentOutdentCommand::indentIntoBlockquote):
* editing/VisibleUnits.cpp:
(WebCore::endOfParagraph): Added a FIXME.

LayoutTests:

Added a regression test.

* editing/execCommand/indent-img-twice-expected.txt: Added.
* editing/execCommand/indent-img-twice.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/indent-img-twice-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/indent-img-twice.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/ApplyBlockElementCommand.cpp
Source/WebCore/editing/IndentOutdentCommand.cpp
Source/WebCore/editing/VisibleUnits.cpp

index e17d7d53961c1b06a13ef8fa5548e2b3d575cdb6..dee178eec75bb6246e8cdf2db8b7737df15fcddd 100644 (file)
@@ -1,3 +1,15 @@
+2014-02-26  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Indenting an indented image element resulted in an extra indentation
+        https://bugs.webkit.org/show_bug.cgi?id=129201
+
+        Reviewed by Enrica Casucci.
+
+        Added a regression test.
+
+        * editing/execCommand/indent-img-twice-expected.txt: Added.
+        * editing/execCommand/indent-img-twice.html: Added.
+
 2014-02-26  Bem Jones-Bey  <bjonesbe@adobe.com>
 
         [CSS Shapes] inset and inset-rectangle trigger assert with replaced element and large percentage dimension
diff --git a/LayoutTests/editing/execCommand/indent-img-twice-expected.txt b/LayoutTests/editing/execCommand/indent-img-twice-expected.txt
new file mode 100644 (file)
index 0000000..8aaba8e
--- /dev/null
@@ -0,0 +1,23 @@
+Test indenting an image element twice.
+
+Initial state:
+| <img>
+|   src="../resources/abe.png"
+
+After indenting once:
+| <blockquote>
+|   style="margin: 0 0 0 40px; border: none; padding: 0px;"
+|   <#selection-anchor>
+|   <img>
+|     src="../resources/abe.png"
+|   <#selection-focus>
+
+After indenting again:
+| <blockquote>
+|   style="margin: 0 0 0 40px; border: none; padding: 0px;"
+|   <blockquote>
+|     style="margin: 0 0 0 40px; border: none; padding: 0px;"
+|     <#selection-anchor>
+|     <img>
+|       src="../resources/abe.png"
+|     <#selection-focus>
diff --git a/LayoutTests/editing/execCommand/indent-img-twice.html b/LayoutTests/editing/execCommand/indent-img-twice.html
new file mode 100644 (file)
index 0000000..3543d2e
--- /dev/null
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/dump-as-markup.js"></script>
+<div id="editor" contenteditable><img src="../resources/abe.png"></div>
+<script>
+
+Markup.description('Test indenting an image element twice.');
+
+var editor = document.getElementById('editor');
+editor.focus();
+document.execCommand('SelectAll', false, null);
+Markup.dump(editor, 'Initial state');
+
+document.execCommand('Indent', false, null);
+Markup.dump(editor, 'After indenting once');
+
+document.execCommand('Indent', false, null);
+Markup.dump(editor, 'After indenting again');
+
+</script>
+</body>
+</html>
index 29c5852db58fd68fedd645aa24d26c599da0189c..99e94cb84566efa66c1ab417769602d85132a2be 100644 (file)
@@ -1,3 +1,37 @@
+2014-02-26  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Indenting an indented image element resulted in an extra indentation
+        https://bugs.webkit.org/show_bug.cgi?id=129201
+
+        Reviewed by Enrica Casucci.
+
+        The bug was caused by endOfParagraph returning a position at the beginning of a block when the position
+        passed into the function was at the beginning of the block. Consider the following DOM:
+        <blockquote><img></blockquote>
+
+        When endOfParagraph is called on (blockquote, 0), the condition r->isBR() || isBlock(n) in endOfParagraph
+        matches immediately on startNode and it returns (blockquote, 0) again.
+
+        This resulted in moveParagraphWithClones invoked by indentIntoBlockquote to erroneously clone the inner
+        blockquote. Worked around this bug in ApplyBlockElementCommand::formatSelection by checking this specific
+        condition and moving the position to the end of the block. Unfortunately, a lot of existing code depends
+        on the current behavior of endOfParagraph so fixing the function itself was not possible.
+
+        There was another bug in indentIntoBlockquote to incorrectly insert a new blockquote into the existing
+        blockquote due to the code introduced in r99594 to avoid inserting before the root editable element.
+        Since this happens only if outerBlock is the root editable element, which is nodeToSplitTo or an ancestor
+        of nodeToSplitTo, explicitly look for this condition.
+
+        Test: editing/execCommand/indent-img-twice.html
+
+        * editing/ApplyBlockElementCommand.cpp:
+        (WebCore::ApplyBlockElementCommand::formatSelection):
+        (WebCore::isNewLineAtPosition):
+        * editing/IndentOutdentCommand.cpp:
+        (WebCore::IndentOutdentCommand::indentIntoBlockquote):
+        * editing/VisibleUnits.cpp:
+        (WebCore::endOfParagraph): Added a FIXME.
+
 2014-02-26  Simon Fraser  <simon.fraser@apple.com>
 
         Fix two assertions/crashes in compositing code
index b58d53c0ba680a63c8e3016c510a90b78f9c7d68..b6938e42f189a084acfe2bcd64211399da3d1361 100644 (file)
@@ -126,6 +126,14 @@ void ApplyBlockElementCommand::formatSelection(const VisiblePosition& startOfSel
         rangeForParagraphSplittingTextNodesIfNeeded(endOfCurrentParagraph, start, end);
         endOfCurrentParagraph = end;
 
+        // FIXME: endOfParagraph can errornously return a position at the beginning of a block element
+        // when the position passed into endOfParagraph is at the beginning of a block.
+        // Work around this bug here because too much of the existing code depends on the current behavior of endOfParagraph.
+        if (start == end && startOfBlock(start) != endOfBlock(start) && !isEndOfBlock(end) && start == startOfParagraph(endOfBlock(start))) {
+            endOfCurrentParagraph = endOfBlock(end);
+            end = endOfCurrentParagraph.deepEquivalent();
+        }
+
         Position afterEnd = end.next();
         Node* enclosingCell = enclosingNodeOfType(start, &isTableCell);
         VisiblePosition endOfNextParagraph = endOfNextParagrahSplittingTextNodesIfNeeded(endOfCurrentParagraph, start, end);
index c233623b25195ce1682042776f965843dfa35635..de50e75c4b87b36efe494f3b77a0646f346f9597 100644 (file)
@@ -108,7 +108,7 @@ void IndentOutdentCommand::indentIntoBlockquote(const Position& start, const Pos
         // Create a new blockquote and insert it as a child of the root editable element. We accomplish
         // this by splitting all parents of the current paragraph up to that point.
         targetBlockquote = createBlockElement();
-        if (outerBlock == start.containerNode())
+        if (outerBlock == nodeToSplitTo)
             insertNodeAt(targetBlockquote, start);
         else
             insertNodeBefore(targetBlockquote, outerBlock);
index bfa31f91025a5ccd345d1b11e696c74854e128d1..4b4954126cff92cdc192abd1a4383821c98e771f 100644 (file)
@@ -1231,6 +1231,7 @@ VisiblePosition endOfParagraph(const VisiblePosition& c, EditingBoundaryCrossing
             continue;
         }
         
+        // FIXME: This is wrong when startNode is a block. We should return a position after the block.
         if (r->isBR() || isBlock(n))
             break;