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.

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

fix(): svg parsing logic with use and style #10050

Conversation

gloriousjob
Copy link
Contributor

Description

style tags between a path and use tag don't seem to work properly compared to Chrome. In Chrome, when both have a style, the path has priority but merges with the use tag where they do not share styles. Previously, fabric would take the use tag's style wholesale. This PR merges the two when both exist and tests were added to verify the both case and cases where only one has a style.

Closes #9840

Copy link

codesandbox bot commented Aug 10, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@asturur
Copy link
Member

asturur commented Aug 10, 2024

Did you find this priority described somewhere or you just checked how a browser behave?

Comment on lines 76 to 79
const mergedStyles = Object.keys(styleRecord).reduce(
(a, v) => a + v + ':' + styleRecord[v] + ';',
''
);
Copy link
Member

Choose a reason for hiding this comment

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

Could this be:

Suggested change
const mergedStyles = Object.keys(styleRecord).reduce(
(a, v) => a + v + ':' + styleRecord[v] + ';',
''
);
const mergedStyles = Object.entries(styleRecord).map(entry => entry.join(':')).join(';');

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s easier to interpret than what I made. Pushed.

@gloriousjob
Copy link
Contributor Author

Did you find this priority described somewhere or you just checked how a browser behave?

I tested the browser and tried different settings. The three tests I came up with show how they acted in chrome. The SVG docs were not clear about how this case should be handled from what I saw.

@asturur asturur changed the title Issue 9840 svg parsing error with use and style fix() svg parsing logic with use and style Aug 11, 2024
@asturur asturur changed the title fix() svg parsing logic with use and style fix(): svg parsing logic with use and style Aug 11, 2024
@asturur
Copy link
Member

asturur commented Aug 11, 2024

Firefox, Safari and Chrome behave the same.
Style gets merged and priority is given to the original element.

Same for attributes.
I'll be sure to revisit our parsing logic to be sure we do same

@asturur asturur merged commit 6f891e9 into fabricjs:master Aug 11, 2024
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVG parsing error with use and style
2 participants