From 0ab8d0bf4f7c1c2afbd407fe217068befab0ef34 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Tue, 12 Dec 2017 16:05:19 -0800 Subject: [PATCH 1/2] Fix bugs around `STPPaymentCardTextField becomeFirstResponder` Previous implementation of `nextFirstResponderField` would return `nil` if all fields were valid, which blocked programatic calls to `becomeFirstResponder`. It *also* used to cycle through the fields every time it was called, which doesn't seem right. It feels more natural for the `nextFirstResponderField` to be either the first invalid field *or* the last field. This puts the insertion point at the right place to start deleting. It also feels better if the user manually changes to the expiration field before entering the card number: once the expiration is valid the firstResponder jumps back to numbers. Then, once they complete numbers, it jumps to the CVC. I also added a test for this behavior. Some of the things it verifies: * becomeFirstResponder should not change the current first responder if the current field isn't valid yet * becomeFirstResponder should take the user to the first invalid field when called * becomeFirstResponder should make the last sub-field first responder when all the fields are valid --- Stripe/STPPaymentCardTextField.m | 33 ++++-------- Tests/Tests/STPPaymentCardTextFieldTest.m | 65 +++++++++++++++++++++-- 2 files changed, 70 insertions(+), 28 deletions(-) diff --git a/Stripe/STPPaymentCardTextField.m b/Stripe/STPPaymentCardTextField.m index 74559c4cbf1..5ace2e979c2 100644 --- a/Stripe/STPPaymentCardTextField.m +++ b/Stripe/STPPaymentCardTextField.m @@ -447,7 +447,7 @@ - (void)setEnabled:(BOOL)enabled { #pragma mark UIResponder & related methods - (BOOL)isFirstResponder { - return [self.currentFirstResponderField isFirstResponder]; + return self.currentFirstResponderField != nil; } - (BOOL)canBecomeFirstResponder { @@ -458,31 +458,11 @@ - (BOOL)becomeFirstResponder { return [[self nextFirstResponderField] becomeFirstResponder]; } -- (STPFormTextField *)nextFirstResponderField { - STPFormTextField *currentSubResponder = self.currentFirstResponderField; - if (currentSubResponder) { - NSUInteger index = [self.allFields indexOfObject:currentSubResponder]; - if (index != NSNotFound) { - index += 1; - if (self.allFields.count > index) { - STPFormTextField *nextField = self.allFields[index]; - if (nextField == self.postalCodeField - && !self.postalCodeEntryEnabled) { - return [self firstInvalidSubField]; - } - else { - return nextField; - } - } - } - return [self firstInvalidSubField]; - } - else { - return [self firstInvalidSubField]; - } +- (nonnull STPFormTextField *)nextFirstResponderField { + return [self firstInvalidSubField] ?: [self lastSubField]; } -- (STPFormTextField *)firstInvalidSubField { +- (nullable STPFormTextField *)firstInvalidSubField { if ([self.viewModel validationStateForField:STPCardFieldTypeNumber] != STPCardValidationStateValid) { return self.numberField; } @@ -501,6 +481,10 @@ - (STPFormTextField *)firstInvalidSubField { } } +- (nonnull STPFormTextField *)lastSubField { + return self.postalCodeEntryEnabled ? self.postalCodeField : self.cvcField; +} + - (STPFormTextField *)currentFirstResponderField { for (STPFormTextField *textField in [self allFields]) { if ([textField isFirstResponder]) { @@ -1283,6 +1267,7 @@ - (void)formTextFieldTextDidChange:(STPFormTextField *)formTextField { } } + // This is a no-op if this is the last field [[self nextFirstResponderField] becomeFirstResponder]; break; } diff --git a/Tests/Tests/STPPaymentCardTextFieldTest.m b/Tests/Tests/STPPaymentCardTextFieldTest.m index 5d0e0a4d303..3903794e009 100644 --- a/Tests/Tests/STPPaymentCardTextFieldTest.m +++ b/Tests/Tests/STPPaymentCardTextFieldTest.m @@ -23,6 +23,7 @@ @interface STPPaymentCardTextField (Testing) @property (nonatomic, readwrite, weak) STPFormTextField *numberField; @property (nonatomic, readwrite, weak) STPFormTextField *expirationField; @property (nonatomic, readwrite, weak) STPFormTextField *cvcField; +@property (nonatomic, readwrite, weak) STPFormTextField *postalCodeField; @property (nonatomic, readonly, weak) STPFormTextField *currentFirstResponderField; @property (nonatomic, readwrite, strong) STPPaymentCardTextFieldViewModel *viewModel; @property (nonatomic, copy) NSNumber *focusedTextFieldForLayout; @@ -394,7 +395,7 @@ - (void)testSetCard_allFields_whileEditingNumber { XCTAssertEqualObjects(self.sut.numberField.text, number); XCTAssertEqualObjects(self.sut.expirationField.text, @"10/99"); XCTAssertEqualObjects(self.sut.cvcField.text, cvc); - XCTAssertFalse([self.sut isFirstResponder]); + XCTAssertFalse([self.sut isFirstResponder], @"after `setCardParams:`, if all fields are valid, should resign firstResponder"); XCTAssertTrue(self.sut.isValid); } @@ -415,7 +416,7 @@ - (void)testSetCard_partialNumberAndExpiration_whileEditingExpiration { XCTAssertEqualObjects(self.sut.numberField.text, number); XCTAssertEqualObjects(self.sut.expirationField.text, @"10/99"); XCTAssertEqual(self.sut.cvcField.text.length, (NSUInteger)0); - XCTAssertTrue([self.sut.numberField isFirstResponder]); + XCTAssertTrue([self.sut.numberField isFirstResponder], @"after `setCardParams:`, when firstResponder becomes valid, first invalid field should become firstResponder"); XCTAssertFalse(self.sut.isValid); } @@ -434,7 +435,7 @@ - (void)testSetCard_number_whileEditingCVC { XCTAssertEqualObjects(self.sut.numberField.text, number); XCTAssertEqual(self.sut.expirationField.text.length, (NSUInteger)0); XCTAssertEqual(self.sut.cvcField.text.length, (NSUInteger)0); - XCTAssertTrue([self.sut.cvcField isFirstResponder]); + XCTAssertTrue([self.sut.cvcField isFirstResponder], @"after `setCardParams:`, if firstResponder is invalid, it should remain firstResponder"); XCTAssertFalse(self.sut.isValid); } @@ -454,7 +455,7 @@ - (void)testSetCard_empty_whileEditingNumber { XCTAssertEqual(self.sut.numberField.text.length, (NSUInteger)0); XCTAssertEqual(self.sut.expirationField.text.length, (NSUInteger)0); XCTAssertEqual(self.sut.cvcField.text.length, (NSUInteger)0); - XCTAssertTrue([self.sut.numberField isFirstResponder]); + XCTAssertTrue([self.sut.numberField isFirstResponder], @"after `setCardParams:` that clears the text fields, the first invalid field should become firstResponder"); XCTAssertFalse(self.sut.isValid); } @@ -486,4 +487,60 @@ - (void)testIsValidKVO { [self waitForExpectationsWithTimeout:2 handler:nil]; } +- (void)testBecomeFirstResponder { + XCTAssertTrue([self.sut canBecomeFirstResponder]); + XCTAssertTrue([self.sut becomeFirstResponder]); + XCTAssertTrue(self.sut.isFirstResponder); + + XCTAssertEqual(self.sut.numberField, self.sut.currentFirstResponderField); + + [self.sut becomeFirstResponder]; + XCTAssertEqual(self.sut.numberField, self.sut.currentFirstResponderField, + @"Repeated calls to becomeFirstResponder should not change the firstResponder"); + + self.sut.numberField.text = @"4242" "4242" "4242" "4242"; + + XCTAssertTrue([self.sut becomeFirstResponder]); + XCTAssertEqual(self.sut.expirationField, self.sut.currentFirstResponderField, + @"Once numberField is valid, becomeFirstResponder should move to the next field (expiration)"); + + XCTAssertTrue([self.sut.cvcField becomeFirstResponder]); + XCTAssertEqual(self.sut.cvcField, self.sut.currentFirstResponderField, + @"We don't block other fields from becoming firstResponder"); + + XCTAssertTrue([self.sut becomeFirstResponder]); + XCTAssertEqual(self.sut.expirationField, self.sut.currentFirstResponderField, + @"Moves firstResponder back to expiration, because it's not valid yet"); + + self.sut.expirationField.text = @"10/99"; + self.sut.cvcField.text = @"123"; + + XCTAssertTrue(self.sut.isValid); + XCTAssertTrue([self.sut canBecomeFirstResponder]); + XCTAssertTrue([self.sut becomeFirstResponder]); + + XCTAssertEqual(self.sut.cvcField, self.sut.currentFirstResponderField, + @"When all fields are valid, the last one should be the preferred firstResponder"); + + self.sut.postalCodeEntryEnabled = YES; + XCTAssertFalse(self.sut.isValid); + + XCTAssertTrue([self.sut becomeFirstResponder]); + XCTAssertEqual(self.sut.postalCodeField, self.sut.currentFirstResponderField, + @"When postalCodeEntryEnabled=YES, it should become firstResponder after other fields are valid"); + + self.sut.expirationField.text = @""; + XCTAssertTrue([self.sut becomeFirstResponder]); + XCTAssertEqual(self.sut.expirationField, self.sut.currentFirstResponderField, + @"Moves firstResponder back to expiration, because it's not valid anymore"); + + self.sut.expirationField.text = @"10/99"; + self.sut.postalCodeField.text = @"90210"; + + XCTAssertTrue(self.sut.isValid); + XCTAssertTrue([self.sut becomeFirstResponder]); + XCTAssertEqual(self.sut.postalCodeField, self.sut.currentFirstResponderField, + @"When all fields are valid, the last one should be the preferred firstResponder"); +} + @end From 65c952bbe10f31d34377e1d6c9167d3c74bacd87 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Wed, 13 Dec 2017 14:33:04 -0800 Subject: [PATCH 2/2] Fix regression: `nextFirstResponderField` *should* prefer to go to the next field, even if others are invalid It's good to have the explicit progression through fields. Keep the previous behavior that when they're on the last field and finish filling it out, take them back to the first invalid field. Also, update `canBecomeFirstResponder` and `becomeFirstResponder` to *never* change which field is the first responder, if one of the subfields already is. If there isn't one, use either the first invalid field or the last field. Updates tests to reflect the new expected behavior. --- Stripe/STPPaymentCardTextField.m | 20 +++++++++++++++++--- Tests/Tests/STPPaymentCardTextFieldTest.m | 11 +++++++---- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/Stripe/STPPaymentCardTextField.m b/Stripe/STPPaymentCardTextField.m index 5ace2e979c2..9f4e1818d9f 100644 --- a/Stripe/STPPaymentCardTextField.m +++ b/Stripe/STPPaymentCardTextField.m @@ -10,6 +10,7 @@ #import "STPPaymentCardTextField.h" +#import "NSArray+Stripe.h" #import "NSString+Stripe.h" #import "STPFormTextField.h" #import "STPImageLibrary.h" @@ -451,14 +452,27 @@ - (BOOL)isFirstResponder { } - (BOOL)canBecomeFirstResponder { - return [[self nextFirstResponderField] canBecomeFirstResponder]; + STPFormTextField *firstResponder = [self currentFirstResponderField] ?: [self nextFirstResponderField]; + return [firstResponder canBecomeFirstResponder]; } - (BOOL)becomeFirstResponder { - return [[self nextFirstResponderField] becomeFirstResponder]; + STPFormTextField *firstResponder = [self currentFirstResponderField] ?: [self nextFirstResponderField]; + return [firstResponder becomeFirstResponder]; } - (nonnull STPFormTextField *)nextFirstResponderField { + STPFormTextField *currentFirstResponder = [self currentFirstResponderField]; + if (currentFirstResponder) { + NSUInteger index = [self.allFields indexOfObject:currentFirstResponder]; + if (index != NSNotFound) { + STPFormTextField *nextField = [self.allFields stp_boundSafeObjectAtIndex:index + 1]; + if (self.postalCodeEntryEnabled || nextField != self.postalCodeField) { + return nextField; + } + } + } + return [self firstInvalidSubField] ?: [self lastSubField]; } @@ -1267,7 +1281,7 @@ - (void)formTextFieldTextDidChange:(STPFormTextField *)formTextField { } } - // This is a no-op if this is the last field + // This is a no-op if this is the last field & they're all valid [[self nextFirstResponderField] becomeFirstResponder]; break; } diff --git a/Tests/Tests/STPPaymentCardTextFieldTest.m b/Tests/Tests/STPPaymentCardTextFieldTest.m index 3903794e009..1aa70e0b18d 100644 --- a/Tests/Tests/STPPaymentCardTextFieldTest.m +++ b/Tests/Tests/STPPaymentCardTextFieldTest.m @@ -500,22 +500,22 @@ - (void)testBecomeFirstResponder { self.sut.numberField.text = @"4242" "4242" "4242" "4242"; - XCTAssertTrue([self.sut becomeFirstResponder]); XCTAssertEqual(self.sut.expirationField, self.sut.currentFirstResponderField, - @"Once numberField is valid, becomeFirstResponder should move to the next field (expiration)"); + @"Once numberField is valid, firstResponder should move to the next field (expiration)"); XCTAssertTrue([self.sut.cvcField becomeFirstResponder]); XCTAssertEqual(self.sut.cvcField, self.sut.currentFirstResponderField, @"We don't block other fields from becoming firstResponder"); XCTAssertTrue([self.sut becomeFirstResponder]); - XCTAssertEqual(self.sut.expirationField, self.sut.currentFirstResponderField, - @"Moves firstResponder back to expiration, because it's not valid yet"); + XCTAssertEqual(self.sut.cvcField, self.sut.currentFirstResponderField, + @"Calling becomeFirstResponder does not change the currentFirstResponder"); self.sut.expirationField.text = @"10/99"; self.sut.cvcField.text = @"123"; XCTAssertTrue(self.sut.isValid); + [self.sut resignFirstResponder]; XCTAssertTrue([self.sut canBecomeFirstResponder]); XCTAssertTrue([self.sut becomeFirstResponder]); @@ -525,11 +525,13 @@ - (void)testBecomeFirstResponder { self.sut.postalCodeEntryEnabled = YES; XCTAssertFalse(self.sut.isValid); + [self.sut resignFirstResponder]; XCTAssertTrue([self.sut becomeFirstResponder]); XCTAssertEqual(self.sut.postalCodeField, self.sut.currentFirstResponderField, @"When postalCodeEntryEnabled=YES, it should become firstResponder after other fields are valid"); self.sut.expirationField.text = @""; + [self.sut resignFirstResponder]; XCTAssertTrue([self.sut becomeFirstResponder]); XCTAssertEqual(self.sut.expirationField, self.sut.currentFirstResponderField, @"Moves firstResponder back to expiration, because it's not valid anymore"); @@ -538,6 +540,7 @@ - (void)testBecomeFirstResponder { self.sut.postalCodeField.text = @"90210"; XCTAssertTrue(self.sut.isValid); + [self.sut resignFirstResponder]; XCTAssertTrue([self.sut becomeFirstResponder]); XCTAssertEqual(self.sut.postalCodeField, self.sut.currentFirstResponderField, @"When all fields are valid, the last one should be the preferred firstResponder");