Bug 193084 - [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
Summary: [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P1 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar, Regression
Depends on: 192802
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-02 11:35 PST by Daniel Bates
Modified: 2019-01-03 07:38 PST (History)
7 users (show)

See Also:


Attachments
Test (352 bytes, text/html)
2019-01-02 11:35 PST, Daniel Bates
no flags Details
Patch (12.65 KB, patch)
2019-01-02 17:37 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-01-02 11:35:06 PST
Created attachment 358192 [details]
Test

Seen with iOS WebKit using a 6th generation iPad in landscape orientation with a hardware keyboard attached. We may not scroll an offscreen <select> into view following r23944.

Steps to reproduce:

1. Open the attached test case.
2. Press the tab key on the hardware keyboard four times.

Then the bottom-most <select> (whose selected item is "Fifth") should be selected the view scrolled to show it. But the view is scrolled to the top of the page.

For some reason we will scroll the <select>s in the middle of the page into view (why?).
Comment 1 Radar WebKit Bug Importer 2019-01-02 11:36:33 PST
<rdar://problem/47006884>
Comment 2 Radar WebKit Bug Importer 2019-01-02 11:36:38 PST
<rdar://problem/47006882>
Comment 3 Wenson Hsieh 2019-01-02 12:07:42 PST
It's interesting that the input accessory view buttons (˄ and ˅) do the right thing, but the keyboard does not. I think I overlooked a nuance in the way that _didAccessoryTabInitiateFocus state is maintained when tabbing/shift-tabbing via hardware keyboard.
Comment 4 Wenson Hsieh 2019-01-02 14:48:08 PST
Ah, this broke in r239441 because I changed this (in WebPage::getFocusedElementInformation, formerly WebPage::getAssistedNodeInformation) from:

    if (!information.elementRect.contains(m_lastInteractionLocation))
        information.selectionRect = IntRect();

...to:

    if (!information.elementRect.contains(m_lastInteractionLocation))
        information.elementInteractionLocation = { };

...and in WKContentViewInteraction.mm, I then have this logic to determine where to zoom/scroll:

    WebCore::IntRect elementInteractionRect(elementInfo.elementInteractionLocation, { 1, 1 });
    if (!shouldZoomToRevealSelectionRect(elementInfo.elementType))
        return elementInteractionRect;

So the net effect of this is that when zooming between form controls when the last interaction location is outside of the element bounding rect, we'll end up passing in a selection rect of {{ 0, 0 }, { 1, 1 }} instead of {{ 0, 0 }, { 0, 0 }} as we did before.

Subsequently, -[WKWebView _zoomToFocusRect:selectionRect:insideFixed:fontSize:minimumScale:maximumScale:allowScaling:forceScroll:] treats {{ 0, 0 }, { 0, 0 }} as a special case by not attempting to reveal that rect, whereas it happily will attempt to reveal {{ 0, 0 }, { 1, 1 }}, causing the page to scroll to the top.

The fix is to revert this unintentional behavior change.
Comment 5 Wenson Hsieh 2019-01-02 17:37:07 PST
Created attachment 358231 [details]
Patch
Comment 6 WebKit Commit Bot 2019-01-03 07:38:42 PST
Comment on attachment 358231 [details]
Patch

Clearing flags on attachment: 358231

Committed r239590: <https://trac.webkit.org/changeset/239590>
Comment 7 WebKit Commit Bot 2019-01-03 07:38:43 PST
All reviewed patches have been landed.  Closing bug.