Skip to content

Commit

Permalink
Fixes crash in STPColorUtils.perceivedBrightnessForColor (#954)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
danj-stripe authored May 30, 2018
1 parent c2880e3 commit 8393509
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 3 deletions.
4 changes: 4 additions & 0 deletions Stripe.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -1046,6 +1047,7 @@
8BD87B911EFB1C1E00269C2B /* STPSourceVerification+Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "STPSourceVerification+Private.h"; sourceTree = "<group>"; };
8BD87B941EFB1CB100269C2B /* STPSourceVerificationTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = STPSourceVerificationTest.m; sourceTree = "<group>"; };
8BE5AE8A1EF8905B0081A33C /* STPCardParamsTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = STPCardParamsTest.m; sourceTree = "<group>"; };
B318518220BE011700EE8C0F /* STPColorUtilsTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = STPColorUtilsTest.m; sourceTree = "<group>"; };
B3302F452006FBA7005DDBE9 /* STPConnectAccountParamsTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = STPConnectAccountParamsTest.m; sourceTree = "<group>"; };
B3302F4B200700AB005DDBE9 /* STPLegalEntityParamsTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = STPLegalEntityParamsTest.m; sourceTree = "<group>"; };
B347DD461FE35423006B3BAC /* STPValidatedTextField.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = STPValidatedTextField.h; sourceTree = "<group>"; };
Expand Down Expand Up @@ -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 */,
Expand Down Expand Up @@ -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 */,
Expand Down
16 changes: 13 additions & 3 deletions Stripe/STPColorUtils.m
Original file line number Diff line number Diff line change
Expand Up @@ -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 * (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 (CGFloat)0.4;
}
}

+ (UIColor *)brighterColor:(UIColor *)color1 color2:(UIColor *)color2 {
Expand Down
141 changes: 141 additions & 0 deletions Tests/Tests/STPColorUtilsTest.m
Original file line number Diff line number Diff line change
@@ -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<UIColor *> *brightColors = @[
[UIColor brownColor],
[UIColor cyanColor],
[UIColor darkGrayColor],
[UIColor grayColor],
[UIColor greenColor],
[UIColor lightGrayColor],
[UIColor magentaColor],
[UIColor orangeColor],
[UIColor whiteColor],
[UIColor yellowColor],
];
NSArray<UIColor *> *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

0 comments on commit 8393509

Please sign in to comment.