Changeset 244897 in webkit

Timestamp:

May 2, 2019 6:05:32 PM (5 years ago)

Author:

Wenson Hsieh

Message:

REGRESSION: Layout test editing/selection/ios/selection-after-changing-text-with-callout-menu.html is failing
​https://bugs.webkit.org/show_bug.cgi?id=197532
<rdar://problem/50177144>

Reviewed by Ryosuke Niwa.

Fixes layout tests that began failing after r244546. See below for details.

• UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:

(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):

Partially reverts a change in r244546, after which we commit the layer tree and scroll before updating the
page's editor state. The purpose of this change was to ensure that UI process-side element focus scrolling logic
would not conflict with web-process-driven scrolling logic.

Instead, we split the existing logic in WebPageProxy::editorStateChanged into two pieces: one that updates the
editor state (by setting m_editorState), and a second that dispatches updates to PageClient when the first
editor state is received after focusing an element. During a layer tree commit in the UI process, we first
update the editor state, then commit the layer tree and apply scroll position changes, and finally scroll to
reveal the focused element if necessary.

When an editor state is delivered to the UI process in an out-of-band update (i.e. not in a layer tree commit),
simply dispatch the initial editor state for a focused element immediately.

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::scheduleFullEditorStateUpdate):

Add an IPC message to schedule an editor state update in the next remote layer tree commit. See below for more
details.

(WebKit::WebPageProxy::editorStateChanged):
(WebKit::WebPageProxy::dispatchDidReceiveEditorStateAfterFocus):

• UIProcess/WebPageProxy.h:
• UIProcess/gtk/WebPageProxyGtk.cpp:

(WebKit::WebPageProxy::updateEditorState):
(WebKit::WebPageProxy::editorStateChanged): Deleted.

• UIProcess/ios/WKContentViewInteraction.mm:

(-[WKContentView willFinishIgnoringCalloutBarFadeAfterPerformingAction]):

Additionally ensure that an editor state update is scheduled. This addresses a potential source of flakiness in
the layout test editing/selection/ios/selection-after-changing-text-with-callout-menu.html, where an editor
state update may only be scheduled after the next layout timer fires (this is the case in custom callout menu
actions that change the DOM but do not otherwise trigger any editing commands).

In the problematic scenario, the client could make a change that triggers layout soon; but before the layout
timer fires, the timer for the next remote layer tree commit could fire, such that the next layer tree commit
would not contain the relevant editor state.

This extra step ensures that we always *schedule* an editor state update when performing a callout menu action
that does not automatically dismiss, so that we can prevent the callout bar from dismissing during the correct
scope.

• UIProcess/ios/WebPageProxyIOS.mm:

(WebKit::WebPageProxy::layerTreeCommitComplete):
(WebKit::WebPageProxy::updateEditorState):

Rename editorStateChanged to updateEditorState, and make the editorStateChanged codepath only executed when an
out-of-band editor state update is delivered to the UI process.

(WebKit::WebPageProxy::dispatchDidReceiveEditorStateAfterFocus):

Notify the UI process that the initial editor state has been received; this prompts us to scroll to reveal the
focused element, if needed.

(WebKit::WebPageProxy::editorStateChanged): Deleted.

• UIProcess/mac/WebPageProxyMac.mm:

(WebKit::WebPageProxy::updateEditorState):
(WebKit::WebPageProxy::editorStateChanged): Deleted.

• UIProcess/win/WebPageProxyWin.cpp:

(WebKit::WebPageProxy::updateEditorState):
(WebKit::WebPageProxy::editorStateChanged): Deleted.

• UIProcess/wpe/WebPageProxyWPE.cpp:

(WebKit::WebPageProxy::updateEditorState):
(WebKit::WebPageProxy::editorStateChanged): Deleted.

• WebProcess/WebPage/WebPage.h:
• WebProcess/WebPage/WebPage.messages.in:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm (modified) (2 diffs)
• UIProcess/WebPageProxy.cpp (modified) (2 diffs)
• UIProcess/WebPageProxy.h (modified) (1 diff)
• UIProcess/gtk/WebPageProxyGtk.cpp (modified) (1 diff)
• UIProcess/ios/WKContentViewInteraction.mm (modified) (1 diff)
• UIProcess/ios/WebPageProxyIOS.mm (modified) (3 diffs)
• UIProcess/mac/WebPageProxyMac.mm (modified) (1 diff)
• UIProcess/win/WebPageProxyWin.cpp (modified) (1 diff)
• UIProcess/wpe/WebPageProxyWPE.cpp (modified) (1 diff)
• WebProcess/WebPage/WebPage.h (modified) (2 diffs)
• WebProcess/WebPage/WebPage.messages.in (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-05-02  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION: Layout test editing/selection/ios/selection-after-changing-text-with-callout-menu.html is failing
+        https://bugs.webkit.org/show_bug.cgi?id=197532
+        <rdar://problem/50177144>
+
+        Reviewed by Ryosuke Niwa.
+
+        Fixes layout tests that began failing after r244546. See below for details.
+
+        * UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
+        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
+
+        Partially reverts a change in r244546, after which we commit the layer tree and scroll before updating the
+        page's editor state. The purpose of this change was to ensure that UI process-side element focus scrolling logic
+        would not conflict with web-process-driven scrolling logic.
+
+        Instead, we split the existing logic in WebPageProxy::editorStateChanged into two pieces: one that updates the
+        editor state (by setting m_editorState), and a second that dispatches updates to PageClient when the first
+        editor state is received after focusing an element. During a layer tree commit in the UI process, we first
+        update the editor state, then commit the layer tree and apply scroll position changes, and finally scroll to
+        reveal the focused element if necessary.
+
+        When an editor state is delivered to the UI process in an out-of-band update (i.e. not in a layer tree commit),
+        simply dispatch the initial editor state for a focused element immediately.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::scheduleFullEditorStateUpdate):
+
+        Add an IPC message to schedule an editor state update in the next remote layer tree commit. See below for more
+        details.
+
+        (WebKit::WebPageProxy::editorStateChanged):
+        (WebKit::WebPageProxy::dispatchDidReceiveEditorStateAfterFocus):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/gtk/WebPageProxyGtk.cpp:
+        (WebKit::WebPageProxy::updateEditorState):
+        (WebKit::WebPageProxy::editorStateChanged): Deleted.
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (-[WKContentView willFinishIgnoringCalloutBarFadeAfterPerformingAction]):
+
+        Additionally ensure that an editor state update is scheduled. This addresses a potential source of flakiness in
+        the layout test editing/selection/ios/selection-after-changing-text-with-callout-menu.html, where an editor
+        state update may only be scheduled after the next layout timer fires (this is the case in custom callout menu
+        actions that change the DOM but do not otherwise trigger any editing commands).
+
+        In the problematic scenario, the client could make a change that triggers layout soon; but before the layout
+        timer fires, the timer for the next remote layer tree commit could fire, such that the next layer tree commit
+        would not contain the relevant editor state.
+
+        This extra step ensures that we always *schedule* an editor state update when performing a callout menu action
+        that does not automatically dismiss, so that we can prevent the callout bar from dismissing during the correct
+        scope.
+
+        * UIProcess/ios/WebPageProxyIOS.mm:
+        (WebKit::WebPageProxy::layerTreeCommitComplete):
+        (WebKit::WebPageProxy::updateEditorState):
+
+        Rename editorStateChanged to updateEditorState, and make the editorStateChanged codepath only executed when an
+        out-of-band editor state update is delivered to the UI process.
+
+        (WebKit::WebPageProxy::dispatchDidReceiveEditorStateAfterFocus):
+
+        Notify the UI process that the initial editor state has been received; this prompts us to scroll to reveal the
+        focused element, if needed.
+
+        (WebKit::WebPageProxy::editorStateChanged): Deleted.
+        * UIProcess/mac/WebPageProxyMac.mm:
+        (WebKit::WebPageProxy::updateEditorState):
+        (WebKit::WebPageProxy::editorStateChanged): Deleted.
+        * UIProcess/win/WebPageProxyWin.cpp:
+        (WebKit::WebPageProxy::updateEditorState):
+        (WebKit::WebPageProxy::editorStateChanged): Deleted.
+        * UIProcess/wpe/WebPageProxyWPE.cpp:
+        (WebKit::WebPageProxy::updateEditorState):
+        (WebKit::WebPageProxy::editorStateChanged): Deleted.
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/WebPage.messages.in:
+
 2019-05-02  Timothy Hatcher  <timothy@apple.com>
 

trunk/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm

@@ -200 +200 @@
     m_activityStateChangeID = layerTreeTransaction.activityStateChangeID();
 
+    if (layerTreeTransaction.hasEditorState())
+        m_webPageProxy.updateEditorState(layerTreeTransaction.editorState());
+
     if (m_remoteLayerTreeHost->updateLayerTree(layerTreeTransaction)) {
         if (layerTreeTransaction.transactionID() >= m_transactionIDForUnhidingContent)
@@ -254 +257 @@
 
     if (layerTreeTransaction.hasEditorState())
-        m_webPageProxy.editorStateChanged(layerTreeTransaction.editorState());
+        m_webPageProxy.dispatchDidReceiveEditorStateAfterFocus();
 
     if (auto milestones = layerTreeTransaction.newlyReachedPaintingMilestones())

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -2007 +2007 @@
 }
 
+void WebPageProxy::scheduleFullEditorStateUpdate()
+{
+    if (!hasRunningProcess())
+        return;
+
+    m_process->send(Messages::WebPage::ScheduleFullEditorStateUpdate(), m_pageID);
+}
+
 void WebPageProxy::executeEditCommand(const String& commandName, const String& argument, WTF::Function<void(CallbackBase::Error)>&& callbackFunction)
 {
@@ -6503 +6511 @@
     callback->performCallbackWithReturnValue(range);
 }
+
+void WebPageProxy::editorStateChanged(const EditorState& editorState)
+{
+    updateEditorState(editorState);
+    dispatchDidReceiveEditorStateAfterFocus();
+}
+
+#if !PLATFORM(IOS_FAMILY)
+
+void WebPageProxy::dispatchDidReceiveEditorStateAfterFocus()
+{
+}
+
+#endif
 
 #if ENABLE(APPLICATION_MANIFEST)

trunk/Source/WebKit/UIProcess/WebPageProxy.h

@@ -1424 +1424 @@
 #endif
     void editorStateChanged(const EditorState&);
+    void updateEditorState(const EditorState&);
+    void scheduleFullEditorStateUpdate();
+    void dispatchDidReceiveEditorStateAfterFocus();
 
 #if PLATFORM(COCOA)

trunk/Source/WebKit/UIProcess/gtk/WebPageProxyGtk.cpp

@@ -80 +80 @@
 }
 
-void WebPageProxy::editorStateChanged(const EditorState& editorState)
+void WebPageProxy::updateEditorState(const EditorState& editorState)
 {
     m_editorState = editorState;

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

@@ -3921 +3921 @@
 {
     _ignoreSelectionCommandFadeCount++;
+    _page->scheduleFullEditorStateUpdate();
     _page->callAfterNextPresentationUpdate([weakSelf = WeakObjCPtr<WKContentView>(self)] (auto) {
         if (auto strongSelf = weakSelf.get())

trunk/Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm

@@ -431 +431 @@
 {
     pageClient().layerTreeCommitComplete();
+
+    if (m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement && !m_editorState.isMissingPostLayoutData) {
+        pageClient().didReceiveEditorStateUpdateAfterFocus();
+        m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement = false;
+    }
 }
 
@@ -1103 +1108 @@
 }
 
-void WebPageProxy::editorStateChanged(const EditorState& editorState)
+void WebPageProxy::updateEditorState(const EditorState& editorState)
 {
     bool couldChangeSecureInputState = m_editorState.isInPasswordField != editorState.isInPasswordField || m_editorState.selectionIsNone;
@@ -1120 +1125 @@
     pageClient().selectionDidChange();
     updateFontAttributesAfterEditorStateChange();
-
-    if (m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement && !m_editorState.isMissingPostLayoutData) {
-        pageClient().didReceiveEditorStateUpdateAfterFocus();
-        m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement = false;
-    }
+}
+
+void WebPageProxy::dispatchDidReceiveEditorStateAfterFocus()
+{
+    if (!m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement || m_editorState.isMissingPostLayoutData)
+        return;
+
+    pageClient().didReceiveEditorStateUpdateAfterFocus();
+    m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement = false;
 }
 

trunk/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm

@@ -608 +608 @@
 }
 
-void WebPageProxy::editorStateChanged(const EditorState& editorState)
+void WebPageProxy::updateEditorState(const EditorState& editorState)
 {
     bool couldChangeSecureInputState = m_editorState.isInPasswordField != editorState.isInPasswordField || m_editorState.selectionIsNone;

trunk/Source/WebKit/UIProcess/win/WebPageProxyWin.cpp

@@ -61 +61 @@
 }
 
-void WebPageProxy::editorStateChanged(const EditorState& editorState)
+void WebPageProxy::updateEditorState(const EditorState& editorState)
 {
     m_editorState = editorState;

trunk/Source/WebKit/UIProcess/wpe/WebPageProxyWPE.cpp

@@ -65 +65 @@
 }
 
-void WebPageProxy::editorStateChanged(const EditorState&)
+void WebPageProxy::updateEditorState(const EditorState&)
 {
     notImplemented();

trunk/Source/WebKit/WebProcess/WebPage/WebPage.h

@@ -1184 +1184 @@
 
     void updateIntrinsicContentSizeIfNeeded(const WebCore::IntSize&);
+    void scheduleFullEditorStateUpdate();
 
 private:
@@ -1200 +1201 @@
     void platformEditorState(WebCore::Frame&, EditorState& result, IncludePostLayoutDataHint) const;
     void sendEditorStateUpdate();
-    void scheduleFullEditorStateUpdate();
 
 #if PLATFORM(COCOA)

trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in

@@ -203 +203 @@
     ForceRepaint(WebKit::CallbackID callbackID)
 
+    ScheduleFullEditorStateUpdate()
+
 #if PLATFORM(COCOA)
     # Dictionary support.