Skip to content

Commit

Permalink
store borderColor in a non lossy way
Browse files Browse the repository at this point in the history
Summary:
Fix for issue #3652 -

Converting from `int` to `float` is lossy for very large numbers, so storing `borderColor` as a single `Spacing` object (which uses `float`) was not working for certain colors.

So, this pull request splits `borderColor` into alpha and RGB components, and stores each of these as their own respective `Spacing` objects.

*Test Plan*

Check out cosmith sample code here that triggers the bug: https://rnplay.org/apps/l1bw2A

What currently looks like this:

<img width="548" alt="screen shot 2016-08-13 at 6 22 28 pm" src="https://cloud.githubusercontent.com/assets/1630466/17645965/9346f05e-6183-11e6-8d40-3e458b08fd9a.png">

Should look like this (with my fix applied):

<img width="543" alt="screen shot 2016-08-13 at 6 20 08 pm" src="https://cloud.githubusercontent.com/assets/1630466/17645968/9c26d1d0-6183-11e6-8759-75a5e99f498a.png">
Closes #9380

Differential Revision: D3716707

Pulled By: foghina

fbshipit-source-id: 1164378112e2a58d43c8f5fc671c2efdb64b412b
  • Loading branch information
Mani Ghasemlou authored and Facebook Github Bot 4 committed Aug 15, 2016
1 parent 8240339 commit 8095707
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
/* package */ class ReactViewBackgroundDrawable extends Drawable {

private static final int DEFAULT_BORDER_COLOR = Color.BLACK;
private static final int DEFAULT_BORDER_RGB = 0x00FFFFFF & DEFAULT_BORDER_COLOR;
private static final int DEFAULT_BORDER_ALPHA = (0xFF000000 & DEFAULT_BORDER_COLOR) >>> 24;

private static enum BorderStyle {
SOLID,
Expand Down Expand Up @@ -73,7 +75,8 @@ private static enum BorderStyle {

/* Value at Spacing.ALL index used for rounded borders, whole array used by rectangular borders */
private @Nullable Spacing mBorderWidth;
private @Nullable Spacing mBorderColor;
private @Nullable Spacing mBorderRGB;
private @Nullable Spacing mBorderAlpha;
private @Nullable BorderStyle mBorderStyle;

/* Used for rounded border and rounded background */
Expand Down Expand Up @@ -164,16 +167,37 @@ public void setBorderWidth(int position, float width) {
}
}

public void setBorderColor(int position, float color) {
if (mBorderColor == null) {
mBorderColor = new Spacing();
mBorderColor.setDefault(Spacing.LEFT, DEFAULT_BORDER_COLOR);
mBorderColor.setDefault(Spacing.TOP, DEFAULT_BORDER_COLOR);
mBorderColor.setDefault(Spacing.RIGHT, DEFAULT_BORDER_COLOR);
mBorderColor.setDefault(Spacing.BOTTOM, DEFAULT_BORDER_COLOR);
public void setBorderColor(int position, float rgb, float alpha) {
this.setBorderRGB(position, rgb);
this.setBorderAlpha(position, alpha);
}

private void setBorderRGB(int position, float rgb) {
// set RGB component
if (mBorderRGB == null) {
mBorderRGB = new Spacing();
mBorderRGB.setDefault(Spacing.LEFT, DEFAULT_BORDER_RGB);
mBorderRGB.setDefault(Spacing.TOP, DEFAULT_BORDER_RGB);
mBorderRGB.setDefault(Spacing.RIGHT, DEFAULT_BORDER_RGB);
mBorderRGB.setDefault(Spacing.BOTTOM, DEFAULT_BORDER_RGB);
}
if (!FloatUtil.floatsEqual(mBorderColor.getRaw(position), color)) {
mBorderColor.set(position, color);
if (!FloatUtil.floatsEqual(mBorderRGB.getRaw(position), rgb)) {
mBorderRGB.set(position, rgb);
invalidateSelf();
}
}

private void setBorderAlpha(int position, float alpha) {
// set Alpha component
if (mBorderAlpha == null) {
mBorderAlpha = new Spacing();
mBorderAlpha.setDefault(Spacing.LEFT, DEFAULT_BORDER_ALPHA);
mBorderAlpha.setDefault(Spacing.TOP, DEFAULT_BORDER_ALPHA);
mBorderAlpha.setDefault(Spacing.RIGHT, DEFAULT_BORDER_ALPHA);
mBorderAlpha.setDefault(Spacing.BOTTOM, DEFAULT_BORDER_ALPHA);
}
if (!FloatUtil.floatsEqual(mBorderAlpha.getRaw(position), alpha)) {
mBorderAlpha.set(position, alpha);
invalidateSelf();
}
}
Expand Down Expand Up @@ -327,8 +351,11 @@ private float getFullBorderWidth() {
* {@link #getFullBorderWidth}.
*/
private int getFullBorderColor() {
return (mBorderColor != null && !CSSConstants.isUndefined(mBorderColor.getRaw(Spacing.ALL))) ?
(int) (long) mBorderColor.getRaw(Spacing.ALL) : DEFAULT_BORDER_COLOR;
float rgb = (mBorderRGB != null && !CSSConstants.isUndefined(mBorderRGB.getRaw(Spacing.ALL))) ?
mBorderRGB.getRaw(Spacing.ALL) : DEFAULT_BORDER_RGB;
float alpha = (mBorderAlpha != null && !CSSConstants.isUndefined(mBorderAlpha.getRaw(Spacing.ALL))) ?
mBorderAlpha.getRaw(Spacing.ALL) : DEFAULT_BORDER_ALPHA;
return ReactViewBackgroundDrawable.colorFromAlphaAndRGBComponents(alpha, rgb);
}

private void drawRectangularBackgroundWithBorders(Canvas canvas) {
Expand Down Expand Up @@ -419,8 +446,17 @@ private int getBorderWidth(int position) {
return mBorderWidth != null ? Math.round(mBorderWidth.get(position)) : 0;
}

private static int colorFromAlphaAndRGBComponents(float alpha, float rgb) {
int rgbComponent = 0x00FFFFFF & (int)rgb;
int alphaComponent = 0xFF000000 & ((int)alpha) << 24;

return rgbComponent | alphaComponent;
}

private int getBorderColor(int position) {
// Check ReactStylesDiffMap#getColorInt() to see why this is needed
return mBorderColor != null ? (int) (long) mBorderColor.get(position) : DEFAULT_BORDER_COLOR;
float rgb = mBorderRGB != null ? mBorderRGB.get(position) : DEFAULT_BORDER_RGB;
float alpha = mBorderAlpha != null ? mBorderAlpha.get(position) : DEFAULT_BORDER_ALPHA;

return ReactViewBackgroundDrawable.colorFromAlphaAndRGBComponents(alpha, rgb);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ public void setBorderWidth(int position, float width) {
getOrCreateReactViewBackground().setBorderWidth(position, width);
}

public void setBorderColor(int position, float color) {
getOrCreateReactViewBackground().setBorderColor(position, color);
public void setBorderColor(int position, float rgb, float alpha) {
getOrCreateReactViewBackground().setBorderColor(position, rgb, alpha);
}

public void setBorderRadius(float borderRadius) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ public void setBorderWidth(ReactViewGroup view, int index, float width) {
"borderColor", "borderLeftColor", "borderRightColor", "borderTopColor", "borderBottomColor"
}, customType = "Color")
public void setBorderColor(ReactViewGroup view, int index, Integer color) {
view.setBorderColor(
SPACING_TYPES[index],
color == null ? CSSConstants.UNDEFINED : (float) color);
float rgbComponent = color == null ? CSSConstants.UNDEFINED : (float) ((int)color & 0x00FFFFFF);
float alphaComponent = color == null ? CSSConstants.UNDEFINED : (float) ((int)color >>> 24);
view.setBorderColor(SPACING_TYPES[index], rgbComponent, alphaComponent);
}

@ReactProp(name = ViewProps.COLLAPSABLE)
Expand Down

0 comments on commit 8095707

Please sign in to comment.