Changeset 148597 in webkit

Timestamp:

Apr 17, 2013 3:29:24 AM (11 years ago)

Author:

Claudio Saavedra

Message:

execCommand("RemoveFormat") might remove format after the selection
​https://bugs.webkit.org/show_bug.cgi?id=112240

Reviewed by Ryosuke Niwa.

Source/WebCore:

Tests: editing/execCommand/remove-format-multiple-elements-mac.html

This bug is hit when ApplyStyleCommand is used to change the
style and the current selection ends in the beginning of a new node.
The bug is actually a two-fold thing:

1. There was no check as to whether the end node is really

selected or not, and format was always removed from it with
pushDownInlineStyleAroundNode(). An equivalent check for the start
node was already in place, so fix it analogously.

2. Previous stage might change the dom tree, resulting in a render

tree that is not up-to-date. Position::upstream() is later used
and, in order to be able to find a visually equivalent position in
a text node, this method needs the render tree to be up-to-date,
therefore, a call to updateLayoutIgnorePendingStylesheets() is
necessary.

• editing/ApplyStyleCommand.cpp:

(WebCore::ApplyStyleCommand::removeInlineStyle): Make sure that no
format is removed from the end node if it's not fully selected.
(WebCore::ApplyStyleCommand::nodeFullySelected): Call updateLayoutIgnorePendingStylesheets()

LayoutTests:

• editing/execCommand/remove-format-multiple-elements-mac-expected.txt: Updated.
• editing/execCommand/script-tests/remove-format-multiple-elements-mac.js:

(selectFirstLine): Add this method to check that RemoveFormat works when
a whole line is selected.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/execCommand/remove-format-multiple-elements-mac-expected.txt (modified) (1 diff)
• LayoutTests/editing/execCommand/script-tests/remove-format-multiple-elements-mac.js (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/ApplyStyleCommand.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-04-17  Claudio Saavedra  <csaavedra@igalia.com>
+
+        execCommand("RemoveFormat") might remove format after the selection
+        https://bugs.webkit.org/show_bug.cgi?id=112240
+
+        Reviewed by Ryosuke Niwa.
+
+        * editing/execCommand/remove-format-multiple-elements-mac-expected.txt: Updated.
+        * editing/execCommand/script-tests/remove-format-multiple-elements-mac.js:
+        (selectFirstLine): Add this method to check that RemoveFormat works when
+        a whole line is selected.
+
 2013-04-17  Zoltan Arvai  <zarvai@inf.u-szeged.hu>
 

trunk/LayoutTests/editing/execCommand/remove-format-multiple-elements-mac-expected.txt

@@ -17 +17 @@
 PASS RemoveFormat on last two words of "<q><b><div>hello world</div></b>WebKit</q>" yields "<div><q><b>hello </b></q>world</div>WebKit"
 PASS RemoveFormat on second word of "<q><b><div>hello world</div></b>WebKit</q>" yields "<div><q><b>hello </b></q>world</div><q>WebKit</q>"
+PASS RemoveFormat on first line of "<b><div>hello</div>webkit</b>" yields "<div>hello</div><b>webkit</b>"
 PASS RemoveFormat on all of "<i style="font-weight:bold;">hello</i> <u>world</u>" yields "hello world"
 PASS RemoveFormat on second word of "<font color="red"><b style="font-size: large;"><u>hello</u> world</b> WebKit</font>" yields "<font color="red"><b style="font-size: large;"><u>hello</u> </b></font>world<font color="red"> WebKit</font>"

trunk/LayoutTests/editing/execCommand/script-tests/remove-format-multiple-elements-mac.js

@@ -52 +52 @@
 }
 
+function selectFirstLine(container) {
+    window.getSelection().setPosition(container, 0);
+    window.getSelection().modify('extend', 'forward', 'line');
+    return 'first line';
+}
+
 testRemoveFormat('hello', selectAll, 'hello');
 testRemoveFormat('<i>hello</i> <u>world</u>', selectAll, 'hello world');
@@ -67 +73 @@
 testRemoveFormat('<q><b><div>hello world</div></b>WebKit</q>', selectLastTwoWords, '<div><q><b>hello </b></q>world</div>WebKit');
 testRemoveFormat('<q><b><div>hello world</div></b>WebKit</q>', selectSecondWord, '<div><q><b>hello </b></q>world</div><q>WebKit</q>');
+testRemoveFormat('<b><div>hello</div>webkit</b>', selectFirstLine, '<div>hello</div><b>webkit</b>');
 
 testRemoveFormat('<i style="font-weight:bold;">hello</i> <u>world</u>', selectAll, 'hello world');

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-04-17  Claudio Saavedra  <csaavedra@igalia.com>
+
+        execCommand("RemoveFormat") might remove format after the selection
+        https://bugs.webkit.org/show_bug.cgi?id=112240
+
+        Reviewed by Ryosuke Niwa.
+
+        Tests: editing/execCommand/remove-format-multiple-elements-mac.html
+
+        This bug is hit when ApplyStyleCommand is used to change the
+        style and the current selection ends in the beginning of a new node.
+        The bug is actually a two-fold thing:
+
+        1. There was no check as to whether the end node is really
+        selected or not, and format was always removed from it with
+        pushDownInlineStyleAroundNode(). An equivalent check for the start
+        node was already in place, so fix it analogously.
+
+        2. Previous stage might change the dom tree, resulting in a render
+        tree that is not up-to-date. Position::upstream() is later used
+        and, in order to be able to find a visually equivalent position in
+        a text node, this method needs the render tree to be up-to-date,
+        therefore, a call to updateLayoutIgnorePendingStylesheets() is
+        necessary.
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::removeInlineStyle): Make sure that no
+        format is removed from the end node if it's not fully selected.
+        (WebCore::ApplyStyleCommand::nodeFullySelected): Call updateLayoutIgnorePendingStylesheets()
+
 2013-04-17  Alberto Garcia  <agarcia@igalia.com>
 

trunk/Source/WebCore/editing/ApplyStyleCommand.cpp

@@ -1077 +1077 @@
     ASSERT(end.anchorNode()->inDocument());
     ASSERT(comparePositions(start, end) <= 0);
+    // FIXME: We should assert that start/end are not in the middle of a text node.
 
     Position pushDownStart = start.downstream();
@@ -1086 +1087 @@
         && pushDownStart.computeOffsetInContainerNode() == pushDownStartContainer->maxCharacterOffset())
         pushDownStart = nextVisuallyDistinctCandidate(pushDownStart);
+    // If pushDownEnd is at the start of a text node, then this node is not fully selected.
+    // Move it to the previous deep equivalent position to avoid removing the style from this node.
     Position pushDownEnd = end.upstream();
+    Node* pushDownEndContainer = pushDownEnd.containerNode();
+    if (pushDownEndContainer && pushDownEndContainer->isTextNode() && !pushDownEnd.computeOffsetInContainerNode())
+        pushDownEnd = previousVisuallyDistinctCandidate(pushDownEnd);
 
     pushDownInlineStyleAroundNode(style, pushDownStart.deprecatedNode());
@@ -1153 +1159 @@
     ASSERT(node->isElementNode());
 
+    // The tree may have changed and Position::upstream() relies on an up-to-date layout.
+    node->document()->updateLayoutIgnorePendingStylesheets();
+
     return comparePositions(firstPositionInOrBeforeNode(node), start) >= 0
         && comparePositions(lastPositionInOrAfterNode(node).upstream(), end) <= 0;