From f9eb0b989f776b851cf58898f2c345c22c9b03d3 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Tue, 29 May 2018 17:41:47 -0700 Subject: [PATCH 1/2] Fixes crash in `STPColorUtils.perceivedBrightnessForColor` This is used for deciding whether to use light/dark scroll bars & status bar indicators, and choosing which color should be used for the switch in `STPSwitchTableViewCell`. This code assumed the color was always in the RGB color space, which is incorrect. It would reliably crash with Address Sanitization turned on, and give inconsistent answers with it turned off. Now using the `UIColor` `getRed:green:blue:alpha:` method to convert the color to RGB before calculating the luma value. Also added some links to documentation for why this calculation is interesting. Added tests for `STPColorUtils colorIsBright:` This also tests `perceivedBrightnessForColor:`, but does so by checking something that's more meaningful to me, rather than testing for specific luma numbers --- Stripe.xcodeproj/project.pbxproj | 4 + Stripe/STPColorUtils.m | 16 +++- Tests/Tests/STPColorUtilsTest.m | 141 +++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 Tests/Tests/STPColorUtilsTest.m diff --git a/Stripe.xcodeproj/project.pbxproj b/Stripe.xcodeproj/project.pbxproj index 22bb8d2c530..caa189a47fe 100644 --- a/Stripe.xcodeproj/project.pbxproj +++ b/Stripe.xcodeproj/project.pbxproj @@ -372,6 +372,7 @@ 8BD87B931EFB1C1E00269C2B /* STPSourceVerification+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 8BD87B911EFB1C1E00269C2B /* STPSourceVerification+Private.h */; }; 8BD87B951EFB1CB100269C2B /* STPSourceVerificationTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BD87B941EFB1CB100269C2B /* STPSourceVerificationTest.m */; }; 8BE5AE8B1EF8905B0081A33C /* STPCardParamsTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BE5AE8A1EF8905B0081A33C /* STPCardParamsTest.m */; }; + B318518320BE011700EE8C0F /* STPColorUtilsTest.m in Sources */ = {isa = PBXBuildFile; fileRef = B318518220BE011700EE8C0F /* STPColorUtilsTest.m */; }; B3302F462006FBA7005DDBE9 /* STPConnectAccountParamsTest.m in Sources */ = {isa = PBXBuildFile; fileRef = B3302F452006FBA7005DDBE9 /* STPConnectAccountParamsTest.m */; }; B3302F4C200700AB005DDBE9 /* STPLegalEntityParamsTest.m in Sources */ = {isa = PBXBuildFile; fileRef = B3302F4B200700AB005DDBE9 /* STPLegalEntityParamsTest.m */; }; B347DD481FE35423006B3BAC /* STPValidatedTextField.h in Headers */ = {isa = PBXBuildFile; fileRef = B347DD461FE35423006B3BAC /* STPValidatedTextField.h */; }; @@ -1046,6 +1047,7 @@ 8BD87B911EFB1C1E00269C2B /* STPSourceVerification+Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "STPSourceVerification+Private.h"; sourceTree = ""; }; 8BD87B941EFB1CB100269C2B /* STPSourceVerificationTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = STPSourceVerificationTest.m; sourceTree = ""; }; 8BE5AE8A1EF8905B0081A33C /* STPCardParamsTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = STPCardParamsTest.m; sourceTree = ""; }; + B318518220BE011700EE8C0F /* STPColorUtilsTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = STPColorUtilsTest.m; sourceTree = ""; }; B3302F452006FBA7005DDBE9 /* STPConnectAccountParamsTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = STPConnectAccountParamsTest.m; sourceTree = ""; }; B3302F4B200700AB005DDBE9 /* STPLegalEntityParamsTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = STPLegalEntityParamsTest.m; sourceTree = ""; }; B347DD461FE35423006B3BAC /* STPValidatedTextField.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = STPValidatedTextField.h; sourceTree = ""; }; @@ -1605,6 +1607,7 @@ 04CDB5251A5F3A9300B854EE /* STPCardTest.m */, 0438EF4A1B741B0100D506CC /* STPCardValidatorTest.m */, 04CDB5261A5F3A9300B854EE /* STPCertTest.m */, + B318518220BE011700EE8C0F /* STPColorUtilsTest.m */, B3302F452006FBA7005DDBE9 /* STPConnectAccountParamsTest.m */, C1E4F8051EBBEB0F00E611F5 /* STPCustomerContextTest.m */, F1303E1A1F90000700E670AE /* STPCustomerSourceTupleTest.m */, @@ -2760,6 +2763,7 @@ C16F66AB1CA21BAC006A21B5 /* STPFormTextFieldTest.m in Sources */, C1D23FAD1D37F81F002FD83C /* STPCustomerTest.m in Sources */, C1CFCB6E1ED5E0F800BE45DF /* STPMocks.m in Sources */, + B318518320BE011700EE8C0F /* STPColorUtilsTest.m in Sources */, 04827D181D257A6C002DB3E8 /* STPImageLibraryTest.m in Sources */, 0438EF4C1B741B0100D506CC /* STPCardValidatorTest.m in Sources */, 04A4C3921C4F263300B3B290 /* NSArray+StripeTest.m in Sources */, diff --git a/Stripe/STPColorUtils.m b/Stripe/STPColorUtils.m index 3f09273de8a..cc962087218 100644 --- a/Stripe/STPColorUtils.m +++ b/Stripe/STPColorUtils.m @@ -11,9 +11,19 @@ @implementation STPColorUtils + (CGFloat)perceivedBrightnessForColor:(UIColor *)color { - const CGFloat *component = CGColorGetComponents(color.CGColor); - CGFloat brightness = ((component[0] * 299) + (component[1] * 587) + (component[2] * 114)) / 1000; - return brightness; + CGFloat red, green, blue; + if ([color getRed:&red green:&green blue:&blue alpha:nil]) { + // We're using the luma value from YIQ + // https://en.wikipedia.org/wiki/YIQ#From_RGB_to_YIQ + // recommended by https://www.w3.org/WAI/ER/WD-AERT/#color-contrast + return red * 0.299 + green * 0.587 + blue * 0.114; + } else { + // Couldn't get RGB for this color, device couldn't convert it from whatever + // colorspace it's in. + // Make it "bright", since most of the color space is (based on our current + // formula), but not very bright. + return 0.4; + } } + (UIColor *)brighterColor:(UIColor *)color1 color2:(UIColor *)color2 { diff --git a/Tests/Tests/STPColorUtilsTest.m b/Tests/Tests/STPColorUtilsTest.m new file mode 100644 index 00000000000..cc5debece9b --- /dev/null +++ b/Tests/Tests/STPColorUtilsTest.m @@ -0,0 +1,141 @@ +// +// STPColorUtilsTest.m +// StripeiOS Tests +// +// Created by Daniel Jackson on 5/29/18. +// Copyright © 2018 Stripe, Inc. All rights reserved. +// + +@import XCTest; + +#import "STPColorUtils.h" + +@interface STPColorUtilsTest : XCTestCase +@end + +@implementation STPColorUtilsTest + +- (void)testGrayscaleColorsIsBright { + CGColorSpaceRef space = CGColorSpaceCreateDeviceGray(); + CGFloat components[2] = {0.0, 1.0}; + + // Using 0.3 as the cutoff from bright/non-bright because that's what + // the current implementation does. + + for (CGFloat white = 0.0; white < 0.3; white += 0.05) { + components[0] = white; + CGColorRef cgcolor = CGColorCreate(space, components); + UIColor *color = [UIColor colorWithCGColor:cgcolor]; + + XCTAssertFalse([STPColorUtils colorIsBright:color], @"colorWithWhite: %f", white); + CGColorRelease(cgcolor); + } + + for (CGFloat white = 0.3001; white < 2; white += 0.1) { + components[0] = white; + CGColorRef cgcolor = CGColorCreate(space, components); + UIColor *color = [UIColor colorWithCGColor:cgcolor]; + + XCTAssertTrue([STPColorUtils colorIsBright:color], @"colorWithWhite: %f", white); + CGColorRelease(cgcolor); + } + CGColorSpaceRelease(space); +} + +- (void)testBuiltinColorsIsBright { + // This is primarily to document what colors are considered bright/dark + NSArray *brightColors = @[ + [UIColor brownColor], + [UIColor cyanColor], + [UIColor darkGrayColor], + [UIColor grayColor], + [UIColor greenColor], + [UIColor lightGrayColor], + [UIColor magentaColor], + [UIColor orangeColor], + [UIColor whiteColor], + [UIColor yellowColor], + ]; + NSArray *darkColors = @[ + [UIColor blackColor], + [UIColor blueColor], + [UIColor clearColor], + [UIColor purpleColor], + [UIColor redColor], + ]; + + for (UIColor *color in brightColors) { + XCTAssertTrue([STPColorUtils colorIsBright:color], @"%@", color); + } + + for (UIColor *color in darkColors) { + XCTAssertFalse([STPColorUtils colorIsBright:color], @"%@", color); + } +} + +- (void)testAllColorSpaces { + // block to create & check brightness of color in a given color space + void (^testColorSpace)(const CFStringRef, BOOL) = ^(const CFStringRef colorSpaceName, BOOL expectedToBeBright) { + // this a bright color in almost all color spaces + CGFloat components[] = {1.0, 1.0, 1.0, 1.0, 1.0, 1.0}; + + UIColor *color = nil; + CGColorSpaceRef colorSpace = CGColorSpaceCreateWithName(colorSpaceName); + + if (colorSpace) { + CGColorRef cgcolor = CGColorCreate(colorSpace, components); + + if (cgcolor) { + color = [UIColor colorWithCGColor:cgcolor]; + } + CGColorRelease(cgcolor); + } + CGColorSpaceRelease(colorSpace); + + if (color) { + if (expectedToBeBright) { + XCTAssertTrue([STPColorUtils colorIsBright:color], @"%@", color); + } else { + XCTAssertFalse([STPColorUtils colorIsBright:color], @"%@", color); + } + } else { + XCTFail(@"Could not create color for %@", colorSpaceName); + } + }; + + CFStringRef colorSpaceNames[] = { + kCGColorSpaceSRGB, + kCGColorSpaceDCIP3, + kCGColorSpaceROMMRGB, + kCGColorSpaceITUR_709, + kCGColorSpaceDisplayP3, + kCGColorSpaceITUR_2020, + kCGColorSpaceGenericRGB, + kCGColorSpaceGenericXYZ, + kCGColorSpaceLinearGray, + kCGColorSpaceLinearSRGB, + kCGColorSpaceGenericCMYK, + kCGColorSpaceGenericGray, + kCGColorSpaceACESCGLinear, + kCGColorSpaceAdobeRGB1998, + kCGColorSpaceExtendedGray, + kCGColorSpaceExtendedSRGB, + kCGColorSpaceGenericRGBLinear, + kCGColorSpaceExtendedLinearGray, + kCGColorSpaceExtendedLinearSRGB, + kCGColorSpaceGenericGrayGamma2_2, + }; + + int colorSpaceCount = sizeof(colorSpaceNames) / sizeof(colorSpaceNames[0]); + for (int i = 0; i < colorSpaceCount; ++i) { + // CMYK is the only one where all 1's results in a dark color + testColorSpace(colorSpaceNames[i], colorSpaceNames[i] != kCGColorSpaceGenericCMYK); + } + + if (@available(iOS 11.0, *)) { + // in LAB all 1's is dark + testColorSpace(kCGColorSpaceGenericLab, NO); + } +} + +@end From baedcf9875fa94e687a98b6bbadb24ba55a5a263 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Wed, 30 May 2018 11:29:55 -0700 Subject: [PATCH 2/2] Add cast of literals to CGFloat to avoid implicit conversion errors --- Stripe/STPColorUtils.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Stripe/STPColorUtils.m b/Stripe/STPColorUtils.m index cc962087218..0070dc75f6d 100644 --- a/Stripe/STPColorUtils.m +++ b/Stripe/STPColorUtils.m @@ -16,13 +16,13 @@ + (CGFloat)perceivedBrightnessForColor:(UIColor *)color { // We're using the luma value from YIQ // https://en.wikipedia.org/wiki/YIQ#From_RGB_to_YIQ // recommended by https://www.w3.org/WAI/ER/WD-AERT/#color-contrast - return red * 0.299 + green * 0.587 + blue * 0.114; + return red * (CGFloat)0.299 + green * (CGFloat)0.587 + blue * (CGFloat)0.114; } else { // Couldn't get RGB for this color, device couldn't convert it from whatever // colorspace it's in. // Make it "bright", since most of the color space is (based on our current // formula), but not very bright. - return 0.4; + return (CGFloat)0.4; } }