[vector_graphics_compiler] fix: handle parsing stroke-width with an invalid value #8004
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Seems reasonable, but it needs a test. There are tests that validate that an svg string parses without exception. Maybe add one with an invalid double? |
using flutter_svg: @jonahwilliams this is the example svg that got invalid double error: |
|
@jonahwilliams i have added a test |
| @@ -81,7 +77,6 @@ double? parseDoubleWithUnits( | |||
| } | |||
| final double? value = parseDouble( | |||
| rawDouble, | |||
| tryParse: tryParse, | |||
There was a problem hiding this comment.
The tryParse parameter to parseDoubleWithUnits is now ignored; we should not have parameters that don't do anything.
| @@ -172,6 +172,10 @@ void main() { | |||
| expect(parseDoubleWithUnits('1pt', theme: const SvgTheme()), 1 + 1 / 3); | |||
| }); | |||
|
|
|||
| test('"none" conversion', () { | |||
| expect(parseDoubleWithUnits('none', theme: const SvgTheme()), null); | |||
There was a problem hiding this comment.
This is testing the specific change to the parsing function, but isn't testing anything related to the original issue. It's not at all clear from the change and the test what actually happens now in the original case of the issue this PR is intended to fix. Does returning null instead of throwing work, or does it just create a different issue elsewhere in that codepath?
If the goal is to fix the case of an SVG with percentage values or none values, we should have a test that exercises that, and ensures that the higher-level behavior is as desired with this change.
There was a problem hiding this comment.
hi @stuartmorgan , what kind of test cases need to be added, do you have any suggestions?
currently I am also trying to read the documentation
There was a problem hiding this comment.
You provided an SVG that triggered this error in a comment above; a test that parsed a minimal version of that SVG and ensured that it behaved as expected seems like it would give good coverage of this use case.
| @@ -1,3 +1,7 @@ | |||
| ## 1.1.13 | |||
|
|
|||
| * Fixes an issue when parse double with 'none' value | |||
There was a problem hiding this comment.
This is missing a period; see the PR checklist link for CHANGELOG style.
| @@ -1,3 +1,7 @@ | |||
| ## 1.1.13 | |||
|
|
|||
| * Fixes an issue when parse double with 'none' value | |||
There was a problem hiding this comment.
"when parsing a double with a value of 'none'."
| return double.tryParse(rawDouble); | ||
| } | ||
| return double.parse(rawDouble); | ||
| return double.tryParse(rawDouble) ?? 0.0; |
There was a problem hiding this comment.
Why is this returning 0.0 instead of null for an invalid value?
| test('Parse SVG with "none" value', () { | ||
| final TestColorMapper mapper = TestColorMapper(); | ||
| final SvgParser parser = SvgParser( | ||
| '<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg"><rect x="100" y="10" width="80" height="80" fill="red" stroke-width="none" /></svg>', |
There was a problem hiding this comment.
Do you have an SVG spec reference documenting none as a valid numeric value? I'm not very familiar with the spec, but I didn't see anything indicating that this would be valid.
It's not clear to me that silently treating invalid values in SVG files as zero is a desirable change. Is there a standard for how SVG parsing and failure handling are supposed to work, or is it client-specific?
There was a problem hiding this comment.
What I'm reading from SVG spec here https://svgwg.org/svg2-draft/geometry.html#Sizing is that width/height are treated as CSS properties, and in CSS i believe none clears a previously established default from the cascade. Its not meaningful for SVGs parsed by this library since we don't actually support CSS, but given that folks drop SVGs from external systems in here I think its reasonable to 'support' it by being tolerant of the value.
There was a problem hiding this comment.
So should none have special handling? That would be a very different fix from what the current code in the PR is doing.
There was a problem hiding this comment.
none would clear out the old default, but there will never be any old default. So treating it as if the width/height wasn't specified. I don't think just returning null or 0 will work, as this needs to behave as if the attribute was not specified at all.
This means that the handling will need to be slightly different per property. So for stroke-width, for example, we need to instead treat none special and pretend there is no stroke properties here:
Then do something similar for fill.
My understanding is those are the properties from the specified SVG that are causing problems. Returning 0 might be correct for certain properties like width/height or incorrect for others.
| @@ -980,7 +980,6 @@ class SvgParser { | |||
| }) { | |||
| return numbers.parseDoubleWithUnits( | |||
| rawDouble, | |||
| tryParse: tryParse, | |||
There was a problem hiding this comment.
This has recreated the same problem, just one level up; the tryParse parameter is now ignored in this method.
If removing tryParse is in fact the correct behavior, it needs to be done in a comprehensive and consistent way. Any version that leaves options at other levels of the code that are silently ignored isn't going to pass review.
…flutter#8000)" This reverts commit 1d00782.
fransdhinta
changed the title
|
Hi @stuartmorgan and @jonahwilliams , apologies for the delayed response. After reading and studying the SVG documentation, I found that each property may have different types of values and default values, meaning that modifying the parseDouble implementation could lead to invalid results in many cases. stroke-width: docs Therefore, I’ve decided that this PR will focus solely on improving the implementation for parsing stroke-width, which is the specific issue I encountered in the reported problem. To address this, I have added special handling for parsing stroke-width and reverted the parseDouble implementation to its original state. I have also updated the title of this PR accordingly. However, eventually, there will need to be additional handling for percentage values, which are currently not supported by the parseDouble function, may add this in a separate PR. |
| try { | ||
| return parseDoubleWithUnits(rawWidth); | ||
| } on FormatException catch (e) { | ||
| switch (e.message) { |
There was a problem hiding this comment.
What is the purpose of this switch?
| } on FormatException catch (e) { | ||
| switch (e.message) { | ||
| case 'Invalid double': | ||
| return 1.0; |
There was a problem hiding this comment.
I was looking for an answer to this question I had earlier:
Is there a standard for how SVG parsing and failure handling are supposed to work, or is it client-specific?
and it appears that the spec says to ignore invalid values. The definition of ignored that it links to says "[the UA] treats it as if it wasn’t there at all."
Given that, I'm not clear on why there's a special wrapper returning 1.0, rather than just using tryParse: true when parsing the width.
There was a problem hiding this comment.
I'm sorry, previously I tried rendering an SVG in the browser and found that if it was given an invalid value then the result rendered was the same as stroke-width with initial value 1 as in the following docs, apparently I missed testing if the value/property was not defined,
it turns out displayed the same result, which means that if the value provided is invalid it will be ignored as stated in the docs.
so yes this special wrapper is not needed, adding tryParse: true when parsing stroke-width is enough
| @@ -1,3 +1,7 @@ | |||
| ## 1.1.16 | |||
|
|
|||
| * Fixes an issue when parsing stroke-width with an invalid value | |||
There was a problem hiding this comment.
Maybe something like "Defaults stroke-width to 1 when an invalid value is parsed instead of throwing an exception"
| @@ -1,3 +1,7 @@ | |||
| ## 1.1.16 | |||
|
|
|||
| * Fixes an issue when parsing stroke-width with an invalid value | |||
There was a problem hiding this comment.
This is missing a period; please see the CHANGELOG style guide linked from the checklist.
|
|
||
| expect(instructionsPt.paints, <Paint>[ | ||
| const Paint( | ||
| stroke: Stroke(color: Color(0xff0000ff), width: 1 + 1 / 3), |
There was a problem hiding this comment.
Where is the 1/3 magic number coming from? A named constant might be helpful here.
There was a problem hiding this comment.
from the following documentation
it is stated that 1pt is 1/72 of 1in, and the value of 1in = 96 so the formula is 1/72*96 or 96/72 or 4/3 or 1 + 1/3. I think i'll change the test to pt * 96/72
There is some issue while calling parseDouble from the original repo of this package issue link. I suggest a fix to change the return of parseDouble to always double.tryParse so that it will return
nullinstead of throwing an error if it parses an invalid StringFixes flutter/flutter#158819
Issue: link
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.