Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vector_graphics_compiler] fix: handle parsing stroke-width with an invalid value #8004

Merged
merged 20 commits into from
Nov 25, 2024
Merged

[vector_graphics_compiler] fix: handle parsing stroke-width with an invalid value #8004

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
878304b
fix: remove unnecessary tryParse attribute on parseDouble
fransdhinta Nov 2, 2024
a14a296
feat: add test, adjust changelog.md and increase version
fransdhinta Nov 3, 2024
27c5129
fix: remove unused attribute
fransdhinta Nov 5, 2024
d23b800
feat: add test
fransdhinta Nov 5, 2024
75aba47
fix: parsers_test
fransdhinta Nov 5, 2024
8d754b7
Revert "Roll Flutter from 0fe615343bd9 to f86b77721524 (16 revisions)…
fransdhinta Nov 9, 2024
94f6168
revert parseDouble and parseDoubleWithUnits implementation
fransdhinta Nov 9, 2024
71830ee
fix: handle stroke-width parsing
fransdhinta Nov 9, 2024
6fbe33b
update CHANGELOG.md
fransdhinta Nov 9, 2024
7cd5021
fix: null check error
fransdhinta Nov 9, 2024
4aa435c
fix: code format
fransdhinta Nov 9, 2024
5783843
Merge branch 'main' into fix-parseDouble
fransdhinta Nov 10, 2024
a69d531
update CHANGELOG.md and version
fransdhinta Nov 10, 2024
e103432
fix: handle stroke-width parsing
fransdhinta Nov 10, 2024
724c7b9
fix: code format
fransdhinta Nov 11, 2024
49a8855
Merge branch 'main' into fix-parseDouble
fransdhinta Nov 25, 2024
66d11b4
fix: parsers_test
fransdhinta Nov 25, 2024
8a1440f
fix: CHANGELOG style
fransdhinta Nov 25, 2024
7674b9b
fix: code format
fransdhinta Nov 25, 2024
141c99a
Fix CHANGELOG style
stuartmorgan Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/vector_graphics_compiler/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.1.16

* Fixes an issue when parsing stroke-width with an invalid value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like "Defaults stroke-width to 1 when an invalid value is parsed instead of throwing an exception"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will adjust this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a period; please see the CHANGELOG style guide linked from the checklist.


## 1.1.15

* Fixes a bug where empty tags caused the parser to crash.
Expand Down
15 changes: 14 additions & 1 deletion packages/vector_graphics_compiler/lib/src/svg/parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,19 @@ class SvgParser {
}
}

double? _parseWidth(String? rawWidth) {
try {
return parseDoubleWithUnits(rawWidth);
} on FormatException catch (e) {
switch (e.message) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this switch?

case 'Invalid double':
return 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

default:
return 1.0;
}
}
}

List<double>? _parseDashArray(String? rawDashArray) {
if (rawDashArray == null || rawDashArray == '') {
return null;
Expand Down Expand Up @@ -1587,7 +1600,7 @@ class SvgParser {
cap: _parseCap(rawStrokeCap, null),
join: _parseJoin(rawLineJoin, null),
miterLimit: parseDouble(rawMiterLimit),
width: parseDoubleWithUnits(rawStrokeWidth),
width: _parseWidth(rawStrokeWidth),
dashArray: _parseDashArray(rawStrokeDashArray),
dashOffset: _parseDashOffset(rawStrokeDashOffset),
hasPattern: hasPattern,
Expand Down
2 changes: 1 addition & 1 deletion packages/vector_graphics_compiler/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: vector_graphics_compiler
description: A compiler to convert SVGs to the binary format used by `package:vector_graphics`.
repository: https://github.com/flutter/packages/tree/main/packages/vector_graphics_compiler
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+vector_graphics%22
version: 1.1.15
version: 1.1.16

executables:
vector_graphics_compiler:
Expand Down
79 changes: 79 additions & 0 deletions packages/vector_graphics_compiler/test/parser_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,85 @@ void main() {
]);
});

test('stroke-width with invalid value', () {
const String svg =
'<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path d="M100 10 H180 V90 H100 Z" fill="#ff0000" stroke="#0000ff" stroke-width="invalid"/></svg>';

final VectorInstructions instructions = parseWithoutOptimizers(svg);

expect(instructions.paints, const <Paint>[
Paint(
stroke: Stroke(color: Color(0xff0000ff), width: 1.0),
fill: Fill(color: Color(0xffff0000))),
]);

expect(instructions.paths, <Path>[
Path(
commands: const <PathCommand>[
MoveToCommand(100.0, 10.0),
LineToCommand(180.0, 10.0),
LineToCommand(180.0, 90.0),
LineToCommand(100.0, 90.0),
CloseCommand(),
],
),
]);
});

test('stroke-width with unit value', () {
const SvgTheme theme = SvgTheme();

const String svg_px =
'<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path d="M100 10 H180 V90 H100 Z" fill="#ff0000" stroke="#0000ff" stroke-width="1px"/></svg>';
const String svg_pt =
'<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path d="M100 10 H180 V90 H100 Z" fill="#ff0000" stroke="#0000ff" stroke-width="1pt"/></svg>';
const String svg_ex =
'<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path d="M100 10 H180 V90 H100 Z" fill="#ff0000" stroke="#0000ff" stroke-width="1ex"/></svg>';
const String svg_em =
'<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path d="M100 10 H180 V90 H100 Z" fill="#ff0000" stroke="#0000ff" stroke-width="1em"/></svg>';
const String svg_rem =
'<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"><path d="M100 10 H180 V90 H100 Z" fill="#ff0000" stroke="#0000ff" stroke-width="1rem"/></svg>';

final VectorInstructions instructionsPx = parseWithoutOptimizers(svg_px);
final VectorInstructions instructionsPt = parseWithoutOptimizers(svg_pt);
final VectorInstructions instructionsEx = parseWithoutOptimizers(svg_ex);
final VectorInstructions instructionsEm = parseWithoutOptimizers(svg_em);
final VectorInstructions instructionsRem = parseWithoutOptimizers(svg_rem);

expect(instructionsPx.paints, <Paint>[
const Paint(
stroke: Stroke(color: Color(0xff0000ff), width: 1.0),
fill: Fill(color: Color(0xffff0000))),
]);

expect(instructionsPt.paints, <Paint>[
const Paint(
stroke: Stroke(color: Color(0xff0000ff), width: 1 + 1 / 3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the 1/3 magic number coming from? A named constant might be helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fill: Fill(color: Color(0xffff0000))),
]);

expect(instructionsEx.paints, <Paint>[
Paint(
stroke: Stroke(
color: const Color(0xff0000ff), width: 1.0 * theme.xHeight),
fill: const Fill(color: Color(0xffff0000))),
]);

expect(instructionsEm.paints, <Paint>[
Paint(
stroke: Stroke(
color: const Color(0xff0000ff), width: 1.0 * theme.fontSize),
fill: const Fill(color: Color(0xffff0000))),
]);

expect(instructionsRem.paints, <Paint>[
Paint(
stroke: Stroke(
color: const Color(0xff0000ff), width: 1.0 * theme.fontSize),
fill: const Fill(color: Color(0xffff0000))),
]);
});

test('Dashed path', () {
final VectorInstructions instructions = parseWithoutOptimizers(
'''
Expand Down