Changeset 239590 in webkit


Ignore:
Timestamp:
Jan 3, 2019 7:38:33 AM (5 years ago)
Author:
Wenson Hsieh
Message:

[iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
​https://bugs.webkit.org/show_bug.cgi?id=193084
<rdar://problem/47006882>

Reviewed by Simon Fraser.

Source/WebKit:

In WKWebView.mm, -_zoomToFocusRect: will ignore the given selection rect if it is of size { 0, 0 } and at
the origin. Prior to r239441, when using the tab key to move focus between non-editable form controls (or any
other method that doesn't involve tapping on the focused select element, with the exception of the next and
previous buttons in the input accessory view), we would compute a selection rect of {{ 0, 0 }, { 0, 0 }}, and
subsequently try to scroll the focused element to the center of the visible area, without taking the selection
rect into account.

However, after r239441, the web process sends the element interaction location to the UI process, which then
computes the selection rect by taking this location and adding a size of { 1, 1 } (before r239441, this was
done in WebPage::getAssistedNodeInformation). However, our new implementation doesn't take into account the
case where the element interaction rect is null, which happens when the last interaction location is outside of
the bounding rect of the element. In this case, we set the element interaction location to { 0, 0 } and end up
computing a selection rect of {{ 0, 0 }, { 1, 1 }} instead of {{ 0, 0 }, { 0, 0 }} as we would have
previously done. This causes us to scroll up to the origin, instead of revealing the focused element.

To fix this, we restore the pre-r239441 behavior. See additional comments below for details.

Test: fast/forms/ios/scroll-to-reveal-focused-select.html

  • Shared/FocusedElementInformation.cpp:

(WebKit::FocusedElementInformation::encode const):
(WebKit::FocusedElementInformation::decode):

  • Shared/FocusedElementInformation.h:

Rename elementInteractionLocation to lastInteractionLocation. This was previously
elementInteractionLocation due to existing logic that tries to move the interaction location into the bounding
rect of the element in the case where visual viewports are disabled; however, since this feature has long been
enabled by default for all modern WebKit clients (internal and external), it's simpler to just use always send
the last interaction location over to the UI process, and have the UI process use {{ 0, 0 }, { 0, 0 }} if
the interaction location is outside of the element rect.

In the very unlikely event that any modern WebKit client disables visual viewports, this will still behave
reasonably, since we'll just use {{ 0, 0 }, { 0, 0 }} as the target rect and scroll to reveal the entire
element rather than the top left corner of the element.

  • UIProcess/ios/WKContentViewInteraction.mm:

(rectToRevealWhenZoomingToFocusedElement):

  • WebProcess/WebPage/ios/WebPageIOS.mm:

Move the check for whether the interaction location is inside the element's bounding rect from the web process
to the UI process. This relocates the logic to determine whether the selection rect should be a 1 by 1 fallback
interaction rect or the zero rect ({{ 0, 0 }, { 0, 0 }}) closer to the code that actually uses this rect.

(WebKit::WebPage::getFocusedElementInformation):

LayoutTests:

Add a layout test to verify that focusing a select element by tapping outside of it scrolls to reveal the
focused select element.

  • fast/forms/ios/scroll-to-reveal-focused-select-expected.txt: Added.
  • fast/forms/ios/scroll-to-reveal-focused-select.html: Added.
Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r239583 r239590  
     12019-01-03  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
     4        https://bugs.webkit.org/show_bug.cgi?id=193084
     5        <rdar://problem/47006882>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Add a layout test to verify that focusing a select element by tapping outside of it scrolls to reveal the
     10        focused select element.
     11
     12        * fast/forms/ios/scroll-to-reveal-focused-select-expected.txt: Added.
     13        * fast/forms/ios/scroll-to-reveal-focused-select.html: Added.
     14
    1152019-01-02  Devin Rousso  <webkit@devinrousso.com>
    216

  • trunk/Source/WebKit/ChangeLog

    r239589 r239590  
     12019-01-03  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
     4        https://bugs.webkit.org/show_bug.cgi?id=193084
     5        <rdar://problem/47006882>
     6
     7        Reviewed by Simon Fraser.
     8
     9        In `WKWebView.mm`, `-_zoomToFocusRect:` will ignore the given selection rect if it is of size `{ 0, 0 }` and at
     10        the origin. Prior to r239441, when using the tab key to move focus between non-editable form controls (or any
     11        other method that doesn't involve tapping on the focused select element, with the exception of the next and
     12        previous buttons in the input accessory view), we would compute a selection rect of `{{ 0, 0 }, { 0, 0 }}`, and
     13        subsequently try to scroll the focused element to the center of the visible area, without taking the selection
     14        rect into account.
     15
     16        However, after r239441, the web process sends the element interaction location to the UI process, which then
     17        computes the selection rect by taking this location and adding a size of `{ 1, 1 }` (before r239441, this was
     18        done in `WebPage::getAssistedNodeInformation`). However, our new implementation doesn't take into account the
     19        case where the element interaction rect is null, which happens when the last interaction location is outside of
     20        the bounding rect of the element. In this case, we set the element interaction location to { 0, 0 } and end up
     21        computing a selection rect of `{{ 0, 0 }, { 1, 1 }}` instead of `{{ 0, 0 }, { 0, 0 }}` as we would have
     22        previously done. This causes us to scroll up to the origin, instead of revealing the focused element.
     23
     24        To fix this, we restore the pre-r239441 behavior. See additional comments below for details.
     25
     26        Test: fast/forms/ios/scroll-to-reveal-focused-select.html
     27
     28        * Shared/FocusedElementInformation.cpp:
     29        (WebKit::FocusedElementInformation::encode const):
     30        (WebKit::FocusedElementInformation::decode):
     31        * Shared/FocusedElementInformation.h:
     32
     33        Rename `elementInteractionLocation` to `lastInteractionLocation`. This was previously
     34        `elementInteractionLocation` due to existing logic that tries to move the interaction location into the bounding
     35        rect of the element in the case where visual viewports are disabled; however, since this feature has long been
     36        enabled by default for all modern WebKit clients (internal and external), it's simpler to just use always send
     37        the last interaction location over to the UI process, and have the UI process use `{{ 0, 0 }, { 0, 0 }}` if
     38        the interaction location is outside of the element rect.
     39
     40        In the very unlikely event that any modern WebKit client disables visual viewports, this will still behave
     41        reasonably, since we'll just use `{{ 0, 0 }, { 0, 0 }}` as the target rect and scroll to reveal the entire
     42        element rather than the top left corner of the element.
     43
     44        * UIProcess/ios/WKContentViewInteraction.mm:
     45        (rectToRevealWhenZoomingToFocusedElement):
     46        * WebProcess/WebPage/ios/WebPageIOS.mm:
     47
     48        Move the check for whether the interaction location is inside the element's bounding rect from the web process
     49        to the UI process. This relocates the logic to determine whether the selection rect should be a 1 by 1 fallback
     50        interaction rect or the zero rect (`{{ 0, 0 }, { 0, 0 }}`) closer to the code that actually uses this rect.
     51
     52        (WebKit::WebPage::getFocusedElementInformation):
     53
    1542019-01-03  Wenson Hsieh  <wenson_hsieh@apple.com>
    255

  • trunk/Source/WebKit/Shared/FocusedElementInformation.cpp

    r239454 r239590  
    6565{
    6666    encoder << elementRect;
    67     encoder << elementInteractionLocation;
     67    encoder << lastInteractionLocation;
    6868    encoder << minimumScaleFactor;
    6969    encoder << maximumScaleFactor;
    … …  
    113113        return false;
    114114
    115     if (!decoder.decode(result.elementInteractionLocation))
     115    if (!decoder.decode(result.lastInteractionLocation))
    116116        return false;
    117117

  • trunk/Source/WebKit/Shared/FocusedElementInformation.h

    r239454 r239590  
    9898struct FocusedElementInformation {
    9999    WebCore::IntRect elementRect;
    100     WebCore::IntPoint elementInteractionLocation;
     100    WebCore::IntPoint lastInteractionLocation;
    101101    double minimumScaleFactor { -INFINITY };
    102102    double maximumScaleFactor { INFINITY };

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

    r239547 r239590  
    43704370static WebCore::FloatRect rectToRevealWhenZoomingToFocusedElement(const WebKit::FocusedElementInformation& elementInfo, const WebKit::EditorState& editorState)
    43714371{
    4372     WebCore::IntRect elementInteractionRect(elementInfo.elementInteractionLocation, { 1, 1 });
     4372    WebCore::IntRect elementInteractionRect;
     4373    if (elementInfo.elementRect.contains(elementInfo.lastInteractionLocation))
     4374        elementInteractionRect = { elementInfo.lastInteractionLocation, { 1, 1 } };
     4375
    43734376    if (!shouldZoomToRevealSelectionRect(elementInfo.elementType))
    43744377        return elementInteractionRect;

  • trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

    r239584 r239590  
    23632363    layoutIfNeeded();
    23642364
    2365     information.elementInteractionLocation = m_lastInteractionLocation;
     2365    information.lastInteractionLocation = m_lastInteractionLocation;
    23662366
    23672367    if (auto* renderer = m_focusedElement->renderer()) {
    … …  
    23822382            information.elementRect = frameView->contentsToRootView(renderer->absoluteBoundingBoxRect());
    23832383            frameView->setCustomFixedPositionLayoutRect(currentFixedPositionRect);
    2384            
    2385             if (!information.elementRect.contains(m_lastInteractionLocation))
    2386                 information.elementInteractionLocation = information.elementRect.location();
    2387         } else {
    2388             // Don't use the selection rect if interaction was outside the element rect.
    2389             if (!information.elementRect.contains(m_lastInteractionLocation))
    2390                 information.elementInteractionLocation = { };
    23912384        }
    23922385    } else
Note: See TracChangeset for help on using the changeset viewer.