Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the Color constructor where regular JavaScript arrays were not being properly handled when passed to Skia.Color(). The issue manifested in createPicture callbacks where colors using arrays (e.g., Skia.Color([0, 1, 1, 1])) caused "Value is undefined, expecting an object" errors.
Changes:
- Added support for regular JavaScript arrays in the C++ Color constructor and fromValue method
- Regular arrays are now converted to Float32Array format
- Added comprehensive test suite for Color API covering arrays, CSS color names, hex colors, and integration with paint.setColor and createPicture
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/skia/cpp/api/JsiSkColor.h | Added array handling logic in both fromValue (lines 48-56) and createCtor (lines 93-121) methods to convert regular JavaScript arrays to Float32Array |
| packages/skia/src/renderer/tests/e2e/Color.spec.tsx | Added new test file with 6 test cases covering array conversion, CSS colors, hex colors, paint.setColor integration, and createPicture scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (object.isArray(runtime)) { | ||
| auto array = object.asArray(runtime); | ||
| auto r = array.getValueAtIndex(runtime, 0).asNumber(); | ||
| auto g = array.getValueAtIndex(runtime, 1).asNumber(); | ||
| auto b = array.getValueAtIndex(runtime, 2).asNumber(); | ||
| auto a = array.getValueAtIndex(runtime, 3).asNumber(); | ||
| return SkColorSetARGB(a * 255, r * 255, g * 255, b * 255); | ||
| } |
There was a problem hiding this comment.
Missing array length validation before accessing array indices. If an array with fewer than 4 elements is passed (e.g., Skia.Color([1, 0, 0])), this will throw a JavaScript error when trying to access index 3. Consider adding a size check similar to JsiSkMatrix::getMatrix which validates array length and provides a clear error message (see packages/skia/cpp/api/JsiSkMatrix.h:30).
| if (obj.isArray(runtime)) { | ||
| auto arr = obj.getArray(runtime); | ||
| auto r = static_cast<float>(arr.getValueAtIndex(runtime, 0).asNumber()); | ||
| auto g = static_cast<float>(arr.getValueAtIndex(runtime, 1).asNumber()); | ||
| auto b = static_cast<float>(arr.getValueAtIndex(runtime, 2).asNumber()); | ||
| auto a = static_cast<float>(arr.getValueAtIndex(runtime, 3).asNumber()); | ||
|
|
||
| // Create Float32Array and populate | ||
| auto result = runtime.global() | ||
| .getPropertyAsFunction(runtime, "Float32Array") | ||
| .callAsConstructor(runtime, 4) | ||
| .getObject(runtime); | ||
| jsi::ArrayBuffer buffer = | ||
| result | ||
| .getProperty(runtime, | ||
| jsi::PropNameID::forAscii(runtime, "buffer")) | ||
| .asObject(runtime) | ||
| .getArrayBuffer(runtime); | ||
| auto bfrPtr = reinterpret_cast<float *>(buffer.data(runtime)); | ||
| bfrPtr[0] = r; | ||
| bfrPtr[1] = g; | ||
| bfrPtr[2] = b; | ||
| bfrPtr[3] = a; | ||
| return result; |
There was a problem hiding this comment.
Missing array length validation before accessing array indices. If an array with fewer than 4 elements is passed, this will throw a JavaScript error when trying to access the missing indices. Consider adding a size check similar to JsiSkMatrix::getMatrix (see packages/skia/cpp/api/JsiSkMatrix.h:30) or GPUColor (see packages/skia/cpp/rnwgpu/api/descriptors/GPUColor.h:32) which validate array length before accessing elements.
| expect(result[1]).toBeCloseTo(0.5); | ||
| expect(result[2]).toBeCloseTo(0.5); | ||
| expect(result[3]).toBeCloseTo(1); | ||
| }); |
There was a problem hiding this comment.
The test suite lacks coverage for edge cases such as arrays with incorrect lengths (fewer than 4 elements), empty arrays, or arrays with non-numeric values. Consider adding test cases like: Skia.Color([1, 0]), Skia.Color([]), and Skia.Color([1, 0, 0, "invalid"]) to ensure proper error handling.
| }); | |
| }); | |
| it("should reject arrays with insufficient length", async () => { | |
| await expect( | |
| surface.eval((Skia) => { | |
| // Fewer than 4 elements | |
| // @ts-expect-error Testing runtime validation of invalid input | |
| return Skia.Color([1, 0]); | |
| }) | |
| ).rejects.toThrow(); | |
| }); | |
| it("should reject empty array input", async () => { | |
| await expect( | |
| surface.eval((Skia) => { | |
| // Empty array | |
| // @ts-expect-error Testing runtime validation of invalid input | |
| return Skia.Color([]); | |
| }) | |
| ).rejects.toThrow(); | |
| }); | |
| it("should reject arrays containing non-numeric values", async () => { | |
| await expect( | |
| surface.eval((Skia) => { | |
| // Contains a non-numeric value | |
| // @ts-expect-error Testing runtime validation of invalid input | |
| return Skia.Color([1, 0, 0, "invalid" as any]); | |
| }) | |
| ).rejects.toThrow(); | |
| }); |
1 participant
fixes #2200