[macOS] Shift-tab in a bullet list in Mail Compose jumps back to Subject field
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Jun 2020 02:26:50 +0000 (02:26 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Jun 2020 02:26:50 +0000 (02:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213320
<rdar://problem/63831962>

Reviewed by Tim Horton.

Source/WebCore:

After the changes in r262051, pressing shift-tab in a bulleted list in Mail compose on macOS no longer triggers
an outdent command. This is because the default behavior of the "keydown" event will now relinquish focus to the
embedding client (i.e. the "chrome"). In this case, Mail makes the Subject field above the compose web view the
first responder.

This is necessary on iOS, where Mail does not attempt to intercept shift+tab and move focus to the subject line.
However, Mail on macOS intercepts the keypress event, and either triggers outdent (if the selection is inside a
list or blockquote) or focuses the Subject line. Since focus is relinquished during "keydown", this logic no
longer runs, and hitting shift+tab in a list always relinquishes focus.

To address this, refactor the changes made in r262051 so that we treat the default behavior of the "keypress"
event (rather than "keydown") as relinquishing focus. See WebKit/ChangeLog for more details.

Test: WebKit.ShiftTabDoesNotTakeFocusFromEditableWebViewWhenPreventingKeyPress

* page/EventHandler.cpp:
(WebCore::EventHandler::defaultTabEventHandler):

Remove a call to relinquish focus when handling tab.

* page/FocusController.h:

Source/WebKit:

See WebCore/ChangeLog for more detail.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::handleKeyEventByRelinquishingFocusToChrome):
* WebProcess/WebPage/WebPage.h:
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::handleEditingKeyboardEvent):

Hoist logic for relinquishing focus as a result of shift+tab from EventHandler to WebPage, so that it is the
default behavior when processing a "keypress" event that corresponds to shift+tab in an editable web view on
Cocoa platforms. If we do end up relinquishing focus, then we consider the keypress event to be handled.

* WebProcess/WebPage/mac/WebPageMac.mm:
(WebKit::WebPage::handleEditingKeyboardEvent):

Tools:

Add a new API test to verify that preventing the "keypress" event for shift+tab in an SPI-editable web view
causes us to avoid relinquishing focus.

* TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263196 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/FocusController.h
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm

index 1b4a143..49edc82 100644 (file)
@@ -1,3 +1,33 @@
+2020-06-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [macOS] Shift-tab in a bullet list in Mail Compose jumps back to Subject field
+        https://bugs.webkit.org/show_bug.cgi?id=213320
+        <rdar://problem/63831962>
+
+        Reviewed by Tim Horton.
+
+        After the changes in r262051, pressing shift-tab in a bulleted list in Mail compose on macOS no longer triggers
+        an outdent command. This is because the default behavior of the "keydown" event will now relinquish focus to the
+        embedding client (i.e. the "chrome"). In this case, Mail makes the Subject field above the compose web view the
+        first responder.
+
+        This is necessary on iOS, where Mail does not attempt to intercept shift+tab and move focus to the subject line.
+        However, Mail on macOS intercepts the keypress event, and either triggers outdent (if the selection is inside a
+        list or blockquote) or focuses the Subject line. Since focus is relinquished during "keydown", this logic no
+        longer runs, and hitting shift+tab in a list always relinquishes focus.
+
+        To address this, refactor the changes made in r262051 so that we treat the default behavior of the "keypress"
+        event (rather than "keydown") as relinquishing focus. See WebKit/ChangeLog for more details.
+
+        Test: WebKit.ShiftTabDoesNotTakeFocusFromEditableWebViewWhenPreventingKeyPress
+
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::defaultTabEventHandler):
+
+        Remove a call to relinquish focus when handling tab.
+
+        * page/FocusController.h:
+
 2020-06-16  Antoine Quint  <graouts@webkit.org>
 
         quikr.com: unable to select item from dropdown
index 5e3db4d..6f1f1b4 100644 (file)
@@ -4097,24 +4097,14 @@ void EventHandler::defaultTabEventHandler(KeyboardEvent& event)
     if (!page)
         return;
 
-    FocusDirection focusDirection = event.shiftKey() ? FocusDirectionBackward : FocusDirectionForward;
-
     // Tabs can be used in design mode editing.
     if (m_frame.document()->inDesignMode())
         return;
 
-    // Allow shift-tab to relinquish focus even if we don't allow tab to cycle between elements inside the view.
-    // We can only do this for shift-tab, not tab itself because tabKeyCyclesThroughElements is used to make
-    // tab character insertion work in editable web views.
-    if (!page->tabKeyCyclesThroughElements()) {
-        if (focusDirection == FocusDirectionBackward) {
-            if (page->focusController().relinquishFocusToChrome(focusDirection))
-                event.setDefaultHandled();
-        }
+    if (!page->tabKeyCyclesThroughElements())
         return;
-    }
 
-    if (page->focusController().advanceFocus(focusDirection, &event))
+    if (page->focusController().advanceFocus(event.shiftKey() ? FocusDirectionBackward : FocusDirectionForward, &event))
         event.setDefaultHandled();
 }
 
index 17b725a..17d34c5 100644 (file)
@@ -78,7 +78,7 @@ public:
     void setFocusedElementNeedsRepaint();
     Seconds timeSinceFocusWasSet() const;
 
-    bool relinquishFocusToChrome(FocusDirection);
+    WEBCORE_EXPORT bool relinquishFocusToChrome(FocusDirection);
 
 private:
     void setActiveInternal(bool);
index 7e5a3b4..7187dda 100644 (file)
@@ -1,3 +1,26 @@
+2020-06-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [macOS] Shift-tab in a bullet list in Mail Compose jumps back to Subject field
+        https://bugs.webkit.org/show_bug.cgi?id=213320
+        <rdar://problem/63831962>
+
+        Reviewed by Tim Horton.
+
+        See WebCore/ChangeLog for more detail.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::handleKeyEventByRelinquishingFocusToChrome):
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/ios/WebPageIOS.mm:
+        (WebKit::WebPage::handleEditingKeyboardEvent):
+
+        Hoist logic for relinquishing focus as a result of shift+tab from EventHandler to WebPage, so that it is the
+        default behavior when processing a "keypress" event that corresponds to shift+tab in an editable web view on
+        Cocoa platforms. If we do end up relinquishing focus, then we consider the keypress event to be handled.
+
+        * WebProcess/WebPage/mac/WebPageMac.mm:
+        (WebKit::WebPage::handleEditingKeyboardEvent):
+
 2020-06-17  Beth Dakin  <bdakin@apple.com>
 
         Remove references to blacklist/whitelist in the old C API
index 0965d2c..770910b 100644 (file)
@@ -2840,6 +2840,24 @@ void WebPage::keyEvent(const WebKeyboardEvent& keyboardEvent)
     send(Messages::WebPageProxy::DidReceiveEvent(static_cast<uint32_t>(keyboardEvent.type()), handled));
 }
 
+bool WebPage::handleKeyEventByRelinquishingFocusToChrome(const KeyboardEvent& event)
+{
+    if (m_page->tabKeyCyclesThroughElements())
+        return false;
+
+    if (event.charCode() != '\t')
+        return false;
+
+    if (!event.shiftKey() || event.ctrlKey() || event.metaKey() || event.altGraphKey())
+        return false;
+
+    ASSERT(event.type() == eventNames().keypressEvent);
+    // Allow a shift-tab keypress event to relinquish focus even if we don't allow tab to cycle between
+    // elements inside the view. We can only do this for shift-tab, not tab itself because
+    // tabKeyCyclesThroughElements is used to make tab character insertion work in editable web views.
+    return m_page->focusController().relinquishFocusToChrome(FocusDirectionBackward);
+}
+
 void WebPage::validateCommand(const String& commandName, CallbackID callbackID)
 {
     bool isEnabled = false;
index 0495602..4dabe09 100644 (file)
@@ -1417,6 +1417,7 @@ private:
 #endif
 
     bool performDefaultBehaviorForKeyEvent(const WebKeyboardEvent&);
+    bool handleKeyEventByRelinquishingFocusToChrome(const WebCore::KeyboardEvent&);
 
 #if PLATFORM(MAC)
     bool executeKeypressCommandsInternal(const Vector<WebCore::KeypressCommand>&, WebCore::KeyboardEvent*);
index 70c8f2a..fe37437 100644 (file)
@@ -493,6 +493,9 @@ bool WebPage::handleEditingKeyboardEvent(KeyboardEvent& event)
     if (platformEvent->isSyntheticEvent())
         return false;
 
+    if (handleKeyEventByRelinquishingFocusToChrome(event))
+        return true;
+
     // FIXME: Interpret the event immediately upon receiving it in UI process, without sending to WebProcess first.
     bool eventWasHandled = false;
     bool sendResult = WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPageProxy::InterpretKeyEvent(editorState(ShouldPerformLayout::Yes), platformEvent->type() == PlatformKeyboardEvent::Char),
index f6ac20f..1c97fa8 100644 (file)
@@ -294,6 +294,9 @@ bool WebPage::handleEditingKeyboardEvent(KeyboardEvent& event)
     if (platformEvent->type() != PlatformEvent::Char && platformEvent->windowsVirtualKeyCode() == VK_ESCAPE && commands.size() == 1 && commandNameForSelectorName(commands[0].commandName) == "cancelOperation")
         return false;
 
+    if (handleKeyEventByRelinquishingFocusToChrome(event))
+        return true;
+
     bool eventWasHandled = false;
 
     // Are there commands that could just cause text insertion if executed via Editor?
index 6b50621..f41076b 100644 (file)
@@ -1,3 +1,16 @@
+2020-06-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [macOS] Shift-tab in a bullet list in Mail Compose jumps back to Subject field
+        https://bugs.webkit.org/show_bug.cgi?id=213320
+        <rdar://problem/63831962>
+
+        Reviewed by Tim Horton.
+
+        Add a new API test to verify that preventing the "keypress" event for shift+tab in an SPI-editable web view
+        causes us to avoid relinquishing focus.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:
+
 2020-06-17  Andy Estes  <aestes@apple.com>
 
         REGRESSION (r258092): fast/forms/ios/file-upload-panel.html fails when HAVE(UICONTEXTMENU_LOCATION)
index 229ee9b..6c7b1f5 100644 (file)
@@ -863,6 +863,34 @@ TEST(WebKit, ShiftTabTakesFocusFromEditableWebView)
     ASSERT_EQ([delegate takenDirection], _WKFocusDirectionBackward);
 }
 
+TEST(WebKit, ShiftTabDoesNotTakeFocusFromEditableWebViewWhenPreventingKeyPress)
+{
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);
+    [webView _setEditable:YES];
+
+    auto delegate = adoptNS([[FocusDelegate alloc] init]);
+    [delegate setUseShiftTab:YES];
+    [webView setUIDelegate:delegate.get()];
+    NSString *markup = @"<script>"
+        "    function loaded() {"
+        "        document.body.focus();"
+        "        document.body.addEventListener('keypress', e => e.preventDefault());"
+        "        document.body.addEventListener('keyup', () => webkit.messageHandlers.testHandler.postMessage('keyup'));"
+        "        alert('ready');"
+        "    }"
+        "</script>"
+        "<body onload='loaded()'></body>";
+
+    __block bool handledKeyUp = false;
+    [webView performAfterReceivingMessage:@"keyup" action:^{
+        handledKeyUp = true;
+    }];
+    [webView synchronouslyLoadHTMLString:markup];
+
+    TestWebKitAPI::Util::run(&handledKeyUp);
+    EXPECT_FALSE(delegate->_done);
+}
+
 TEST(WebKit, TabDoesNotTakeFocusFromEditableWebView)
 {
     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);