Bug 236584 - focus({preventScroll: true}) does not prevent scrolling on iOS
Summary: focus({preventScroll: true}) does not prevent scrolling on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: Safari 15
Hardware: iPhone / iPad iOS 15
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-14 03:47 PST by Alex Moore
Modified: 2022-05-16 15:37 PDT (History)
19 users (show)

See Also:


Attachments
test case (1.73 KB, text/html)
2022-02-14 03:47 PST, Alex Moore
no flags Details
For EWS (54.37 KB, patch)
2022-02-28 13:44 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
v2 (27.64 KB, patch)
2022-02-28 17:36 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Add missing tests (33.04 KB, patch)
2022-02-28 17:43 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 Alex Moore 2022-02-14 03:47:21 PST
Created attachment 451882 [details]
test case

REPORT:
iOS (15.3)

Although focus({preventScroll: true}) is supposed to be supported on iOS, it doesn't seem to have any effect. As you can see in the test case, when scrolling the input out of view and clicking the button to focus it, it scrolls into view, where-as scroll should not be affected.

This works as expected on MacOS.

Feature implemented last year in ticket https://bugs.webkit.org/show_bug.cgi?id=178583
Comment 1 Radar WebKit Bug Importer 2022-02-14 09:56:55 PST
<rdar://problem/88911184>
Comment 2 Sam Sneddon [:gsnedders] 2022-02-22 08:23:33 PST
Because it's defined in the IDL, this means trying to feature detect it (like https://github.com/shoelace-style/shoelace/blob/e60b5f670a208704bbeb9ebd6e4393bb596a244d/src/internal/support.ts) claims it is supported, but then it appears to not work.
Comment 3 Wenson Hsieh 2022-02-28 13:44:11 PST
Created attachment 453420 [details]
For EWS
Comment 4 Simon Fraser (smfr) 2022-02-28 15:43:32 PST
Comment on attachment 453420 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=453420&action=review

> Source/WebKit/UIProcess/WebPageProxy.h:1651
> +    enum class PreventScroll : bool { No, Yes };
> +    void startWaitingForEditorStateAfterFocusingElement(PreventScroll preventScroll) { m_preventScrollAfterWaitingForEditorStateAfterFocusingElement = preventScroll; }
> +    void stopWaitingForEditorStateAfterFocusingElement() { m_preventScrollAfterWaitingForEditorStateAfterFocusingElement = std::nullopt; }
> +    bool waitingForEditorStateAfterFocusingElement() const { return m_preventScrollAfterWaitingForEditorStateAfterFocusingElement.has_value(); }
> +    bool preventScrollAfterWaitingForEditorStateAfterFocusingElement() const { return m_preventScrollAfterWaitingForEditorStateAfterFocusingElement == PreventScroll::Yes; }

I wish this wasn't so convoluted. Ideally the UI-side "scroll into view" logic would be closer to the web content "scroll into view" logic.

Would it be any cleaner to put the "preventScroll" bit into FocusedElementInformation? That was my original plan.
Comment 5 Wenson Hsieh 2022-02-28 15:49:13 PST
Comment on attachment 453420 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=453420&action=review

Thanks for the review!

>> Source/WebKit/UIProcess/WebPageProxy.h:1651
>> +    bool preventScrollAfterWaitingForEditorStateAfterFocusingElement() const { return m_preventScrollAfterWaitingForEditorStateAfterFocusingElement == PreventScroll::Yes; }
> 
> I wish this wasn't so convoluted. Ideally the UI-side "scroll into view" logic would be closer to the web content "scroll into view" logic.
> 
> Would it be any cleaner to put the "preventScroll" bit into FocusedElementInformation? That was my original plan.

Yes — that should work! I had thought of doing this but hesitated a bit, because FocusedElementInformation is mostly a state snapshot describing the focused element itself (rather than being about the call to `focus()`).

That said, it does make this code a lot more understandable (and elegant), so I think it's probably in the end to go with that approach (adding the flag to FEI). I'll go with that approach, and upload a new version soon!
Comment 6 Wenson Hsieh 2022-02-28 17:36:25 PST
Created attachment 453452 [details]
v2
Comment 7 Wenson Hsieh 2022-02-28 17:43:41 PST
Created attachment 453453 [details]
Add missing tests
Comment 8 Simon Fraser (smfr) 2022-02-28 18:12:22 PST
Comment on attachment 453453 [details]
Add missing tests

That seems much nicer.
Comment 9 Wenson Hsieh 2022-02-28 18:12:58 PST
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 453453 [details]
> Add missing tests
> 
> That seems much nicer.

Indeed — thank you for the suggestion!
Comment 10 EWS 2022-03-01 07:17:19 PST
Committed r290646 (247919@main): <https://commits.webkit.org/247919@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453453 [details].
Comment 11 Sam Sneddon [:gsnedders] 2022-05-16 15:37:07 PDT
Just to let everyone know, the fix for this has shipped in Safari 15.5.