Bug 191096 – Force a gregorian calendar to show for credit card expiration date inputs (autocomplete='cc-exp'*)

• Component: New Bugs
• Status: RESOLVED FIXED

[Zamiul Haque]

Force a gregorian calendar to show for credit card expiration date inputs (autocomplete='cc-exp'*)

[Zamiul Haque]

Created attachment 353450 [details]
Patch

[Tim Horton]

Comment on attachment 353450 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:97
> +@class WKFormInputControl;

Sort this alphabetically, please.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6005
> +-(WKFormInputControl *)inputPeripheral {

WebKit style would be:

- (WKFormInputControl *)inputPeripheral
{
    ...
}

(with a space after the -, braces on their own lines, and no extraneous newlines)

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.h:36
> +@interface WKFormInputControl(WKTesting)

Please add a space before the category name:

@interface WKFormInputControl (WKTesting)

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:50
> +@interface WKDateTimePopover : WKFormRotatingAccessoryPopover<WKFormControl> {

This brace is OK where it is :P It's confusing.

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:119
> +        _datePicker.get().calendar = [NSCalendar calendarWithIdentifier: NSCalendarIdentifierGregorian];

All of these ObjC messages shouldn't have spaces after the colons in the method name.

[Zamiul Haque]

Created attachment 353464 [details]
Patch

[Zamiul Haque]

(In reply to Tim Horton from comment #2)
> Comment on attachment 353450 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353450&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:97
> > +@class WKFormInputControl;
> 
> Sort this alphabetically, please.
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6005
> > +-(WKFormInputControl *)inputPeripheral {
> 
> WebKit style would be:
> 
> - (WKFormInputControl *)inputPeripheral
> {
>     ...
> }
> 
> (with a space after the -, braces on their own lines, and no extraneous
> newlines)
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormInputControl.h:36
> > +@interface WKFormInputControl(WKTesting)
> 
> Please add a space before the category name:
> 
> @interface WKFormInputControl (WKTesting)
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:50
> > +@interface WKDateTimePopover : WKFormRotatingAccessoryPopover<WKFormControl> {
> 
> This brace is OK where it is :P It's confusing.
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:119
> > +        _datePicker.get().calendar = [NSCalendar calendarWithIdentifier: NSCalendarIdentifierGregorian];
> 
> All of these ObjC messages shouldn't have spaces after the colons in the
> method name.

Done!

[Tim Horton]

Comment on attachment 353464 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6007
> +    return (WKFormInputControl *)_inputPeripheral.get();

This is an unsafe cast. We only know that _inputPeripheral is a NSObject that conforms to the WKFormPeripheral protocol, we do *not* know that it's a WKFormInputControl.

You have two options: put a method that does whatever you need from this object onto the WKFormPeripheral protocol, and make this getter return a `NSObject <WKFormInputPeripheral>`

OR

rename this method "formInputControl", and make it return nil if _inputPeripheral is not a WKFormInputControl (and keep this cast).

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:137
> +    || nodeInfo.autofillFieldName == WebCore::AutofillFieldName::CcExp
> +    || nodeInfo.autofillFieldName == WebCore::AutofillFieldName::CcExpYear;

I think these two lines should be indented one more level (one level more than the line that started the multi-line statement).

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:176
> +    [dateFormatter setCalendar:[NSCalendar calendarWithIdentifier:NSCalendarIdentifierGregorian]];

Is the fix in two different places?

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:304
> +    return [(WKDateTimePicker *)_control.get() calendarType];

Another unsafe cast. Same story.

> LayoutTests/ChangeLog:3
> +        Created a layout test to ensure that date controls marked as credit card expiry

The first line here should be the title of the bug. This blurb should go *after* the "Reviewed by" line. See other entries in this file.

[Wenson Hsieh]

Comment on attachment 353464 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Force a gregorian calendar to show for credit card expiration date inputs

This should just be the bug title!

The description goes below the Reviewed by line.

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:52
> +    WKContentView* _view;

Nit - * on the other side.

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:55
> +- (WKDateTimePopoverViewController *) viewController;

Nit - extra space between ) and viewController

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:121
> +    calendarType = _datePicker.get().calendar.calendarIdentifier;

Nit - we usually prefix ivars with an _

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:133
> +- (BOOL)shouldPresentGregorianCalendar:(AssistedNodeInformation)nodeInfo

Nit - const AssistedNodeInformation&

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:301
> +@implementation WKFormInputControl(WKTesting)

Nit - space between Control and (

> Tools/DumpRenderTree/ios/UIScriptControllerIOS.mm:399
> +

Nit - extra newline.

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:247
> +    void simulateForeignDefaultCalendar(DOMString calendarIdentifier);

Perhaps just simulateCurrentCalendarLocale or simulateCalendarLocale? Foreign is a relative concept :P

> Tools/WebKitTestRunner/TestController.h:277
> +    static RetainPtr<NSString> foreignCalendarIdentifier;

Seems odd to make this static, but make the swizzler a private member. Does foreignCalendarIdentifier ever need to outlive calendarSwizzler?

> Tools/WebKitTestRunner/TestController.h:302
> +    std::unique_ptr<ClassMethodSwizzler> calendarSwizzler;

Nit - private members should be prefixed with m_

> LayoutTests/editing/input/ios/force-gregorian-calendar-for-credit-card-expiry.html:40
> +        // Establish an asynchronous context to make use of 'await'

I think some of these comments don't add a whole lot of value (they're "what" comments that describe what the code is doing, rather than "why" comments). In general, we try not to add "what" comments if the code already more-or-less documents itself.

In the case of this comment (Establish an asynchronous context to make use of 'await'), while it does describe why we use async-await here, this is a common pattern in a lot of these UI interaction tests, so I think we can omit it.

> LayoutTests/editing/input/ios/force-gregorian-calendar-for-credit-card-expiry.html:53
> +            // Tap the target input date form control

I think you can remove this comment.

> LayoutTests/editing/input/ios/force-gregorian-calendar-for-credit-card-expiry.html:57
> +            // Get the calendar type, stringify it and output it

I think you can remove this comment.

> LayoutTests/editing/input/ios/force-gregorian-calendar-for-credit-card-expiry.html:58
> +            [(await UIHelper.calendarType())].map(toString).map(appendOutput);

Use of `map` is a bit interesting here! I would prefer just `appendOutput(toString(await UIHelper.calendarType()))`, but it's up to you.

> LayoutTests/editing/input/ios/force-gregorian-calendar-for-credit-card-expiry.html:73
> +            // Notify test runner that test is complete

I think you can remove this comment.

> LayoutTests/platform/win/TestExpectations:3418
> +editing/input/ios [ Skip ]

I don't think you meant to change this?

[EWS Watchlist]

Comment on attachment 353464 [details]
Patch

Attachment 353464 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9791713

New failing tests:
editing/input/ios/force-gregorian-calendar-for-credit-card-expiry.html

[EWS Watchlist]

Created attachment 353475 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Wenson Hsieh]

Comment on attachment 353464 [details]
Patch

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

>> LayoutTests/platform/win/TestExpectations:3418
>> +editing/input/ios [ Skip ]
> 
> I don't think you meant to change this?

(Or rather: I think you meant to add "editing/input/ios" here instead of replacing the other test expectation).

It seems that "fast/forms/ios" would be more appropriate for this test, however, since it's more about form controls than editing.

[Wenson Hsieh]

Comment on attachment 353464 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:121
>> +    calendarType = _datePicker.get().calendar.calendarIdentifier;
> 
> Nit - we usually prefix ivars with an _

Also, I think this would be simpler if we just implemented a getter that returns `[_datePicker calendar].calendarIdentifier` instead of keeping track of it in a separate variable.

[Zamiul Haque]

Created attachment 353725 [details]
Patch

[Wenson Hsieh]

Comment on attachment 353725 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6007
> +    if ([_inputPeripheral.get() isKindOfClass:WKFormInputControl.class])

I don't think you need the .get() here.

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:83
> +

Nit - extra newline here.

> Tools/ChangeLog:4
> +        for changing the default calendar returned by the system (ie. [NSCalendar

Nit - Description goes after the Reviewed by line!

> Tools/WebKitTestRunner/TestController.cpp:99
> +    

Nit - stray whitespace change

> Tools/WebKitTestRunner/TestController.h:277
> +    RetainPtr<NSString> overriddenCalendarIdentifier;

This should be a private member prefixed with m_, with a const getter.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:227
> +    if (m_calendarSwizzler == nullptr)

Hmm, I'd think that we do actually want to replace m_calendarSwizzler here. Is there a reason why calling UIScriptController.setDefaultCalendarType multiple times in a layout test with different identifiers shouldn't be allowed?

> LayoutTests/ChangeLog:20
> +        (window.UIHelper.simulateForeignDefaultCalendar):

I think you need to update these ChangeLog entries.

> LayoutTests/fast/forms/ios/force-gregorian-calendar-for-credit-card-expiry.html:53
> +			appendOutput(toString(await UIHelper.calendarType()));

Nit - \t!

> LayoutTests/fast/forms/ios/force-gregorian-calendar-for-credit-card-expiry.html:57
> +			appendOutput(toString(await UIHelper.calendarType()));

Here too.

> LayoutTests/resources/ui-helper.js:351
> +        if (!this.isWebKit2() || !this.isIOS())

This should work on both macOS and iOS, no? If so, we shouldn't bail here when `!this.isIOS()`.

> LayoutTests/resources/ui-helper.js:367
> +        if (!this.isWebKit2() || !this.isIOS())

Ditto.

[Wenson Hsieh]

<rdar://problem/42640256>

[Wenson Hsieh]

Comment on attachment 353725 [details]
Patch

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

>> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:227
>> +    if (m_calendarSwizzler == nullptr)
> 
> Hmm, I'd think that we do actually want to replace m_calendarSwizzler here. Is there a reason why calling UIScriptController.setDefaultCalendarType multiple times in a layout test with different identifiers shouldn't be allowed?

(Ah, I see, this is just ensuring that m_calendarSwizzler exists).

For null checks, we would use !m_calendarSwizzler instead of comparing to nullptr.

[Zamiul Haque]

Created attachment 353759 [details]
Patch

[Wenson Hsieh]

Comment on attachment 353759 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormInputControl.mm:67
> +@property (nonatomic, readonly) NSString *_calendarType;

I think you could just remove this property definition, since -calendarType is only used down below, but it's up to you.

Also, the property here should just be calendarType, instead of _calendarType.

[Zamiul Haque]

Created attachment 353809 [details]
Patch

[Zamiul Haque]

Created attachment 353811 [details]
Patch

[Zamiul Haque]

Created attachment 353829 [details]
Patch

[Zamiul Haque]

Created attachment 353835 [details]
Patch

[Wenson Hsieh]

Comment on attachment 353835 [details]
Patch

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

LGTM, with a few minor comments. Tim/Andy — wk2r?

> Tools/TestRunnerShared/UIScriptContext/UIScriptController.cpp:229
> +    

Nit - stray newline change.

> Tools/WebKitTestRunner/TestController.h:45
> +OBJC_CLASS NSString;

Nit - I think we generally try to keep these sorted.

> Tools/WebKitTestRunner/TestController.h:287
> +    RetainPtr<NSString> m_overriddenCalendarIdentifier;
> +    std::unique_ptr<ClassMethodSwizzler> m_calendarSwizzler;

Nit - these should be moved to where the rest of the private members are (i.e. after the private method declarations).

[Tim Horton]

Comment on attachment 353835 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5954
> +- (WKFormInputControl *)formInputPeripheral

Doesn't this still have the wrong name? (shouldn't it be formInputControl?)

> Tools/WebKitTestRunner/TestController.h:32
> +#if PLATFORM(COCOA)

#conditionals inside the include list go down in their own paragraph.

> LayoutTests/fast/forms/ios/force-gregorian-calendar-for-credit-card-expiry.html:42
> +            // Print human readable description of test if test runner is not running

Don't really need this comment

[Zamiul Haque]

Created attachment 353980 [details]
Patch

[Wenson Hsieh]

Comment on attachment 353980 [details]
Patch

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

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:245
> +#if PLATFORM(COCOA)

It seems a bit strange to introduce platform-specific bindings here. We typically make bindings available everywhere and add implementation stubs as needed.

[Zamiul Haque]

Created attachment 354001 [details]
Patch

[Zamiul Haque]

Created attachment 354006 [details]
Patch

[Zamiul Haque]

Created attachment 354008 [details]
Patch

[Zamiul Haque]

Created attachment 354025 [details]
Patch

[Zamiul Haque]

Created attachment 354037 [details]
Patch

[Wenson Hsieh]

Comment on attachment 354037 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.h:314
> +    void platformResetStateToConsistentValues();

Oops!

> Tools/WebKitTestRunner/TestController.h:318
> +    

(Extra newline here)

[Zamiul Haque]

Created attachment 354054 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354054 [details]
Patch

Clearing flags on attachment: 354054

Committed r237924: <https://trac.webkit.org/changeset/237924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.