Remove conditional compile guard for InsertIntoTextNodeCommand::doReapply
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Feb 2019 01:13:50 +0000 (01:13 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Feb 2019 01:13:50 +0000 (01:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195067
<rdar://problem/44812080>

Reviewed by Tim Horton.

Source/WebCore:

This iOS-specific override was introduced to fix <rdar://problem/7114425>, in which the last typed character
would be revealed when redoing text input on iOS inside a password field. The associated change fixed this bug
by overriding doReapply on iOS to only insert text (instead of additionally handling password echo); however, it
really makes sense to skip password echo when redoing on all platforms, so we can just remove the platform-
specific guards around this logic.

Doing this allows us to add the `hasEditableStyle()` check on iOS when redoing text insertion, which results in
a very subtle behavior change covered by the new layout test below.

Test: editing/undo/redo-text-insertion-in-non-editable-node.html

* editing/InsertIntoTextNodeCommand.cpp:
(WebCore::InsertIntoTextNodeCommand::doReapply):
* editing/InsertIntoTextNodeCommand.h:

LayoutTests:

Add a new layout test to verify that redoing text insertion in a non-editable element (which was previously
editable) does not mutate the text nodes affected by editing. This test case currently fails on iOS, since we
take a separate codepath when redoing that does not contain this additional check.

* editing/undo/redo-text-insertion-in-non-editable-node-expected.txt: Added.
* editing/undo/redo-text-insertion-in-non-editable-node.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/undo/redo-text-insertion-in-non-editable-node-expected.txt [new file with mode: 0644]
LayoutTests/editing/undo/redo-text-insertion-in-non-editable-node.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/InsertIntoTextNodeCommand.cpp
Source/WebCore/editing/InsertIntoTextNodeCommand.h

index 6535c3e..4557ea9 100644 (file)
@@ -1,3 +1,18 @@
+2019-02-26  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Remove conditional compile guard for InsertIntoTextNodeCommand::doReapply
+        https://bugs.webkit.org/show_bug.cgi?id=195067
+        <rdar://problem/44812080>
+
+        Reviewed by Tim Horton.
+
+        Add a new layout test to verify that redoing text insertion in a non-editable element (which was previously
+        editable) does not mutate the text nodes affected by editing. This test case currently fails on iOS, since we
+        take a separate codepath when redoing that does not contain this additional check.
+
+        * editing/undo/redo-text-insertion-in-non-editable-node-expected.txt: Added.
+        * editing/undo/redo-text-insertion-in-non-editable-node.html: Added.
+
 2019-02-26  Youenn Fablet  <youenn@apple.com>
 
         Move service worker response validation from the service worker client to the service worker itself
diff --git a/LayoutTests/editing/undo/redo-text-insertion-in-non-editable-node-expected.txt b/LayoutTests/editing/undo/redo-text-insertion-in-non-editable-node-expected.txt
new file mode 100644 (file)
index 0000000..4519f59
--- /dev/null
@@ -0,0 +1,11 @@
+This test verifies that redoing text insertion in a non-editable element is a no-op.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS textNode.data is "Hello"
+PASS textNode.data is ""
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/undo/redo-text-insertion-in-non-editable-node.html b/LayoutTests/editing/undo/redo-text-insertion-in-non-editable-node.html
new file mode 100644 (file)
index 0000000..1488626
--- /dev/null
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test.js"></script>
+</head>
+<body>
+<div id="editor" contenteditable></div>
+</body>
+<script>
+description("This test verifies that redoing text insertion in a non-editable element is a no-op.");
+editor.focus();
+
+document.execCommand("InsertText", true, "Hello");
+const textNode = editor.firstChild;
+shouldBeEqualToString("textNode.data", "Hello");
+
+document.execCommand("Undo");
+editor.setAttribute("contenteditable", "false");
+document.execCommand("Redo");
+
+shouldBeEqualToString("textNode.data", "");
+</script>
+</html>
index 1882153..e30a4e0 100644 (file)
@@ -1,3 +1,26 @@
+2019-02-26  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Remove conditional compile guard for InsertIntoTextNodeCommand::doReapply
+        https://bugs.webkit.org/show_bug.cgi?id=195067
+        <rdar://problem/44812080>
+
+        Reviewed by Tim Horton.
+
+        This iOS-specific override was introduced to fix <rdar://problem/7114425>, in which the last typed character
+        would be revealed when redoing text input on iOS inside a password field. The associated change fixed this bug
+        by overriding doReapply on iOS to only insert text (instead of additionally handling password echo); however, it
+        really makes sense to skip password echo when redoing on all platforms, so we can just remove the platform-
+        specific guards around this logic.
+
+        Doing this allows us to add the `hasEditableStyle()` check on iOS when redoing text insertion, which results in
+        a very subtle behavior change covered by the new layout test below.
+
+        Test: editing/undo/redo-text-insertion-in-non-editable-node.html
+
+        * editing/InsertIntoTextNodeCommand.cpp:
+        (WebCore::InsertIntoTextNodeCommand::doReapply):
+        * editing/InsertIntoTextNodeCommand.h:
+
 2019-02-26  Keith Miller  <keith_miller@apple.com>
 
         Code quality cleanup in NeverDestroyed
index ba5d28e..73076de 100644 (file)
@@ -65,18 +65,14 @@ void InsertIntoTextNodeCommand::doApply()
     m_node->insertData(m_offset, m_text);
 }
 
-#if PLATFORM(IOS_FAMILY)
-
-// FIXME: Why would reapply be iOS-specific?
 void InsertIntoTextNodeCommand::doReapply()
 {
-    // FIXME: Shouldn't this have a hasEditableStyle check?
+    if (!m_node->hasEditableStyle())
+        return;
 
     m_node->insertData(m_offset, m_text);
 }
 
-#endif
-    
 void InsertIntoTextNodeCommand::doUnapply()
 {
     if (!m_node->hasEditableStyle())
index a54c172..236dd6f 100644 (file)
@@ -46,9 +46,7 @@ protected:
 private:
     void doApply() override;
     void doUnapply() override;
-#if PLATFORM(IOS_FAMILY)
     void doReapply() override;
-#endif
     
 #ifndef NDEBUG
     void getNodesInCommand(HashSet<Node*>&) override;