Force a gregorian calendar to show for credit card expiration date inputs (autocomplete='cc-exp'*)
Created attachment 353450 [details] Patch
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.
Created attachment 353464 [details] Patch
(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!
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.
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?
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
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
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.
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.
Created attachment 353725 [details] Patch
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.
<rdar://problem/42640256>
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.
Created attachment 353759 [details] Patch
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.
Created attachment 353809 [details] Patch
Created attachment 353811 [details] Patch
Created attachment 353829 [details] Patch
Created attachment 353835 [details] Patch
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).
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
Created attachment 353980 [details] Patch
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.
Created attachment 354001 [details] Patch
Created attachment 354006 [details] Patch
Created attachment 354008 [details] Patch
Created attachment 354025 [details] Patch
Created attachment 354037 [details] Patch
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)
Created attachment 354054 [details] Patch
Comment on attachment 354054 [details] Patch Clearing flags on attachment: 354054 Committed r237924: <https://trac.webkit.org/changeset/237924>
All reviewed patches have been landed. Closing bug.