zathras / jovial_svg

Flutter library for robust, efficient rendering of SVG static images
BSD 3-Clause "New" or "Revised" License
115 stars 21 forks source link

Transformations mess up `currentColor` #71

Closed PrimaryFeather closed 1 year ago

PrimaryFeather commented 1 year ago

I think I found another color related problem that only pops up in specific situations.

When an element that uses currentColor is transformed, that color is no longer used. At least that's what happens in this example:

import 'package:flutter/material.dart';
import 'package:jovial_svg/jovial_svg.dart';

void main() {
  runApp(const JovialTestApp());
}

const svgStringCircle = '''
<svg version="1.1" width="50" height="50" xmlns="http://www.w3.org/2000/svg">
  <circle cx="25" cy="25" r="20" fill="currentColor" />
</svg>
''';

const svgStringGroup = '''
    <svg version="1.1" width="50" height="50" xmlns="http://www.w3.org/2000/svg">
      <g fill="currentColor" transform="translate(1 1)">
        <circle cx="25" cy="25" r="20" />
      </g>
    </svg>
    ''';

class JovialTestApp extends StatelessWidget {
  const JovialTestApp({super.key});

  @override
  Widget build(BuildContext context) => MaterialApp(
        home: Scaffold(
          body: Stack(children: [
            Row(children: [
              for (final src in [svgStringCircle, svgStringGroup])
                ScalableImageWidget(
                  si: ScalableImage.fromSvgString(src)
                      .modifyCurrentColor(Colors.amber),
                ),
            ]),
          ]),
        ),
      );
}

This code displays two circles, but one of them is wrapped in a group that contains a transform attribute. The expectation would be that the two circles have the same color; instead, the code is rendered like this:

Screenshot 2023-09-25 at 18 44 09

When I remove the transformation, the colors end up correctly.

Or is this maybe a known limitation that I overlooked in the README file? If so, sorry for the noise. πŸ˜‰ In any case: thanks a lot in advance for looking into it! πŸ™

zathras commented 1 year ago

Very good, thanks for report. I'll look into this before too long, but that might be a couple of weeks... I have a bit of a vacation coming up :-)

PrimaryFeather commented 1 year ago

No worries, this is not a pressing issue on my side. Whenever you find the time! Enjoy your vacation! 😁

zathras commented 1 year ago

Yep, it's definitely broken. It looks like I used RenderContext for something other than a context for rendering - I'm storing the RenderContext on group nodes, which certainly seems wrong. The fix should be cleaning that up, by making RenderContext just have this single responsibility.

zathras commented 1 year ago

OK, I just published it as 1.1.18-rc.2. The fix was pretty straightforward, so I have high confidence, but let me know if it works with any other images where you saw this problem. The fix could have been one line, but along the way I made this area of the code less confusing.

PrimaryFeather commented 1 year ago

That sounds great, Bill!

I literally had just one SVG file that showed the issue, and after upgrading to RC2 it now displays its colors just the way it should. Perfect! 😁

Thanks a lot for the fix! πŸ™

PrimaryFeather commented 1 year ago

Since I opened up the issue in the first place, I believe I should also close it, now that everything works just fine. ☺️