Bug 236584 – focus({preventScroll: true}) does not prevent scrolling on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Alex Moore]

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

[Radar WebKit Bug Importer]

<rdar://problem/88911184>

[Sam Sneddon [:gsnedders]]

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.

[Wenson Hsieh]

Created attachment 453420 [details]
For EWS

[Simon Fraser (smfr)]

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.

[Wenson Hsieh]

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!

[Wenson Hsieh]

Created attachment 453452 [details]
v2

[Wenson Hsieh]

Created attachment 453453 [details]
Add missing tests

[Simon Fraser (smfr)]

Comment on attachment 453453 [details]
Add missing tests

That seems much nicer.

[Wenson Hsieh]

(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!

[EWS]

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].

[Sam Sneddon [:gsnedders]]

Just to let everyone know, the fix for this has shipped in Safari 15.5.