Bug 210230 - Popovers are dismissed immediately when they try and bring up the keyboard.
Summary: Popovers are dismissed immediately when they try and bring up the keyboard.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-08 17:32 PDT by Megan Gardner
Modified: 2020-04-09 16:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.94 KB, patch)
2020-04-08 17:36 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (4.96 KB, patch)
2020-04-09 16:17 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-04-08 17:32:02 PDT
Correctly retain focus when popovers employ keyboard
Comment 1 Megan Gardner 2020-04-08 17:36:58 PDT
Created attachment 395890 [details]
Patch
Comment 2 Megan Gardner 2020-04-08 17:37:20 PDT
<rdar://problem/60385504>
Comment 3 Darin Adler 2020-04-09 12:56:51 PDT
Comment on attachment 395890 [details]
Patch

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

Any way to make a test for this?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6669
> +- (void)preserveFocus
> +{
> +    [_webView _incrementFocusPreservationCount];
> +}
> +
> +- (void)releaseFocus
> +{
> +    [_webView _decrementFocusPreservationCount];
> +}

Is there any risk that a WKContentView could be deallocated before releaseFocus is called, or have _webView set to null or any new value before this is called?

Often, designs like this one can be dangerous if a stale count can be left behind. There are techniques to make them more resilient, like using objects and weak pointers, that are most costly but more foolproof.

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:341
> +        if (_view.focusedElementInformation.elementType == InputType::Time || _view.focusedElementInformation.elementType == InputType::DateTimeLocal)
> +            [_view releaseFocus];

Maybe we could use a boolean _preservingFocus rather than repeating this check here. That way there’s no risk of the check in controlBeginEditing being out of sync with this one.
Comment 4 Megan Gardner 2020-04-09 16:17:45 PDT
Created attachment 396022 [details]
Patch for landing
Comment 5 Darin Adler 2020-04-09 16:34:23 PDT
Comment on attachment 396022 [details]
Patch for landing

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

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:393
> +            _preservingFocus = YES;
> +            [_view preserveFocus];

Could harden this slightly by wrapping this in:

    if (!_preservingFocus) {
    }
Comment 6 EWS 2020-04-09 16:58:55 PDT
Committed r259840: <https://trac.webkit.org/changeset/259840>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396022 [details].