Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes crash in STPColorUtils.perceivedBrightnessForColor #954

Merged
merged 2 commits into from
May 30, 2018

Conversation

danj-stripe
Copy link
Contributor

Summary

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.

Motivation

Fixes #951

Testing

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

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
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎓

Copy link
Contributor

@joeydong-stripe joeydong-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice work

  1. I see 0.3 gets passed around quite often. Should we consolidate a little? Maybe just add a #define BRIGHT_COLOR_THRESHOLD 0.4 to the implementation and tests. I imagine figuring out a way to consolidate further can be tricky.

  2. Aside:

I can quickly understand testBuiltinColorsIsBright but the other tests get pretty low level and I'm not sure if it'll be maintained (maybe we don't need to change them ever?).

I guess just testing that [UIColor whiteColor] is "bright" covers the original ask and everything else is a bonus.

@danj-stripe
Copy link
Contributor Author

Thanks for the review, and for the chance to write a little more about this - mostly for posterity.

There's a couple things going on. We have a pre-existing 0.3 threshold for brightness, and it shows up in the testGrayscaleColorsIsBright for loops. I'm not sure it's worth the effort to make it a constant, I don't think it's likely that we'll change that without making more changes - and if we do it should be pretty easy to find in the failing tests.

I think it's (slightly?) more likely that in the future we'll want to change our algorithm without changing the behavior with respect to grayscale, so that 30% gray is still the transition point. I think this is an updated algorithm (plus a example swift implementation). If I'm right (and I don't know how likely it that is), then having the value in the tests tied to the value in the implementation would be incorrect.

There's also a situation where we cannot convert a color to RGB (probably because it's in an exotic color space?). The 0.4 return value is somewhat based off the 0.3, but I really have no idea what the right behavior is for an unknown color, and so I'm less sure that it needs to be calculated based on the threshold - even though it currently is. Maybe it should be 0.5, since that's (I think) the mean/median value for standard RGB? If it was only for determining colorIsBright it'd be a little easier to reason about, but this is also used in [STPColorUtils brighterColor:color2:]

The testAllColorSpaces test case probably has the least value. It'll help prevent from regressing #951, and was also added so that I could verify (using the debugger/changing the code) that at least one color (1 1 1 ...) in each color space could be converted to RGB, and it wasn't hitting the else return 0.4 branch.

@danj-stripe danj-stripe merged commit 8393509 into master May 30, 2018
@danj-stripe danj-stripe deleted the danj/bugfix/951 branch May 30, 2018 21:41
jaimepark-stripe pushed a commit that referenced this pull request Apr 11, 2022
* Conform to UITextInput

* Split text storage

* Cleanup

* Cleanup

* Fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants