[webkit-changes] [264418] trunk

Wed, 15 Jul 2020 13:29:36 -0700

Title: [264418] trunk
Revision
264418
Author
[email protected]
Date
2020-07-15 13:28:06 -0700 (Wed, 15 Jul 2020)

Log Message

[iOS] Caret should be before text placeholder instead of after it
https://bugs.webkit.org/show_bug.cgi?id=214319
<rdar://problem/65295523>

Source/WebCore:

Reviewed by Wenson Hsieh.

Move the caret to before the text placeholder to match UIKit.

Tests: editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html
       editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html
       editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html

* editing/Editor.cpp:
(WebCore::Editor::insertTextPlaceholder): Compute the position before the placeholder in its parent.
This computation can only be done if the placeholder still has a parent. It may not after insertion
because arbitrary _javascript_ code can run. If this happens then bail out and return nullptr. Otherwise,
set the new selection.

LayoutTests:

Reviewed by Wenson Hsieh and Simon Fraser (1).

Add some tests.

[1] Simon only reviewed editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html.

* editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word-expected.txt: Added.
* editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html: Added.
* editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word-expected.txt: Added.
* editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html: Added.
* editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event-expected.txt: Added.
* editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html: Added.
* resources/js-test.js:
(shouldBeLessThanOrEqual): Added. Just like shouldBeGreaterThanOrEqual, but for <=.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (264417 => 264418)


--- trunk/LayoutTests/ChangeLog	2020-07-15 20:18:13 UTC (rev 264417)
+++ trunk/LayoutTests/ChangeLog	2020-07-15 20:28:06 UTC (rev 264418)
@@ -1,3 +1,24 @@
+2020-07-15  Daniel Bates  <[email protected]>
+
+        [iOS] Caret should be before text placeholder instead of after it
+        https://bugs.webkit.org/show_bug.cgi?id=214319
+        <rdar://problem/65295523>
+
+        Reviewed by Wenson Hsieh and Simon Fraser (1).
+
+        Add some tests.
+
+        [1] Simon only reviewed editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html.
+
+        * editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word-expected.txt: Added.
+        * editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html: Added.
+        * editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word-expected.txt: Added.
+        * editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html: Added.
+        * editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event-expected.txt: Added.
+        * editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html: Added.
+        * resources/js-test.js:
+        (shouldBeLessThanOrEqual): Added. Just like shouldBeGreaterThanOrEqual, but for <=.
+
 2020-07-15  Simon Fraser  <[email protected]>
 
         itsnicethat.com page is sometimes non-scrollable

Added: trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word-expected.txt (0 => 264418)


--- trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word-expected.txt	2020-07-15 20:28:06 UTC (rev 264418)
@@ -0,0 +1,11 @@
+This tests that the text insertion point is before the inserted text placeholder when it's inserted at the end of a word. This test can only be run in WebKitTestRunner or DumpRenderTree.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.getSelection().type is "Caret"
+PASS window.getSelection().baseOffset is 'Hello'.length
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html (0 => 264418)


--- trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html	                        (rev 0)
+++ trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html	2020-07-15 20:28:06 UTC (rev 264418)
@@ -0,0 +1,47 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<meta name="viewport" content="initial-scale=1.0">
+<script src=""
+<script src=""
+<link rel="stylesheet" href=""
+</head>
+<body>
+<p id="description"></p>
+<div id="test" class="test" style="width: 200px; height: 200px; border: 1px solid black" contenteditable="true">Hello from Cupertino!</div>
+<p id="console"></p>
+<script>
+window.jsTestIsAsync = true;
+
+description("This tests that the text insertion point is before the inserted text placeholder when it's inserted at the end of a word. This test can only be run in WebKitTestRunner or DumpRenderTree.");
+
+async function runTest()
+{
+    let testElement = document.getElementById("test");
+    if (window.testRunner)
+        await UIHelper.activateElementAndWaitForInputSession(testElement);
+    else
+        await UIHelper.callFunctionAndWaitForEvent(() => testElement.focus(), testElement, "focus");
+
+    let endOfHello = testElement.textContent.indexOf("Hello") + 5;
+    window.getSelection().setBaseAndExtent(testElement.firstChild, endOfHello, testElement.firstChild, endOfHello);
+
+    if (window.internals) {
+        let lineHeight = parseInt(window.getComputedStyle(testElement, null).lineHeight, 10);
+        internals.insertTextPlaceholder(0 /* width */, lineHeight);
+    }
+
+    shouldBeEqualToString("window.getSelection().type", "Caret");
+    shouldBe("window.getSelection().baseOffset", "'Hello'.length");
+
+    if (window.testRunner) {
+        // Cleanup to make the results pretty.
+        document.body.removeChild(testElement);
+    }
+
+    finishJSTest();
+}
+runTest();
+</script>
+</body>
+</html>

Added: trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word-expected.txt (0 => 264418)


--- trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word-expected.txt	2020-07-15 20:28:06 UTC (rev 264418)
@@ -0,0 +1,11 @@
+This tests that the text insertion point is before the inserted text placeholder when it's inserted before the start of a word. This test can only be run in WebKitTestRunner or DumpRenderTree.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.getSelection().type is "Caret"
+PASS window.getSelection().baseOffset is 'Hello'.length
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html (0 => 264418)


--- trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html	                        (rev 0)
+++ trunk/LayoutTests/editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html	2020-07-15 20:28:06 UTC (rev 264418)
@@ -0,0 +1,47 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<meta name="viewport" content="initial-scale=1.0">
+<script src=""
+<script src=""
+<link rel="stylesheet" href=""
+</head>
+<body>
+<p id="description"></p>
+<div id="test" class="test" style="width: 200px; height: 200px; border: 1px solid black" contenteditable="true">Hello from Cupertino!</div>
+<p id="console"></p>
+<script>
+window.jsTestIsAsync = true;
+
+description("This tests that the text insertion point is before the inserted text placeholder when it's inserted before the start of a word. This test can only be run in WebKitTestRunner or DumpRenderTree.");
+
+async function runTest()
+{
+    let testElement = document.getElementById("test");
+    if (window.testRunner)
+        await UIHelper.activateElementAndWaitForInputSession(testElement);
+    else
+        await UIHelper.callFunctionAndWaitForEvent(() => testElement.focus(), testElement, "focus");
+
+    let positionOfFrom = testElement.textContent.indexOf("from");
+    window.getSelection().setBaseAndExtent(testElement.firstChild, positionOfFrom, testElement.firstChild, positionOfFrom);
+
+    if (window.internals) {
+        let lineHeight = parseInt(window.getComputedStyle(testElement, null).lineHeight, 10);
+        internals.insertTextPlaceholder(0 /* width */, lineHeight);
+    }
+
+    shouldBeEqualToString("window.getSelection().type", "Caret");
+    shouldBe("window.getSelection().baseOffset", "'Hello'.length");
+
+    if (window.testRunner) {
+        // Cleanup to make the results pretty.
+        document.body.removeChild(testElement);
+    }
+
+    finishJSTest();
+}
+runTest();
+</script>
+</body>
+</html>

Added: trunk/LayoutTests/editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event-expected.txt (0 => 264418)


--- trunk/LayoutTests/editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event-expected.txt	2020-07-15 20:28:06 UTC (rev 264418)
@@ -0,0 +1,20 @@
+This tests that a text placeholder that is removed from the page via a mutation event callback after being inserted does not cause a crash. This test can only be run in WebKitTestRunner or DumpRenderTree.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Before inserting placeholder:
+PASS document.getElementById("test").childNodes.length is 1
+
+After inserting placeholder:
+PASS document.getElementById("test").childNodes.length is 3
+PASS placeholderElement.parentNode is non-null.
+
+After removing placeholder:
+PASS document.getElementById("test").childNodes.length became different from 3
+PASS document.getElementById("test").childNodes.length is <= 2
+PASS placeholderElement.parentNode became null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html (0 => 264418)


--- trunk/LayoutTests/editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html	                        (rev 0)
+++ trunk/LayoutTests/editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html	2020-07-15 20:28:06 UTC (rev 264418)
@@ -0,0 +1,77 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<meta name="viewport" content="initial-scale=1.0">
+<script src=""
+<script src=""
+<link rel="stylesheet" href=""
+</head>
+<body>
+<p id="description"></p>
+<div id="test" class="test" style="width: 200px; height: 200px; border: 1px solid black" contenteditable="true">Hello from Cupertino!</div>
+<p id="console"></p>
+<script>
+window.jsTestIsAsync = true;
+
+description("This tests that a text placeholder that is removed from the page via a mutation event callback after being inserted does not cause a crash. This test can only be run in WebKitTestRunner or DumpRenderTree.");
+
+let placeholderElement;
+
+function done()
+{
+    // Clean up to make the result pretty.
+    if (window.testRunner)
+        document.getElementById("test").remove();
+    finishJSTest();
+}
+
+function didRemovePlaceholder()
+{
+    debug("<br>After removing placeholder:");
+    // Number of child nodes should be 2 or fewer. It depends on how aggressively adjacent #text nodes are coalesced.
+    shouldBecomeDifferent('document.getElementById("test").childNodes.length', "3", () => {
+        shouldBeLessThanOrEqual('document.getElementById("test").childNodes.length', "2");
+        shouldBecomeEqual("placeholderElement.parentNode", "null", done);
+    });
+}
+
+function didInsertPlaceholder()
+{
+    debug("<br>After inserting placeholder:");
+    shouldBe('document.getElementById("test").childNodes.length', "3");
+
+    console.assert(document.getElementById("test").childNodes.length >= 2);
+    console.assert(document.getElementById("test").childNodes[1] instanceof Element);
+
+    document.addEventListener("DOMSubtreeModified", didRemovePlaceholder, { once: true });
+    placeholderElement = document.getElementById("test").childNodes[1];
+    shouldBeNonNull("placeholderElement.parentNode");
+    placeholderElement.remove();
+}
+
+async function runTest()
+{
+    let testElement = document.getElementById("test");
+    if (window.testRunner)
+        await UIHelper.activateElementAndWaitForInputSession(testElement);
+    else
+        await UIHelper.callFunctionAndWaitForEvent(() => testElement.focus(), testElement, "focus");
+
+    let positionOfFrom = testElement.textContent.indexOf("from");
+    window.getSelection().setBaseAndExtent(testElement.firstChild, positionOfFrom, testElement.firstChild, positionOfFrom);
+
+    debug("Before inserting placeholder:")
+    shouldBeEqualToNumber('document.getElementById("test").childNodes.length', 1);
+
+    // To allow for the control flow to be debugged on Mac, add the event listener here instead of right before insertTextPlaceholder.
+    document.addEventListener("DOMSubtreeModified", didInsertPlaceholder, { once: true });
+
+    if (window.internals) {
+        let lineHeight = parseInt(window.getComputedStyle(testElement, null).lineHeight, 10);
+        internals.insertTextPlaceholder(0 /* width */, lineHeight);
+    }
+}
+runTest();
+</script>
+</body>
+</html>

Modified: trunk/LayoutTests/resources/js-test.js (264417 => 264418)


--- trunk/LayoutTests/resources/js-test.js	2020-07-15 20:18:13 UTC (rev 264417)
+++ trunk/LayoutTests/resources/js-test.js	2020-07-15 20:28:06 UTC (rev 264418)
@@ -620,6 +620,27 @@
         testPassed(_a + " is >= " + _b);
 }
 
+function shouldBeLessThanOrEqual(_a, _b) {
+    if (typeof _a != "string" || typeof _b != "string")
+        debug("WARN: shouldBeLessThanOrEqual expects string arguments");
+
+    var _exception;
+    var _av;
+    try {
+        _av = eval(_a);
+    } catch (e) {
+        _exception = e;
+    }
+    var _bv = eval(_b);
+
+    if (_exception)
+        testFailed(_a + " should be <= " + _b + ". Threw exception " + _exception);
+    else if (typeof _av == "undefined" || _av > _bv)
+        testFailed(_a + " should be <= " + _b + ". Was " + _av + " (of type " + typeof _av + ").");
+    else
+        testPassed(_a + " is <= " + _b);
+}
+
 function expectTrue(v, msg) {
   if (v) {
     testPassed(msg);

Modified: trunk/Source/WebCore/ChangeLog (264417 => 264418)


--- trunk/Source/WebCore/ChangeLog	2020-07-15 20:18:13 UTC (rev 264417)
+++ trunk/Source/WebCore/ChangeLog	2020-07-15 20:28:06 UTC (rev 264418)
@@ -1,3 +1,23 @@
+2020-07-15  Daniel Bates  <[email protected]>
+
+        [iOS] Caret should be before text placeholder instead of after it
+        https://bugs.webkit.org/show_bug.cgi?id=214319
+        <rdar://problem/65295523>
+
+        Reviewed by Wenson Hsieh.
+
+        Move the caret to before the text placeholder to match UIKit.
+
+        Tests: editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-end-of-word.html
+               editing/text-placeholder/caret-before-zero-width-placeholder-in-content-editable-start-of-word.html
+               editing/text-placeholder/insert-into-content-editable-and-remove-via-mutation-event.html
+
+        * editing/Editor.cpp:
+        (WebCore::Editor::insertTextPlaceholder): Compute the position before the placeholder in its parent.
+        This computation can only be done if the placeholder still has a parent. It may not after insertion
+        because arbitrary _javascript_ code can run. If this happens then bail out and return nullptr. Otherwise,
+        set the new selection.
+
 2020-07-15  Simon Fraser  <[email protected]>
 
         itsnicethat.com page is sometimes non-scrollable

Modified: trunk/Source/WebCore/editing/Editor.cpp (264417 => 264418)


--- trunk/Source/WebCore/editing/Editor.cpp	2020-07-15 20:18:13 UTC (rev 264417)
+++ trunk/Source/WebCore/editing/Editor.cpp	2020-07-15 20:28:06 UTC (rev 264418)
@@ -3323,9 +3323,12 @@
     auto placeholder = TextPlaceholderElement::create(document, size);
     createLiveRange(*range)->insertNode(placeholder.copyRef());
 
-    VisibleSelection newSelection { positionBeforeNode(placeholder.ptr()), positionAfterNode(placeholder.ptr()) };
-    m_document.selection().setSelection(newSelection, FrameSelection::defaultSetSelectionOptions(UserTriggered));
+    // Inserting the placeholder can run arbitrary _javascript_. Check that it still has a parent.
+    if (!placeholder->parentNode())
+        return nullptr;
 
+    m_document.selection().setSelection(VisibleSelection { positionInParentBeforeNode(placeholder.ptr()), SEL_DEFAULT_AFFINITY }, FrameSelection::defaultSetSelectionOptions(UserTriggered));
+
     return placeholder;
 }
 

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to