- Revision
- 248974
- Author
- timothy_hor...@apple.com
- Date
- 2019-08-21 16:05:26 -0700 (Wed, 21 Aug 2019)
Log Message
[Mail] Tapping top of message scrolls back to copied text instead of top of the message
https://bugs.webkit.org/show_bug.cgi?id=200999
<rdar://problem/54564878>
Reviewed by Wenson Hsieh.
Source/WebCore:
Test: editing/selection/ios/change-selection-by-tapping-with-existing-selection.html
* page/EditorClient.h:
(WebCore::EditorClient::shouldAllowSingleClickToChangeSelection const):
* page/EventHandler.cpp:
(WebCore::EventHandler::handleMousePressEventSingleClick):
Instead of encoding platform behaviors in EventHandler, defer to EditorClient.
Source/WebKit:
In the case where you have a WebCore selection but are not first responder,
when you tap the WKWebView to become first responder, EventHandler would
bail from setting the selection, assuming UIKit was going to do it. This
behavior was introduced in r233311.
However, since we are not first responder, UIKit does not change the
selection, since it considers the view to not be editable.
Fix this by letting WebCore set the selection in this case, as it used to.
* WebProcess/WebCoreSupport/WebEditorClient.h:
* WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:
(WebKit::WebEditorClient::shouldAllowSingleClickToChangeSelection const):
* WebProcess/WebPage/WebPage.h:
(WebKit::WebPage::isShowingInputViewForFocusedElement const):
Copy the logic from EventHandler, with the added caveat (which fixes the
aforementioned behavior) that we will allow EventHandler to change the
selection if we don't have a focused node in the UIKit sense, because
we know that the platform text interaction code will *not* change the
selection if that is the case, so it's up to us.
Source/WebKitLegacy/mac:
* WebCoreSupport/WebEditorClient.h:
* WebCoreSupport/WebEditorClient.mm:
(WebEditorClient::shouldAllowSingleClickToChangeSelection const):
Copy the existing behavior from EventHandler.
We do not fix the bug in WebKitLegacy for a multitude of reasons, primarily
because we do not know of any user impact.
LayoutTests:
* editing/selection/ios/change-selection-by-tapping-with-existing-selection-expected.txt: Added.
* editing/selection/ios/change-selection-by-tapping-with-existing-selection.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (248973 => 248974)
--- trunk/LayoutTests/ChangeLog 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/LayoutTests/ChangeLog 2019-08-21 23:05:26 UTC (rev 248974)
@@ -1,3 +1,14 @@
+2019-08-21 Tim Horton <timothy_hor...@apple.com>
+
+ [Mail] Tapping top of message scrolls back to copied text instead of top of the message
+ https://bugs.webkit.org/show_bug.cgi?id=200999
+ <rdar://problem/54564878>
+
+ Reviewed by Wenson Hsieh.
+
+ * editing/selection/ios/change-selection-by-tapping-with-existing-selection-expected.txt: Added.
+ * editing/selection/ios/change-selection-by-tapping-with-existing-selection.html: Added.
+
2019-08-21 Alex Christensen <achristen...@webkit.org>
Disabling text autosizing should prevent text autosizing
Added: trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-with-existing-selection-expected.txt (0 => 248974)
--- trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-with-existing-selection-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-with-existing-selection-expected.txt 2019-08-21 23:05:26 UTC (rev 248974)
@@ -0,0 +1,13 @@
+Here's to the crazy ones, the misfits, the rebels, the trouble makers, the round pegs in the square holes, the ones who see things differently. There not fond of rules, and they have no respect for the status quo, you can quote then, disagree with them, glorify or vilify them, about the only thing you can't do is ignore them. Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world are the ones who do.
+
+Verifies that tapping to change selection works when we already have a selection in the same editable root but do not currently have a focused node in the UIKit sense.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+Selection before tap: (target#0, target#2)
+Selection after tap: (editor#0, editor#0)
Added: trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-with-existing-selection.html (0 => 248974)
--- trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-with-existing-selection.html (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-with-existing-selection.html 2019-08-21 23:05:26 UTC (rev 248974)
@@ -0,0 +1,69 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<script src=""
+<script src=""
+<meta name=viewport content="width=device-width, initial-scale=1, user-scalable=no">
+<style>
+body, html {
+ width: 100%;
+ height: 100%;
+ margin: 0;
+}
+
+#editor {
+ width: 300px;
+ height: 320px;
+ font-size: 18px;
+}
+</style>
+<script>
+jsTestIsAsync = true;
+
+function selectionToString() {
+ const selection = getSelection();
+ if (!selection.rangeCount)
+ return "(no selection)";
+
+ const range = selection.getRangeAt(0);
+ return `(${range.startContainer.parentElement.id}#${range.startOffset}, ${range.endContainer.parentElement.id}#${range.endOffset})`;
+}
+
+function tapAndWaitForSelectionChange(x, y) {
+ return new Promise(resolve => {
+ const editor = document.getElementById("editor");
+ let doneCount = 0;
+ const checkDone = () => {
+ if (++doneCount != 2)
+ return;
+
+ document.removeEventListener("selectionchange", checkDone);
+ resolve();
+ }
+ document.addEventListener("selectionchange", checkDone);
+ UIHelper.activateAt(x, y).then(checkDone);
+ });
+}
+
+addEventListener("load", async () => {
+ description("Verifies that tapping to change selection works when we already have a selection in the same editable root but do not currently have a focused node in the UIKit sense.");
+
+ var target = document.getElementById("target");
+ window.getSelection().setBaseAndExtent(target, 0, target, 2);
+
+ document.querySelector("#selection-before").textContent = selectionToString();
+ await tapAndWaitForSelectionChange(5, 5);
+ document.querySelector("#selection-after").textContent = selectionToString();
+
+ finishJSTest();
+});
+</script>
+</head>
+<body>
+<p contenteditable id="editor">Here's to the crazy ones, the misfits, the rebels, the trouble makers, the round pegs in the square holes, the ones who see things differently. There not fond of rules, and they have no respect for the status quo, you can quote then, disagree with them, glorify or vilify them, about the only thing you can't do is ignore them. Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world are the ones who <span id='target'>do</span>.</p>
+ <p id="description"></p>
+ <p id="console"></p>
+ <div>Selection before tap: <span id="selection-before"></span></div>
+ <div>Selection after tap: <span id="selection-after"></span></div>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (248973 => 248974)
--- trunk/Source/WebCore/ChangeLog 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebCore/ChangeLog 2019-08-21 23:05:26 UTC (rev 248974)
@@ -1,3 +1,19 @@
+2019-08-21 Tim Horton <timothy_hor...@apple.com>
+
+ [Mail] Tapping top of message scrolls back to copied text instead of top of the message
+ https://bugs.webkit.org/show_bug.cgi?id=200999
+ <rdar://problem/54564878>
+
+ Reviewed by Wenson Hsieh.
+
+ Test: editing/selection/ios/change-selection-by-tapping-with-existing-selection.html
+
+ * page/EditorClient.h:
+ (WebCore::EditorClient::shouldAllowSingleClickToChangeSelection const):
+ * page/EventHandler.cpp:
+ (WebCore::EventHandler::handleMousePressEventSingleClick):
+ Instead of encoding platform behaviors in EventHandler, defer to EditorClient.
+
2019-08-21 Chris Dumez <cdu...@apple.com>
Crash under StringImpl::endsWith() in SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade()
Modified: trunk/Source/WebCore/page/EditorClient.h (248973 => 248974)
--- trunk/Source/WebCore/page/EditorClient.h 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebCore/page/EditorClient.h 2019-08-21 23:05:26 UTC (rev 248974)
@@ -186,6 +186,8 @@
virtual bool performTwoStepDrop(DocumentFragment&, Range& destination, bool isMove) = 0;
virtual bool canShowFontPanel() const = 0;
+
+ virtual bool shouldAllowSingleClickToChangeSelection(Node&, const VisibleSelection&) const { return true; }
};
}
Modified: trunk/Source/WebCore/page/EventHandler.cpp (248973 => 248974)
--- trunk/Source/WebCore/page/EventHandler.cpp 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebCore/page/EventHandler.cpp 2019-08-21 23:05:26 UTC (rev 248974)
@@ -698,12 +698,8 @@
VisibleSelection newSelection = m_frame.selection().selection();
TextGranularity granularity = CharacterGranularity;
-#if PLATFORM(IOS_FAMILY)
- // The text selection assistant will handle selection in the case where we are already editing the node
- auto* editableRoot = newSelection.rootEditableElement();
- if (editableRoot && editableRoot == targetNode->rootEditableElement())
+ if (!m_frame.editor().client()->shouldAllowSingleClickToChangeSelection(*targetNode, newSelection))
return true;
-#endif
if (extendSelection && newSelection.isCaretOrRange()) {
VisibleSelection selectionInUserSelectAll = expandSelectionToRespectSelectOnMouseDown(*targetNode, VisibleSelection(pos));
Modified: trunk/Source/WebKit/ChangeLog (248973 => 248974)
--- trunk/Source/WebKit/ChangeLog 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKit/ChangeLog 2019-08-21 23:05:26 UTC (rev 248974)
@@ -1,3 +1,32 @@
+2019-08-21 Tim Horton <timothy_hor...@apple.com>
+
+ [Mail] Tapping top of message scrolls back to copied text instead of top of the message
+ https://bugs.webkit.org/show_bug.cgi?id=200999
+ <rdar://problem/54564878>
+
+ Reviewed by Wenson Hsieh.
+
+ In the case where you have a WebCore selection but are not first responder,
+ when you tap the WKWebView to become first responder, EventHandler would
+ bail from setting the selection, assuming UIKit was going to do it. This
+ behavior was introduced in r233311.
+
+ However, since we are not first responder, UIKit does not change the
+ selection, since it considers the view to not be editable.
+
+ Fix this by letting WebCore set the selection in this case, as it used to.
+
+ * WebProcess/WebCoreSupport/WebEditorClient.h:
+ * WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:
+ (WebKit::WebEditorClient::shouldAllowSingleClickToChangeSelection const):
+ * WebProcess/WebPage/WebPage.h:
+ (WebKit::WebPage::isShowingInputViewForFocusedElement const):
+ Copy the logic from EventHandler, with the added caveat (which fixes the
+ aforementioned behavior) that we will allow EventHandler to change the
+ selection if we don't have a focused node in the UIKit sense, because
+ we know that the platform text interaction code will *not* change the
+ selection if that is the case, so it's up to us.
+
2019-08-21 Chris Dumez <cdu...@apple.com>
Crash under NetworkCache::Data::mapToFile()
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h (248973 => 248974)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h 2019-08-21 23:05:26 UTC (rev 248974)
@@ -181,6 +181,7 @@
RefPtr<WebCore::DocumentFragment> documentFragmentFromDelegate(int index) final;
bool performsTwoStepPaste(WebCore::DocumentFragment*) final;
void updateStringForFind(const String&) final;
+ bool shouldAllowSingleClickToChangeSelection(WebCore::Node&, const WebCore::VisibleSelection&) const final;
#endif
bool performTwoStepDrop(WebCore::DocumentFragment&, WebCore::Range&, bool isMove) final;
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm (248973 => 248974)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm 2019-08-21 23:05:26 UTC (rev 248974)
@@ -102,6 +102,13 @@
m_page->didChangeOverflowScrollPosition();
}
+bool WebEditorClient::shouldAllowSingleClickToChangeSelection(WebCore::Node& targetNode, const WebCore::VisibleSelection& newSelection) const
+{
+ // The text selection assistant will handle selection in the case where we are already editing the node
+ auto* editableRoot = newSelection.rootEditableElement();
+ return !editableRoot || editableRoot != targetNode.rootEditableElement() || !m_page->isShowingInputViewForFocusedElement();
+}
+
} // namespace WebKit
#endif // PLATFORM(IOS_FAMILY)
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.h (248973 => 248974)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2019-08-21 23:05:26 UTC (rev 248974)
@@ -678,6 +678,7 @@
void setFocusedElementValueAsNumber(double);
void setFocusedElementSelectedIndex(uint32_t index, bool allowMultipleSelection);
void setIsShowingInputViewForFocusedElement(bool);
+ bool isShowingInputViewForFocusedElement() const { return m_isShowingInputViewForFocusedElement; }
void updateSelectionAppearance();
void getSelectionContext(CallbackID);
void handleTwoFingerTapAtPoint(const WebCore::IntPoint&, OptionSet<WebKit::WebEvent::Modifier>, uint64_t requestID);
Modified: trunk/Source/WebKitLegacy/mac/ChangeLog (248973 => 248974)
--- trunk/Source/WebKitLegacy/mac/ChangeLog 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKitLegacy/mac/ChangeLog 2019-08-21 23:05:26 UTC (rev 248974)
@@ -1,3 +1,18 @@
+2019-08-21 Tim Horton <timothy_hor...@apple.com>
+
+ [Mail] Tapping top of message scrolls back to copied text instead of top of the message
+ https://bugs.webkit.org/show_bug.cgi?id=200999
+ <rdar://problem/54564878>
+
+ Reviewed by Wenson Hsieh.
+
+ * WebCoreSupport/WebEditorClient.h:
+ * WebCoreSupport/WebEditorClient.mm:
+ (WebEditorClient::shouldAllowSingleClickToChangeSelection const):
+ Copy the existing behavior from EventHandler.
+ We do not fix the bug in WebKitLegacy for a multitude of reasons, primarily
+ because we do not know of any user impact.
+
2019-08-21 Ryosuke Niwa <rn...@webkit.org>
Put keygen element behind a runtime flag and disable it by default
Modified: trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h (248973 => 248974)
--- trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h 2019-08-21 23:05:26 UTC (rev 248974)
@@ -176,6 +176,10 @@
void registerUndoOrRedoStep(WebCore::UndoStep&, bool isRedo);
+#if PLATFORM(IOS_FAMILY)
+ bool shouldAllowSingleClickToChangeSelection(WebCore::Node& targetNode, const WebCore::VisibleSelection& newSelection) const;
+#endif
+
bool canShowFontPanel() const final
{
#if PLATFORM(MAC)
Modified: trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm (248973 => 248974)
--- trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm 2019-08-21 23:01:41 UTC (rev 248973)
+++ trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm 2019-08-21 23:05:26 UTC (rev 248974)
@@ -1258,3 +1258,12 @@
}];
#endif
}
+
+#if PLATFORM(IOS_FAMILY)
+bool WebEditorClient::shouldAllowSingleClickToChangeSelection(WebCore::Node& targetNode, const WebCore::VisibleSelection& newSelection) const
+{
+ // The text selection assistant will handle selection in the case where we are already editing the node
+ auto* editableRoot = newSelection.rootEditableElement();
+ return !editableRoot || editableRoot != targetNode.rootEditableElement();
+}
+#endif
