Bug 199326 - Allow Clients to Add Fields to the AutoFillContext Dictionary
Summary: Allow Clients to Add Fields to the AutoFillContext Dictionary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari 12
Hardware: iPhone / iPad All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-28 10:51 PDT by Priyanka
Modified: 2019-07-24 17:45 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.82 KB, patch)
2019-06-28 11:07 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2019-06-28 16:04 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (4.95 KB, patch)
2019-06-28 16:06 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2019-07-01 17:15 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2019-07-23 18:08 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (8.82 KB, patch)
2019-07-23 22:09 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2019-07-23 22:30 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (11.36 KB, patch)
2019-07-24 13:41 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2019-07-24 14:04 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2019-07-24 14:44 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (13.29 KB, patch)
2019-07-24 16:41 PDT, Priyanka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Priyanka 2019-06-28 10:51:59 PDT
Allow Clients to Add Fields to the AutoFillContext Dictionary
Comment 1 Priyanka 2019-06-28 11:07:29 PDT
Created attachment 373133 [details]
Patch
Comment 2 Wenson Hsieh 2019-06-28 11:14:57 PDT
Comment on attachment 373133 [details]
Patch

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

Seems reasonable, with a few minor adjustments. It would also be nice to write an API test for this.

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=199326

Nit - please include a radar link below the bugzilla URL.

> Source/WebKit/UIProcess/API/Cocoa/_WKInputDelegate.h:58
> +- (NSDictionary *)_webViewAdditionalContextForStrongPasswordAssistance:(WKWebView *)webView;

This should have an availability macro (i.e. `WK_API_AVAILABLE(ios(WK_IOS_TBA))`) at the end.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:352
> +    NSDictionary *_sfAutoFillAdditionalContext;

Nit - just _autoFillAdditionalContext, or _additionalContextForAutoFill? (It’s a bit weird for SF-prefixed things to leak into WebKit)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6644
> +        return @{ @"_automaticPasswordKeyboard" : @YES, @"sfAutoFillAdditionalContext": _sfAutoFillAdditionalContext};

Nit - (same naming tweak for this key name)
Comment 3 Wenson Hsieh 2019-06-28 11:17:11 PDT
Comment on attachment 373133 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:352
>> +    NSDictionary *_sfAutoFillAdditionalContext;
> 
> Nit - just _autoFillAdditionalContext, or _additionalContextForAutoFill? (It’s a bit weird for SF-prefixed things to leak into WebKit)

This should also be a RetainPtr<NSDictionary>. We should probably clean this up too in the same places where we currently reset _focusRequiresStrongPasswordAssistance to NO.
Comment 4 Brian Weinstein 2019-06-28 11:17:38 PDT
Comment on attachment 373133 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:352
>>> +    NSDictionary *_sfAutoFillAdditionalContext;
>> 
>> Nit - just _autoFillAdditionalContext, or _additionalContextForAutoFill? (It’s a bit weird for SF-prefixed things to leak into WebKit)
> 
> This should also be a RetainPtr<NSDictionary>. We should probably clean this up too in the same places where we currently reset _focusRequiresStrongPasswordAssistance to NO.

I’d get rid of the sf prefix here, and just call this something like:

NSDictionary *_additionalContextForStrongPasswordAssistance;

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5218
> +        _sfAutoFillAdditionalContext = @{ };

Nit: Our style is @{} (no space).

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6644
>> +        return @{ @"_automaticPasswordKeyboard" : @YES, @"sfAutoFillAdditionalContext": _sfAutoFillAdditionalContext};
> 
> Nit - (same naming tweak for this key name)

I’d also get rid of the sf prefix in your string.

@“_strongPasswordAdditionalContext” seems more clear.
Comment 5 Priyanka 2019-06-28 16:04:49 PDT
Created attachment 373157 [details]
Patch
Comment 6 Priyanka 2019-06-28 16:06:26 PDT
Created attachment 373158 [details]
Patch
Comment 7 Priyanka 2019-06-28 16:07:06 PDT
Adding API Tests to the Patch.
Comment 8 mitz 2019-06-30 15:55:00 PDT
Comment on attachment 373158 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInputDelegate.h:58
> +- (NSDictionary *)_webViewAdditionalContextForStrongPasswordAssistance:(WKWebView *)webView WK_API_AVAILABLE(ios(WK_IOS_TBA));

Can we use lightweight generics here to make the key and value types in the dictionary more specific?
Comment 9 Priyanka 2019-07-01 17:15:33 PDT
Created attachment 373284 [details]
Patch
Comment 10 Priyanka 2019-07-23 18:08:00 PDT
Created attachment 374748 [details]
Patch
Comment 11 Priyanka 2019-07-23 22:09:59 PDT
Created attachment 374761 [details]
Patch
Comment 12 Priyanka 2019-07-23 22:30:29 PDT
Created attachment 374763 [details]
Patch
Comment 13 Wenson Hsieh 2019-07-24 07:42:29 PDT
Comment on attachment 374763 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:852
> +    _additionalContextForStrongPasswordAssistance = nullptr;

Nit - nil or { } instead of nullptr.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5317
> +    _additionalContextForStrongPasswordAssistance = nullptr;

(Ditto)

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:542
> +    if ([inputDelegate respondsToSelector:@selector(_webViewAdditionalContextForStrongPasswordAssistance:)])

Hmmm I might be missing something here, but shouldn’t TestInputDelegate implement -_webViewAdditionalContextForStrongPasswordAssistance: with something like:

- (NSDictionary<id, NSString *> *)_webViewAdditionalContextForStrongPasswordAssistance:(WKWebView *)webView
{
    if (_webViewAdditionalContextForStrongPasswordAssistance)
        return _webViewAdditionalContextForStrongPasswordAssistance(webView);
    return @{ };
}

…and then we would check the value of [[webView textInputContentView] _autofillContext] after focusing a password field?

You’ll probably also need to implement -_webView:_focusRequiresStrongPasswordAssistance: on TestInputDelegate, and return YES in this case.

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:547
> +    ASSERT([_additionalContextForStrongPasswordAssistance isEqualToDictionary:expected]);

This should be EXPECT_TRUE.
Comment 14 Priyanka 2019-07-24 13:41:18 PDT
Created attachment 374800 [details]
Patch
Comment 15 Wenson Hsieh 2019-07-24 14:00:50 PDT
Comment on attachment 374800 [details]
Patch

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

r=mews! (with the NSLog removed)

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:553
> +    NSLog( @"%@", [[webView textInputContentView] _autofillContext] );

Nit — it looks like you left a bit of logging here.
Comment 16 Priyanka 2019-07-24 14:04:05 PDT
Created attachment 374804 [details]
Patch
Comment 17 Wenson Hsieh 2019-07-24 14:37:19 PDT
(In reply to Priyanka from comment #16)
> Created attachment 374804 [details]
> Patch

It looks like you're missing a ChangeLog entry for Tools — you can generate this using `OpenSource/Tools/Scripts/prepare-ChangeLog Tools`.
Comment 18 Priyanka 2019-07-24 14:44:59 PDT
Created attachment 374812 [details]
Patch
Comment 19 Priyanka 2019-07-24 14:45:34 PDT
Added ChangeLog for Tools and removed the leftover logging.
Comment 20 Wenson Hsieh 2019-07-24 16:35:29 PDT
Comment on attachment 374812 [details]
Patch

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

This patch looks great. Thanks again for working on the test! Just a couple more very minor nits.

> Tools/ChangeLog:4
> +

Nit - extra newline here.

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:532
> +    NSDictionary *expected =  @{ @"strongPasswordAdditionalContext" : @"testUUID" };

Nit - extra space here between = and @
Comment 21 Priyanka 2019-07-24 16:41:38 PDT
Created attachment 374833 [details]
Patch
Comment 22 Priyanka 2019-07-24 16:42:16 PDT
Fixed spacing between the = and @, removed the new line, and also removed some tabs in the beginning of blank lines I saw.
Comment 23 Wenson Hsieh 2019-07-24 16:46:18 PDT
<rdar://problem/42816957>
Comment 24 WebKit Commit Bot 2019-07-24 17:45:04 PDT
Comment on attachment 374833 [details]
Patch

Clearing flags on attachment: 374833

Committed r247804: <https://trac.webkit.org/changeset/247804>
Comment 25 WebKit Commit Bot 2019-07-24 17:45:06 PDT
All reviewed patches have been landed.  Closing bug.