Bug 193084

Summary: [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, megan_gardner, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P1 Keywords: InRadar, Regression
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 12   
Bug Depends on: 192802    
Bug Blocks:    
Attachments:
Description Flags
Test
none
Patch none

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.