Fix opacity bugs (#1183)

package/cpp/rnskia/dom/base/DrawingContext.cpp

@@ -179,8 +179,12 @@ double DrawingContext::getOpacity() {
  Sets the opacity value
  */
 void DrawingContext::setOpacity(double opacity) {
-  getMutablePaint()->setAlphaf(_opacity);
-  _opacity = opacity;
+  auto currentOpacity = opacity;
+  if (_parent != nullptr) {
+    currentOpacity *= _parent->getOpacity();
+  }
+  getMutablePaint()->setAlphaf(currentOpacity);
+  _opacity = currentOpacity;
 }
 
 /**

package/cpp/rnskia/dom/props/PaintProps.h

@@ -93,8 +93,7 @@ class PaintProps : public BaseDerivedProp {
     // Opacity
     if (_opacity->isChanged() || context->isChanged()) {
       if (_opacity->isSet()) {
-        context->setOpacity(context->getOpacity() *
-                            _opacity->value().getAsNumber());
+        context->setOpacity(_opacity->value().getAsNumber());
       } else {
         context->clearOpacity();
       }

package/src/__tests__/setup.ts

@@ -35,13 +35,27 @@ export const processResult = (
   ckSurface.getCanvas().clear(Float32Array.of(0, 0, 0, 0));
 };
 
+interface CheckImageOptions {
+  maxPixelDiff?: number;
+  threshold?: number;
+  overwrite?: boolean;
+  mute?: boolean;
+}
+
+const defaultCheckImageOptions = {
+  maxPixelDiff: 0,
+  threshold: 0.1,
+  overwrite: false,
+  mute: false,
+};
+
 export const checkImage = (
   image: SkImage,
   relPath: string,
-  overwrite = false,
-  mute = false,
-  threshold = 0.1
+  opts?: CheckImageOptions
 ) => {
+  const options = { ...defaultCheckImageOptions, ...opts };
+  const { overwrite, threshold, mute, maxPixelDiff } = options;
   const png = image.encodeToBytes();
   const p = path.resolve(__dirname, relPath);
   if (fs.existsSync(p) && !overwrite) {
@@ -61,11 +75,11 @@ export const checkImage = (
       { threshold }
     );
     if (!mute) {
-      if (diffPixelsCount !== 0) {
+      if (diffPixelsCount > maxPixelDiff) {
         fs.writeFileSync(`${p}.test.png`, PNG.sync.write(toTest));
-        // fs.writeFileSync(`${p}-diff-test.png`, PNG.sync.write(diffImage));
+        fs.writeFileSync(`${p}-diff-test.png`, PNG.sync.write(diffImage));
       }
-      expect(diffPixelsCount).toBe(0);
+      expect(diffPixelsCount).toBeLessThanOrEqual(maxPixelDiff);
     }
     return diffPixelsCount;
   } else {

package/src/dom/nodes/RenderNode.ts

@@ -208,27 +208,19 @@ export abstract class JsiRenderNode<P extends GroupProps>
     const { invertClip, layer, matrix, transform } = this.props;
     const { canvas } = parentCtx;
 
-    const opacity =
-      this.props.opacity !== undefined
-        ? parentCtx.paint.getAlphaf() * this.props.opacity
-        : parentCtx.paint.getAlphaf();
-
     if (
       this.paintCache === null ||
       this.paintCache.parent !== parentCtx.paint
     ) {
       const paintCtx = this.getPaintCtx();
-      if (paintCtx) {
-        paintCtx.opacity = opacity;
-      }
       const child = paintCtx
-        ? concatPaint(parentCtx.paint, paintCtx)
+        ? concatPaint(parentCtx.paint.copy(), paintCtx)
         : parentCtx.paint;
       this.paintCache = { parent: parentCtx.paint, child };
     }
     const paint = this.paintCache.child;
     // TODO: can we only recreate a new context here if needed?
-    const ctx = { ...parentCtx, opacity, paint };
+    const ctx = { ...parentCtx, paint };
     const hasTransform = matrix !== undefined || transform !== undefined;
     const hasClip =
       this.clipRect !== undefined ||
@@ -270,7 +262,7 @@ export abstract class JsiRenderNode<P extends GroupProps>
 }
 
 const concatPaint = (
-  parent: SkPaint,
+  paint: SkPaint,
   {
     color,
     strokeWidth,
@@ -288,10 +280,14 @@ const concatPaint = (
     style,
   }: PaintContext
 ) => {
-  const paint = parent.copy();
+  if (opacity !== undefined) {
+    paint.setAlphaf(paint.getAlphaf() * opacity);
+  }
   if (color !== undefined) {
+    const currentOpacity = paint.getAlphaf();
     paint.setShader(null);
     paint.setColor(color);
+    paint.setAlphaf(currentOpacity * paint.getAlphaf());
   }
   if (strokeWidth !== undefined) {
     paint.setStrokeWidth(strokeWidth);
@@ -329,6 +325,5 @@ const concatPaint = (
   if (style !== undefined) {
     paint.setStyle(style);
   }
-  paint.setAlphaf(paint.getAlphaf() * (opacity ?? 1));
   return paint;
 };

package/src/renderer/__tests__/e2e/Drawings.spec.tsx

@@ -50,7 +50,9 @@ describe("Drawings", () => {
         <Circle cx={width / 2} cy={height - r} r={r} color="yellow" />
       </Group>
     );
-    const diff = checkImage(image, "snapshots/drawings/blur.png", false, true);
+    const diff = checkImage(image, "snapshots/drawings/blur.png", {
+      mute: true,
+    });
     expect(diff).not.toBe(0);
   });
 

package/src/renderer/__tests__/e2e/Opacity.spec.tsx

@@ -20,6 +20,39 @@ import { setupSkia } from "../../../skia/__tests__/setup";
 import { fitRects } from "../../../dom/nodes/datatypes/Fitting";
 
 describe("Opacity", () => {
+  it("Build reference result", async () => {
+    const image = await surface.draw(
+      <>
+        <Fill color="white" />
+        <Fill color="rgba(0, 0, 255, 0.25)" />
+      </>
+    );
+    checkImage(image, "snapshots/drawings/violet.png");
+  });
+  it("Should multiply multiply the color opacity (1)", async () => {
+    const image = await surface.draw(
+      <>
+        <Fill color="white" />
+        <Group opacity={0.5}>
+          <Fill color="rgba(0, 0, 255, 0.5)" />
+        </Group>
+      </>
+    );
+    checkImage(image, "snapshots/drawings/violet.png");
+  });
+  it("Should multiply multiply the color opacity (2)", async () => {
+    const image = await surface.draw(
+      <>
+        <Fill color="white" />
+        <Group opacity={0.5}>
+          <Group opacity={0.5}>
+            <Fill color="blue" />
+          </Group>
+        </Group>
+      </>
+    );
+    checkImage(image, "snapshots/drawings/violet.png");
+  });
   it("Should multiply the opacity to 0", async () => {
     const { rect, rrect } = importSkia();
     const { width } = surface;
@@ -134,7 +167,7 @@ describe("Opacity", () => {
     );
     checkImage(image, "snapshots/drawings/opacity-multiplication2.png");
   });
-  it("Build reference result", () => {
+  it("Build reference result (2)", () => {
     const {
       surface: ckSurface,
       Skia,
@@ -217,7 +250,9 @@ describe("Opacity", () => {
         </Fill>
       </Group>
     );
-    checkImage(img, "snapshots/drawings/opacity-image.png", false, false, 0.2);
+    checkImage(img, "snapshots/drawings/opacity-image.png", {
+      maxPixelDiff: 8,
+    });
   });
   it("Should apply opacity to an image shader (2)", async () => {
     const { oslo } = images;
@@ -240,6 +275,8 @@ describe("Opacity", () => {
         </Group>
       </Group>
     );
-    checkImage(img, "snapshots/drawings/opacity-image.png", false, false, 0.2);
+    checkImage(img, "snapshots/drawings/opacity-image.png", {
+      maxPixelDiff: 8,
+    });
   });
 });