Attachment 343810 Details for Bug 187142 – Patch

[patch] Patch

bug-187142-20180628092409.patch (text/plain), 6.12 KB, created by Aditya Keerthi on 2018-06-28 09:24:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Aditya Keerthi

Created: 2018-06-28 09:24:10 PDT

Size: 6.12 KB

patch

obsolete

>Subversion Revision: 232962
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d7c6ec562f3df57ecc72c1f3c91c98f600c7c174..90ba1724a7aef785fc7e3877c32978086b317015 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2018-06-27  Aditya Keerthi  <akeerthi@apple.com>
>+
>+        REGRESSION (r232040): Cursor jumping in Safari text fields
>+        https://bugs.webkit.org/show_bug.cgi?id=187142
>+        <rdar://problem/41397577>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        r232040 enabled click events to fire on nodes that are already being edited in
>+        iOS. This resulted FrameSelection::setSelection being called twice. One call
>+        originated from the UIWKTextInteractionAssistant, which snaps the caret to word
>+        boundaries. The other call originates from handleMousePressEvent in EventHandler,
>+        and uses character boundaries. Consequently, we see the caret jumping around.
>+
>+        To fix this issue, an early return was added in the handleMousePressEvent
>+        codepath, which prevents FrameSelection::setSelection from being called when
>+        clicking on a node that is already being edited. This ensures that the
>+        UIWKTextInteractionAssistant codepath is the only influence on the caret position.
>+
>+        Test: fast/events/ios/click-selectionchange-once.html
>+
>+        * page/EventHandler.cpp:
>+        (WebCore::EventHandler::handleMousePressEventSingleClick):
>+
> 2018-06-19  Aditya Keerthi  <akeerthi@apple.com>
> 
>         [Datalist][macOS] Add suggestions UI for TextFieldInputTypes
>diff --git a/Source/WebCore/page/EventHandler.cpp b/Source/WebCore/page/EventHandler.cpp
>index a5bbf90434a5dd939adddd402d48e0357d0512bf..d488685030bca7e8f815a1120d1cfafb786710c1 100644
>--- a/Source/WebCore/page/EventHandler.cpp
>+++ b/Source/WebCore/page/EventHandler.cpp
>@@ -681,6 +681,12 @@ bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestR
>     VisibleSelection newSelection = m_frame.selection().selection();
>     TextGranularity granularity = CharacterGranularity;
> 
>+#if PLATFORM(IOS)
>+    // The text selection assistant will handle selection in the case where we are already editing the node
>+    if (newSelection.rootEditableElement() == targetNode->rootEditableElement())
>+        return true;
>+#endif
>+
>     if (extendSelection && newSelection.isCaretOrRange()) {
>         VisibleSelection selectionInUserSelectAll = expandSelectionToRespectSelectOnMouseDown(*targetNode, VisibleSelection(pos));
>         if (selectionInUserSelectAll.isRange()) {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 64f793728864eacf6d8104ed7a4af1ced20d7323..2a9f53def538acd30b1ba58fa69c2d3c643bcfe3 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-27  Aditya Keerthi  <akeerthi@apple.com>
>+
>+        REGRESSION (r232040): Cursor jumping in Safari text fields
>+        https://bugs.webkit.org/show_bug.cgi?id=187142
>+        <rdar://problem/41397577>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added test to ensure that the 'selectionchange' event is only fired once per
>+        click in an editable node.
>+
>+        * fast/events/ios/click-selectionchange-once-expected.txt: Added.
>+        * fast/events/ios/click-selectionchange-once.html: Added.
>+
> 2018-06-19  Aditya Keerthi  <akeerthi@apple.com>
> 
>         [Datalist][macOS] Add suggestions UI for TextFieldInputTypes
>diff --git a/LayoutTests/fast/events/ios/click-selectionchange-once-expected.txt b/LayoutTests/fast/events/ios/click-selectionchange-once-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..4f51a8894dc849038061faab69e034687591cf17
>--- /dev/null
>+++ b/LayoutTests/fast/events/ios/click-selectionchange-once-expected.txt
>@@ -0,0 +1,12 @@
>+PASS document.getElementById('selectionChange').textContent is "1"
>+PASS document.getElementById('selectionStart').textContent is "45"
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+The selectionchange event should only be fired once when clicking a node that is being edited.
>+
>+The selectionchange count in the editable node is: 1
>+
>+The caret position in the editable node is: 45
>+
>+
>diff --git a/LayoutTests/fast/events/ios/click-selectionchange-once.html b/LayoutTests/fast/events/ios/click-selectionchange-once.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..86210abea0a74fbd33ee1efe3659537b431bff03
>--- /dev/null
>+++ b/LayoutTests/fast/events/ios/click-selectionchange-once.html
>@@ -0,0 +1,44 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <meta name="viewport" content="width=device-width, initial-scale=1">
>+    <script src="../../../resources/js-test-pre.js"></script>
>+    <script src="../../../resources/ui-helper.js"></script>
>+</head>
>+<body>
>+    <div id="description">
>+        <p>The selectionchange event should only be fired once when clicking a node that is being edited.</p>
>+        <p>The selectionchange count in the editable node is: <span id="selectionChange">0</span></p>
>+        <p>The caret position in the editable node is: <span id="selectionStart">0</span></p>
>+    </div>
>+    <input id="input" style:"width: 100%"/>
>+</body>
>+<script>
>+
>+if (window.testRunner) {
>+    testRunner.dumpAsText();
>+    jsTestIsAsync = true;
>+
>+    const x = input.offsetLeft;
>+    const y = input.offsetTop + input.offsetHeight / 2;
>+    const string = "Thisisareallylongstringthatfillstheinputfield";
>+    input.value = string;
>+
>+    UIHelper.activateAndWaitForInputSessionAt(x, y).then(() => {
>+        selectionChangeCount = 0;
>+        document.addEventListener("selectionchange", function(e) {
>+            selectionChangeCount += 1;
>+            selectionChange.textContent = selectionChangeCount;
>+            selectionStart.textContent = input.selectionStart;
>+        });
>+
>+        UIHelper.activateElement(input).then(() => {
>+            shouldBeEqualToString("document.getElementById('selectionChange').textContent", `${selectionChangeCount}`);
>+            shouldBeEqualToString("document.getElementById('selectionStart').textContent", `${string.length}`);
>+            finishJSTest();
>+        });
>+    });
>+}
>+</script>
>+<script src="../../../resources/js-test-post.js"></script>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 187142: 343810