Bad shirt with tie

[rsheeter]

emoji_u1f454.svg (from Noto Emoji) doesn't convert well, seemingly because opacity is not carried over to the gradient colors when ungrouping.

Cairo, diff, Skia

Test with nanoemoji diff mode.

[anthrotype]

I believe this specific issue is with nanoemoji rather than picosvg. Picosvg is correctly inheriting the opacity for all the children shapes, but then nanoemoji is ignoring it when constructing COLRv1 Paint structs for shapes whose fill contains a gradient; it is only using the shape.opacity for the solid color paints.

I have this patch that fixes the issue, feel free to cherry pick (I don't have time right now to apply and make a test)

diff --git a/src/nanoemoji/color_glyph.py b/src/nanoemoji/color_glyph.py
index dd872d9..77605be 100644
--- a/src/nanoemoji/color_glyph.py
+++ b/src/nanoemoji/color_glyph.py
@@ -198,22 +198,22 @@ _GRADIENT_INFO = {
 }
 
 
-def _color_stop(stop_el) -> ColorStop:
+def _color_stop(stop_el, shape_opacity=1.0) -> ColorStop:
     offset = _number_or_percentage(stop_el.attrib.get("offset", "0"))
     color = Color.fromstring(stop_el.attrib.get("stop-color", "black"))
     opacity = _number_or_percentage(stop_el.attrib.get("stop-opacity", "1"))
-    color = color._replace(alpha=color.alpha * opacity)
+    color = color._replace(alpha=color.alpha * opacity * shape_opacity)
     return ColorStop(stopOffset=offset, color=color)
 
 
-def _common_gradient_parts(el):
+def _common_gradient_parts(el, shape_opacity=1.0):
     spread_method = el.attrib.get("spreadMethod", "pad").upper()
     if spread_method not in Extend.__members__:
         raise ValueError(f"Unknown spreadMethod {spread_method}")
 
     return {
         "extend": Extend.__members__[spread_method],
-        "stops": tuple(_color_stop(stop) for stop in el),
+        "stops": tuple(_color_stop(stop, shape_opacity) for stop in el),
     }
 
 
@@ -237,7 +237,7 @@ class ColorGlyph(NamedTuple):
             el = self.picosvg.resolve_url(shape.fill, "*")
 
             grad_type, grad_type_parser = _GRADIENT_INFO[etree.QName(el).localname]
-            grad_args = _common_gradient_parts(el)
+            grad_args = _common_gradient_parts(el, shape.opacity)
             try:
                 grad_args.update(
                     grad_type_parser(

[rsheeter]

Will copy into nanoemoji and add a test

[anthrotype]

the logic in picosvg _ungroup currently copies all supported attributes indiscriminately to all the children elements, whether they are shape elements or not.
That means that, e.g. if a linearGradient is contained inside a group that sets opacity=0.2 alongside with some paths that use that gradient, both the children path and the gradient element itself (used by the same path) will have an inherited opacity attribute.
However only the path's opacity will be used when rendering (together with its fill); whereas setting opacity attribute on a gradient element appears to be ignored. I haven't checked the spec, but I don't think opacity (or neither of the group attributes that we copy down to children) belongs to gradient elements such. Only stop elements can have stop-opacity (but we don't need to modify that one, since we are already adding opacity to the paths that use the gradient).

So I propose to deliberately exclude radial|linearGradient children tags when doing the ungrouping, so that picosvg doesn't add extra attributes that don't belong to gradients as such. Something like this (untested)

diff --git a/src/picosvg/svg.py b/src/picosvg/svg.py
index 5b28bc4..a905baf 100644
--- a/src/picosvg/svg.py
+++ b/src/picosvg/svg.py
@@ -385,6 +385,7 @@ class SVG:
         # Any groups left are displayed
         groups = [e for e in self.xpath(f".//svg:g", scope_el)]
         multi_clips = []
+        exclude = {"linearGradient", "radialGradient"}
         for group in groups:
             # move groups children up
             # reverse because "for each addnext" effectively reverses
@@ -394,7 +395,8 @@ class SVG:
                 group.remove(child)
                 group.addnext(child)
 
-                self._inherit_group_attrib(group, child)
+                if etree.QName(child.tag).localname not in exclude:
+                    self._inherit_group_attrib(group, child)
                 if "," in child.attrib.get("clip-path", ""):
                     multi_clips.append(child)
 

[anthrotype]

I forgot to say this also happens in the infamous necktie emoji, which has some linearGradients inside semi-transparent groups.

[rsheeter]

Merged color_glyph change w/test googlefonts/nanoemoji@46618c1. I won't close this since the issue with blind copies to wrong elements sounds valid.