Changeset 248974 in webkit


Ignore:
Timestamp:
Aug 21, 2019 4:05:26 PM (5 years ago)
Author:
timothy_horton@apple.com
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.
Location:
trunk
Files:
2 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r248966 r248974  
     12019-08-21  Tim Horton  <timothy_horton@apple.com>
     2
     3        [Mail] Tapping top of message scrolls back to copied text instead of top of the message
     4        https://bugs.webkit.org/show_bug.cgi?id=200999
     5        <rdar://problem/54564878>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        * editing/selection/ios/change-selection-by-tapping-with-existing-selection-expected.txt: Added.
     10        * editing/selection/ios/change-selection-by-tapping-with-existing-selection.html: Added.
     11
    1122019-08-21  Alex Christensen  <achristensen@webkit.org>
    213

  • trunk/Source/WebCore/ChangeLog

    r248971 r248974  
     12019-08-21  Tim Horton  <timothy_horton@apple.com>
     2
     3        [Mail] Tapping top of message scrolls back to copied text instead of top of the message
     4        https://bugs.webkit.org/show_bug.cgi?id=200999
     5        <rdar://problem/54564878>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        Test: editing/selection/ios/change-selection-by-tapping-with-existing-selection.html
     10
     11        * page/EditorClient.h:
     12        (WebCore::EditorClient::shouldAllowSingleClickToChangeSelection const):
     13        * page/EventHandler.cpp:
     14        (WebCore::EventHandler::handleMousePressEventSingleClick):
     15        Instead of encoding platform behaviors in EventHandler, defer to EditorClient.
     16
    1172019-08-21  Chris Dumez  <cdumez@apple.com>
    218

  • trunk/Source/WebCore/page/EditorClient.h

    r247722 r248974  
    187187
    188188    virtual bool canShowFontPanel() const = 0;
     189
     190    virtual bool shouldAllowSingleClickToChangeSelection(Node&, const VisibleSelection&) const { return true; }
    189191};
    190192

  • trunk/Source/WebCore/page/EventHandler.cpp

    r248846 r248974  
    699699    TextGranularity granularity = CharacterGranularity;
    700700
    701 #if PLATFORM(IOS_FAMILY)
    702     // The text selection assistant will handle selection in the case where we are already editing the node
    703     auto* editableRoot = newSelection.rootEditableElement();
    704     if (editableRoot && editableRoot == targetNode->rootEditableElement())
     701    if (!m_frame.editor().client()->shouldAllowSingleClickToChangeSelection(*targetNode, newSelection))
    705702        return true;
    706 #endif
    707703
    708704    if (extendSelection && newSelection.isCaretOrRange()) {

  • trunk/Source/WebKit/ChangeLog

    r248973 r248974  
     12019-08-21  Tim Horton  <timothy_horton@apple.com>
     2
     3        [Mail] Tapping top of message scrolls back to copied text instead of top of the message
     4        https://bugs.webkit.org/show_bug.cgi?id=200999
     5        <rdar://problem/54564878>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        In the case where you have a WebCore selection but are not first responder,
     10        when you tap the WKWebView to become first responder, EventHandler would
     11        bail from setting the selection, assuming UIKit was going to do it. This
     12        behavior was introduced in r233311.
     13
     14        However, since we are not first responder, UIKit does not change the
     15        selection, since it considers the view to not be editable.
     16
     17        Fix this by letting WebCore set the selection in this case, as it used to.
     18
     19        * WebProcess/WebCoreSupport/WebEditorClient.h:
     20        * WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:
     21        (WebKit::WebEditorClient::shouldAllowSingleClickToChangeSelection const):
     22        * WebProcess/WebPage/WebPage.h:
     23        (WebKit::WebPage::isShowingInputViewForFocusedElement const):
     24        Copy the logic from EventHandler, with the added caveat (which fixes the
     25        aforementioned behavior) that we will allow EventHandler to change the
     26        selection if we don't have a focused node in the UIKit sense, because
     27        we know that the platform text interaction code will *not* change the
     28        selection if that is the case, so it's up to us.
     29
    1302019-08-21  Chris Dumez  <cdumez@apple.com>
    231

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h

    r248762 r248974  
    182182    bool performsTwoStepPaste(WebCore::DocumentFragment*) final;
    183183    void updateStringForFind(const String&) final;
     184    bool shouldAllowSingleClickToChangeSelection(WebCore::Node&, const WebCore::VisibleSelection&) const final;
    184185#endif
    185186

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm

    r244982 r248974  
    103103}
    104104
     105bool WebEditorClient::shouldAllowSingleClickToChangeSelection(WebCore::Node& targetNode, const WebCore::VisibleSelection& newSelection) const
     106{
     107    // The text selection assistant will handle selection in the case where we are already editing the node
     108    auto* editableRoot = newSelection.rootEditableElement();
     109    return !editableRoot || editableRoot != targetNode.rootEditableElement() || !m_page->isShowingInputViewForFocusedElement();
     110}
     111
    105112} // namespace WebKit
    106113

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

    r248901 r248974  
    679679    void setFocusedElementSelectedIndex(uint32_t index, bool allowMultipleSelection);
    680680    void setIsShowingInputViewForFocusedElement(bool);
     681    bool isShowingInputViewForFocusedElement() const { return m_isShowingInputViewForFocusedElement; }
    681682    void updateSelectionAppearance();
    682683    void getSelectionContext(CallbackID);

  • trunk/Source/WebKitLegacy/mac/ChangeLog

    r248960 r248974  
     12019-08-21  Tim Horton  <timothy_horton@apple.com>
     2
     3        [Mail] Tapping top of message scrolls back to copied text instead of top of the message
     4        https://bugs.webkit.org/show_bug.cgi?id=200999
     5        <rdar://problem/54564878>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        * WebCoreSupport/WebEditorClient.h:
     10        * WebCoreSupport/WebEditorClient.mm:
     11        (WebEditorClient::shouldAllowSingleClickToChangeSelection const):
     12        Copy the existing behavior from EventHandler.
     13        We do not fix the bug in WebKitLegacy for a multitude of reasons, primarily
     14        because we do not know of any user impact.
     15
    1162019-08-21  Ryosuke Niwa  <rniwa@webkit.org>
    217

  • trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h

    r248762 r248974  
    177177    void registerUndoOrRedoStep(WebCore::UndoStep&, bool isRedo);
    178178
     179#if PLATFORM(IOS_FAMILY)
     180    bool shouldAllowSingleClickToChangeSelection(WebCore::Node& targetNode, const WebCore::VisibleSelection& newSelection) const;
     181#endif
     182
    179183    bool canShowFontPanel() const final
    180184    {

  • trunk/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm

    r248697 r248974  
    12591259#endif
    12601260}
     1261
     1262#if PLATFORM(IOS_FAMILY)
     1263bool WebEditorClient::shouldAllowSingleClickToChangeSelection(WebCore::Node& targetNode, const WebCore::VisibleSelection& newSelection) const
     1264{
     1265    // The text selection assistant will handle selection in the case where we are already editing the node
     1266    auto* editableRoot = newSelection.rootEditableElement();
     1267    return !editableRoot || editableRoot != targetNode.rootEditableElement();
     1268}
     1269#endif
Note: See TracChangeset for help on using the changeset viewer.