- Revision
- 239573
- Author
- wenson_hs...@apple.com
- Date
- 2019-01-02 12:28:24 -0800 (Wed, 02 Jan 2019)
Log Message
REGRESSION (r239441): [iOS] Selection UI sometimes doesn't change after tapping "select all" in the callout bar
https://bugs.webkit.org/show_bug.cgi?id=193070
<rdar://problem/46921508>
Reviewed by Tim Horton.
Source/WebKit:
r239441 added logic to include an EditorState in the next layer tree commit when refocusing an element; this was
done to ensure that after tapping an element that has already been programmatically focused, we still send up-
to-date editor information to the UI process for the purposes of zooming to the selection rect, even if the
selection in the DOM is unchanged, since other aspects of the editor state may have changed since the element
was initially focused.
We currently try to flag the next layer tree commit by setting `m_hasPendingEditorStateUpdate` to `true`.
However, this is problematic since we aren't guaranteed in all cases that a compositing flush has been
scheduled. In the case where it hasn't, we'll end up in a state where the editor state update flag has been set,
yet the update will not make it over to the UI process until something happens that forces a layer tree commit
(e.g. scrolling, pinch zooming). Worse still, if the selection is then programmatically changed from the web
process, we will bail from sending a subsequent editor state update to the UI process because `WebPage` thinks
that a pending editor state has already been scheduled. This manifests in selection UI not updating after
tapping "Select All" in the callout bar, if the callout bar was brought up by tapping near the selection (since
this refocuses the element).
To fix this, we adjust this logic in `WebPage::elementDidRefocus` so that it sets the flag and then schedules a
compositing flush, but only if the user is actually interacting with the focused element (i.e., if the page
calls `focus` repeatedly, we won't continue to schedule compositing flushes).
Test: editing/selection/ios/change-selection-after-tapping-focused-element.html
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::elementDidRefocus):
(WebKit::WebPage::sendEditorStateUpdate):
(WebKit::WebPage::scheduleFullEditorStateUpdate):
Add a private helper method to schedule an editor state update by setting `m_hasPendingEditorStateUpdate` to
`true` and then scheduling a compositing layer flush. Also, add a FIXME aluding to the fact that scheduling an
entire compositing layer flush to send an editor state update is somewhat wasteful, and should be replaced by
just scheduling this work to be done before the next frame (see: <rdar://problem/36523583> for more detail).
We also use this helper method in a few places where we currently turn on the editor state flag and schedule a
subsequent compositing flush.
(WebKit::WebPage::sendPartialEditorStateAndSchedulePostLayoutUpdate):
* WebProcess/WebPage/WebPage.h:
LayoutTests:
Add a test to ensure that selection UI is shown after tapping on a focused element and then changing the
selection programmatically.
* editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt: Added.
* editing/selection/ios/change-selection-after-tapping-focused-element.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (239572 => 239573)
--- trunk/LayoutTests/ChangeLog 2019-01-02 19:40:05 UTC (rev 239572)
+++ trunk/LayoutTests/ChangeLog 2019-01-02 20:28:24 UTC (rev 239573)
@@ -1,3 +1,17 @@
+2019-01-02 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r239441): [iOS] Selection UI sometimes doesn't change after tapping "select all" in the callout bar
+ https://bugs.webkit.org/show_bug.cgi?id=193070
+ <rdar://problem/46921508>
+
+ Reviewed by Tim Horton.
+
+ Add a test to ensure that selection UI is shown after tapping on a focused element and then changing the
+ selection programmatically.
+
+ * editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt: Added.
+ * editing/selection/ios/change-selection-after-tapping-focused-element.html: Added.
+
2019-01-02 Simon Fraser <simon.fra...@apple.com>
Handle calc() expressions in gradient color stops
Added: trunk/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt (0 => 239573)
--- trunk/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt 2019-01-02 20:28:24 UTC (rev 239573)
@@ -0,0 +1,14 @@
+WebKit
+Verifies that after tapping a focused element to bring up the keyboard, changing the selection via script causes the native selection UI to be shown. To manually test, tap the red box above and tap select all via the callout bar; the entire text in the editable element should be selected.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+PASS selectionRects.length is 1
+PASS selectionRects[0].left is 2
+PASS selectionRects[0].top is 2
+PASS selectionRects[0].width is 309
+PASS selectionRects[0].height is 114
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element.html (0 => 239573)
--- trunk/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element.html (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element.html 2019-01-02 20:28:24 UTC (rev 239573)
@@ -0,0 +1,73 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+<head>
+ <script src=""
+ <script src=""
+ <style>
+ body {
+ margin: 0;
+ }
+
+ #editor {
+ width: 100%;
+ height: 280px;
+ border: 2px solid tomato;
+ outline: none;
+ box-sizing: border-box;
+ font-size: 100px;
+ margin: 0;
+ }
+ </style>
+ <script>
+ jsTestIsAsync = true;
+
+ function doAfterDetectingVisibleSelectionViewRects(callback)
+ {
+ const uiScript = `
+ function waitForSelectionRects() {
+ uiController.doAsyncTask(() => {
+ const rects = uiController.selectionRangeViewRects;
+ if (rects && rects.length)
+ uiController.uiScriptComplete(JSON.stringify(rects));
+ else
+ waitForSelectionRects();
+ });
+ }
+ uiController.doAsyncTask(waitForSelectionRects);`;
+ testRunner.runUIScript(uiScript, result => callback(JSON.parse(result)));
+ }
+
+ async function run()
+ {
+ description("Verifies that after tapping a focused element to bring up the keyboard, changing the selection via "
+ + "script causes the native selection UI to be shown. To manually test, tap the red box above and tap select "
+ + "all via the callout bar; the entire text in the editable element should be selected.");
+
+ if (!window.testRunner)
+ return;
+
+ await UIHelper.activateAndWaitForInputSessionAt(130, 100);
+ await new Promise(resolve => setTimeout(resolve, 500));
+
+ doAfterDetectingVisibleSelectionViewRects(selectionRects => {
+ window.selectionRects = selectionRects;
+ shouldBe("selectionRects.length", "1");
+ shouldBe("selectionRects[0].left", "2");
+ shouldBe("selectionRects[0].top", "2");
+ shouldBe("selectionRects[0].width", "309");
+ shouldBe("selectionRects[0].height", "114");
+ finishJSTest();
+ });
+
+ await UIHelper.tapAt(130, 100);
+ getSelection().selectAllChildren(editor);
+ }
+ </script>
+</head>
+<body _onload_=run()>
+ <div id="editor" contenteditable>WebKit</div>
+ <div id="description"></div>
+ <div id="console"></div>
+</body>
+</html>
Modified: trunk/Source/WebKit/ChangeLog (239572 => 239573)
--- trunk/Source/WebKit/ChangeLog 2019-01-02 19:40:05 UTC (rev 239572)
+++ trunk/Source/WebKit/ChangeLog 2019-01-02 20:28:24 UTC (rev 239573)
@@ -1,3 +1,49 @@
+2019-01-02 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r239441): [iOS] Selection UI sometimes doesn't change after tapping "select all" in the callout bar
+ https://bugs.webkit.org/show_bug.cgi?id=193070
+ <rdar://problem/46921508>
+
+ Reviewed by Tim Horton.
+
+ r239441 added logic to include an EditorState in the next layer tree commit when refocusing an element; this was
+ done to ensure that after tapping an element that has already been programmatically focused, we still send up-
+ to-date editor information to the UI process for the purposes of zooming to the selection rect, even if the
+ selection in the DOM is unchanged, since other aspects of the editor state may have changed since the element
+ was initially focused.
+
+ We currently try to flag the next layer tree commit by setting `m_hasPendingEditorStateUpdate` to `true`.
+ However, this is problematic since we aren't guaranteed in all cases that a compositing flush has been
+ scheduled. In the case where it hasn't, we'll end up in a state where the editor state update flag has been set,
+ yet the update will not make it over to the UI process until something happens that forces a layer tree commit
+ (e.g. scrolling, pinch zooming). Worse still, if the selection is then programmatically changed from the web
+ process, we will bail from sending a subsequent editor state update to the UI process because `WebPage` thinks
+ that a pending editor state has already been scheduled. This manifests in selection UI not updating after
+ tapping "Select All" in the callout bar, if the callout bar was brought up by tapping near the selection (since
+ this refocuses the element).
+
+ To fix this, we adjust this logic in `WebPage::elementDidRefocus` so that it sets the flag and then schedules a
+ compositing flush, but only if the user is actually interacting with the focused element (i.e., if the page
+ calls `focus` repeatedly, we won't continue to schedule compositing flushes).
+
+ Test: editing/selection/ios/change-selection-after-tapping-focused-element.html
+
+ * WebProcess/WebPage/WebPage.cpp:
+ (WebKit::WebPage::elementDidRefocus):
+ (WebKit::WebPage::sendEditorStateUpdate):
+ (WebKit::WebPage::scheduleFullEditorStateUpdate):
+
+ Add a private helper method to schedule an editor state update by setting `m_hasPendingEditorStateUpdate` to
+ `true` and then scheduling a compositing layer flush. Also, add a FIXME aluding to the fact that scheduling an
+ entire compositing layer flush to send an editor state update is somewhat wasteful, and should be replaced by
+ just scheduling this work to be done before the next frame (see: <rdar://problem/36523583> for more detail).
+
+ We also use this helper method in a few places where we currently turn on the editor state flag and schedule a
+ subsequent compositing flush.
+
+ (WebKit::WebPage::sendPartialEditorStateAndSchedulePostLayoutUpdate):
+ * WebProcess/WebPage/WebPage.h:
+
2019-01-02 Commit Queue <commit-qu...@webkit.org>
Unreviewed, rolling out r239524.
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp (239572 => 239573)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2019-01-02 19:40:05 UTC (rev 239572)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2019-01-02 20:28:24 UTC (rev 239573)
@@ -5281,7 +5281,9 @@
void WebPage::elementDidRefocus(WebCore::Element& element)
{
elementDidFocus(element);
- m_hasPendingEditorStateUpdate = true;
+
+ if (m_isFocusingElementDueToUserInteraction)
+ scheduleFullEditorStateUpdate();
}
void WebPage::elementDidFocus(WebCore::Element& element)
@@ -5874,12 +5876,21 @@
auto state = editorState();
send(Messages::WebPageProxy::EditorStateChanged(state), pageID());
- if (state.isMissingPostLayoutData) {
- m_hasPendingEditorStateUpdate = true;
- m_drawingArea->scheduleCompositingLayerFlush();
- }
+ if (state.isMissingPostLayoutData)
+ scheduleFullEditorStateUpdate();
}
+void WebPage::scheduleFullEditorStateUpdate()
+{
+ if (m_hasPendingEditorStateUpdate)
+ return;
+
+ m_hasPendingEditorStateUpdate = true;
+ // FIXME: Scheduling a compositing layer flush here can be more expensive than necessary.
+ // Instead, we should just compute and send post-layout editor state during the next frame.
+ m_drawingArea->scheduleCompositingLayerFlush();
+}
+
#if PLATFORM(COCOA)
void WebPage::sendTouchBarMenuDataRemovedUpdate(HTMLMenuElement& element)
{
@@ -5909,13 +5920,7 @@
return;
send(Messages::WebPageProxy::EditorStateChanged(editorState(IncludePostLayoutDataHint::No)), pageID());
-
- if (m_hasPendingEditorStateUpdate)
- return;
-
- // Flag the next layer flush to compute and propagate an EditorState to the UI process.
- m_hasPendingEditorStateUpdate = true;
- m_drawingArea->scheduleCompositingLayerFlush();
+ scheduleFullEditorStateUpdate();
}
void WebPage::flushPendingEditorStateUpdate()
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.h (239572 => 239573)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2019-01-02 19:40:05 UTC (rev 239572)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2019-01-02 20:28:24 UTC (rev 239573)
@@ -1147,6 +1147,7 @@
void platformDetach();
void platformEditorState(WebCore::Frame&, EditorState& result, IncludePostLayoutDataHint) const;
void sendEditorStateUpdate();
+ void scheduleFullEditorStateUpdate();
#if PLATFORM(COCOA)
void sendTouchBarMenuDataAddedUpdate(WebCore::HTMLMenuElement&);
