Fixes STPPaymentCardTextField layout issues on small screens #1009

csabol-stripe · 2018-08-07T21:17:58Z

Summary

Adds correct formatting for Diner's Club card numbers
When compressed, STPPaymentCardTextField will now use the card number format (not just the fragment length) to decide which numbers to show, matching web behavior
Incomplete card numbers will be marked as invalid, matching web behavior
Correctly sizes card number text field when compressed so that it doesn't push other text fields off screen on small devices

Motivation

https://jira.corp.stripe.com/browse/IOS-600
#1007

Testing

Manually tested on iPhone SE 11.4 simulator in portrait and landscape

When compressed, STPPaymentCardTextField will now use the card number format (not just the fragment length) to decide which numbers to show, matching web behavior Incomplete card numbers will be marked as invalid, matching web behavior Correctly sizes card number text field when compressed so that it doesn't push other text fields off screen on small devices

csabol-stripe · 2018-08-07T21:21:27Z

Stripe/STPPaymentCardTextField.m

@@ -201,7 +202,7 @@ - (void)commonInit {
    [brandImageView addGestureRecognizer:[[UITapGestureRecognizer alloc] initWithTarget:numberField
                                                                                 action:@selector(becomeFirstResponder)]];

-    self.focusedTextFieldForLayout = @(STPCardFieldTypeNumber);
+    self.focusedTextFieldForLayout = nil;


I made this change because this causes the layout to force expiry and cvc to hidden if the screen isn't wide enough for full length number field and those fields. See line 1169, which still causes the number field to expand AND become the first responder (previously it was just expanded -- no first responder), however when the view first appears user will see all the fields, then an animation expanding the number field as it becomes first responder. Not a necessary change, but I thought it felt a lot better. Let me know your thoughts!

Sounds good to me! (I haven't played with it yet)

danj-stripe · 2018-08-08T19:41:28Z

Looks like the build is failing on the check_category_linking.rb script, and you'll need to add some stuff to STPCategoryLoader

edit: or maybe just move the implementation of the private function into STPCardValidator.m, while leaving the declaration in the private header.

danj-stripe

Looks pretty reasonable! Left some questions that I had, and a suggestion for some additional tests.

danj-stripe · 2018-08-08T19:50:23Z

Tests/Tests/NSString+StripeTest.m

+    XCTAssertEqualObjects([@"foobar" stp_stringByRemovingSuffix:nil], @"foobar");
+    XCTAssertEqualObjects([@"foobar" stp_stringByRemovingSuffix:@"foobar"], @"");
+    XCTAssertEqualObjects([@"foobar" stp_stringByRemovingSuffix:@""], @"foobar");
+    XCTAssertEqualObjects([@"foobar" stp_stringByRemovingSuffix:@"oba"], @"foobar");


It might be worth adding tests for some more complex unicode cases, if only to capture what'll happen.

I'm thinking about combined characters, the family emoji, etc. Not a comprehensive set of tests, but maybe two or three cases.

danj-stripe · 2018-08-08T20:02:09Z

Stripe/STPPaymentCardTextField.m

@@ -201,7 +202,7 @@ - (void)commonInit {
    [brandImageView addGestureRecognizer:[[UITapGestureRecognizer alloc] initWithTarget:numberField
                                                                                 action:@selector(becomeFirstResponder)]];

-    self.focusedTextFieldForLayout = @(STPCardFieldTypeNumber);
+    self.focusedTextFieldForLayout = nil;


Sounds good to me! (I haven't played with it yet)

danj-stripe · 2018-08-08T20:09:04Z

Stripe/STPCardValidator+Private.m

+        case STPCardBrandAmex:
+            return @[@4, @6, @5];
+        case STPCardBrandDinersClub:
+            return @[@4, @6, @4];


fragmentLengthForCardBrand: returns 2 for Diners club. Should it do something different? Or should this end in 2?

The length of the final grouping of digits to use when formatting a card number for display.

Should fragmentLengthForCardBrand: call this method and just return the last element of the array?

I thought it should until I saw that they were different. I'm not sure on the source for the 2 fragment length. These values are from https://git.corp.stripe.com/stripe-internal/checkout/blob/d9430b90/manhattan/src/scripts/vendor/stripe-js-v3/src/lib/fields/format.js#L19

Web dashboard shows the last 4

I also do not know the source of the 2, I think it's (probably?) desirable to update the behavior of fragmentLengthForCardBrand:

danj-stripe · 2018-08-08T20:09:46Z

Stripe/STPPaymentCardTextField.m

+    for (NSNumber *segment in cardNumberFormat) {
+        NSUInteger segmentLength = [segment unsignedIntegerValue];
+        while (segmentLength > maxCompressedString.length) {
+            [maxCompressedString appendString:@"8"];


Is this for & while simply adding max(fragmentLength, cardNumberFormat.max()) characters to the string?

I think it looks more complicated than it is, and might benefit from separating the "calculate how many characters we need" from the "add that many characters to the string"

yeah, let me see if I can simplify

danj-stripe · 2018-08-08T22:55:28Z

Stripe/STPPaymentCardTextFieldViewModel.m

@@ -20,6 +21,38 @@ - (void)setCardNumber:(NSString *)cardNumber {
    _cardNumber = [sanitizedNumber stp_safeSubstringToIndex:maxLength];
 }

+- (NSString *)compressedCardNumber {


WDYT about adding some tests for this?

csabol-stripe · 2018-08-09T19:50:58Z

@danj-stripe not sure I'm understanding the CI failure here. Do you think it's because I moved that test out of the warning suppression?

danj-stripe

As far as the CI failure, the easy ones to diagnose are the tests that failed:

STPFormTextFieldTest .testAutoFormattingBehavior_CardNumbers, ((range.length) equal to ((NSUInteger)4)) failed: ("3") is not equal to ("4")
STPPaymentCardTextFieldTest.testIntrinsicContentSize ((textField.intrinsicContentSize.width) equal to (247) +/- (0.1)) failed: ("252") is not equal to ("247") +/- ("0.1") (and two more)
STPAddCardViewControllerLocalizationTests seem to have a very slight difference in the location of the MM/YY text in the STPPaymentCardTextField, which may be related to the changed intrinsic content size?

The failing manual_installation/test.sh script was reproducible locally, but it wasn't until I disabled/deleted the piping through xcpretty that I knew what the problem was:

Undefined symbols for architecture x86_64:
  "_linkSTPCardValidatorPrivateCategory", referenced from:
      l001 in Stripe(STPCategoryLoader.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

** TEST BUILD FAILED **


The following build commands failed:
	Ld /Users/danj/Library/Developer/Xcode/DerivedData/ManualInstallationTest-ggkmyflxlhxqyzehbzsoeqmvinsa/Build/Products/Debug-iphonesimulator/ManualInstallationTest.app/ManualInstallationTest normal x86_64

I've made the same mistake, and I thought I was looking for it while reviewing but I obviously missed it.

Any new .m files for the SDK need to be added to the StripeiOS framework target (like you did), and also the StripeiOSStatic static library target:

danj-stripe · 2018-08-10T23:00:39Z

Stripe/STPCardValidator+Private.m

+        case STPCardBrandAmex:
+            return @[@4, @6, @5];
+        case STPCardBrandDinersClub:
+            return @[@4, @6, @4];


I also do not know the source of the 2, I think it's (probably?) desirable to update the behavior of fragmentLengthForCardBrand:

danj-stripe · 2018-08-10T23:06:55Z

Stripe/STPPaymentCardTextField.m

+
+    NSMutableString *maxCompressedString = [[NSMutableString alloc] initWithCapacity:maxLength];
+    for (NSUInteger i = 0; i < maxLength; ++i) {
+        [maxCompressedString appendString:@"8"];


Looks like you can do this in a one-liner with

NSString *maxCompressedString = [@"" stringByPaddingToLength:maxLength withString:@"8" startingAtIndex:0];

I had no idea that method existed, and this is totally fine with me as-is. I was curious if foundation provided something we could use and now that I've found it, wanted to share.

oh same. Cool!

…he last format segment length

csabol-stripe · 2018-08-13T21:33:58Z

Thanks for your help on the CI issues, @danj-stripe !

danj-stripe

Looks great!

danj-stripe · 2018-08-13T21:45:43Z

Stripe/STPFormTextField.m

@@ -140,7 +140,7 @@ - (void)setAutoFormattingBehavior:(STPFormTextFieldAutoFormattingBehavior)autoFo
                for (NSNumber *segmentLength in cardNumberFormat) {
                    NSUInteger segmentIndex = 0;
                    for (; index < attributedString.length && segmentIndex < [segmentLength unsignedIntegerValue]; index++, segmentIndex++) {
-                        if (segmentIndex + 1 == [segmentLength unsignedIntegerValue]) {
+                        if (index + 1 != attributedString.length && segmentIndex + 1 == [segmentLength unsignedIntegerValue]) {


Ah, nice. This fixed the failing tests for the intrinsicContentSize?

yeah and the extra space at the end of the card number

* Only call source_cancel for card payment methods * Update CHANGELOG.md * Update CHANGELOG.md

csabol-stripe requested a review from danj-stripe August 7, 2018 21:17

csabol-stripe commented Aug 7, 2018

View reviewed changes

danj-stripe reviewed Aug 8, 2018

View reviewed changes

danj-stripe assigned csabol-stripe Aug 8, 2018

csabol-stripe added 4 commits August 9, 2018 10:17

Fix category loading

46e77af

Hopefully simpler max compressed size calc

27ffb51

Adds tests for unicode control and combining characters

4e80ca0

Compressed card number tests

ee61f36

csabol-stripe assigned danj-stripe and unassigned csabol-stripe Aug 9, 2018

Merge branch 'master' into csabol/IOS-600_card_number

b262127

danj-stripe reviewed Aug 10, 2018

View reviewed changes

danj-stripe assigned csabol-stripe and unassigned danj-stripe Aug 10, 2018

csabol-stripe added 4 commits August 13, 2018 10:31

Merge branch 'master' into csabol/IOS-600_card_number

964dcb2

Some fixes from review

2924168

Fix extra space/failing test

25e7846

Update test now that fragment length for diners club is the same as t…

adaae56

…he last format segment length

csabol-stripe assigned danj-stripe and unassigned csabol-stripe Aug 13, 2018

danj-stripe approved these changes Aug 13, 2018

View reviewed changes

stripe-ci added the approved label Aug 13, 2018

stripe-ci assigned csabol-stripe and unassigned danj-stripe Aug 13, 2018

csabol-stripe merged commit 198d4ce into master Aug 13, 2018

csabol-stripe deleted the csabol/IOS-600_card_number branch August 13, 2018 21:50

davidme-stripe mentioned this pull request Sep 27, 2019

STPPaymentCardTextField shouldn't attempt to becomeFirstResponder while being dismissed #1393

Merged

csabol-stripe pushed a commit that referenced this pull request Apr 26, 2022

Only call source_cancel for 3DS card payment methods (#1009)

449828d

* Only call source_cancel for card payment methods * Update CHANGELOG.md * Update CHANGELOG.md

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes STPPaymentCardTextField layout issues on small screens #1009

Fixes STPPaymentCardTextField layout issues on small screens #1009

csabol-stripe commented Aug 7, 2018

csabol-stripe Aug 7, 2018

danj-stripe Aug 8, 2018

danj-stripe commented Aug 8, 2018 •

edited

Loading

danj-stripe left a comment

danj-stripe Aug 8, 2018

danj-stripe Aug 8, 2018

danj-stripe Aug 8, 2018

csabol-stripe Aug 9, 2018

csabol-stripe Aug 9, 2018

danj-stripe Aug 10, 2018

danj-stripe Aug 8, 2018

csabol-stripe Aug 9, 2018

danj-stripe Aug 8, 2018

csabol-stripe Aug 9, 2018

csabol-stripe commented Aug 9, 2018

danj-stripe left a comment

danj-stripe Aug 10, 2018

danj-stripe Aug 10, 2018

csabol-stripe Aug 13, 2018

csabol-stripe commented Aug 13, 2018

danj-stripe left a comment

danj-stripe Aug 13, 2018

csabol-stripe Aug 13, 2018

Fixes STPPaymentCardTextField layout issues on small screens #1009

Fixes STPPaymentCardTextField layout issues on small screens #1009

Conversation

csabol-stripe commented Aug 7, 2018

Summary

Motivation

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danj-stripe commented Aug 8, 2018 • edited Loading

danj-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabol-stripe commented Aug 9, 2018

danj-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabol-stripe commented Aug 13, 2018

danj-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danj-stripe commented Aug 8, 2018 •

edited

Loading