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

Jump to bottom

Add --cssFile as <style> node to SVG #358

Merged
merged 4 commits into from
Aug 22, 2022
Merged

Add --cssFile as <style> node to SVG #358

merged 4 commits into from
Aug 22, 2022

Conversation

aloisklink
Copy link
Member

@aloisklink aloisklink commented Aug 18, 2022 •
edited
Loading

📑 Summary

If a --cssFile is given to mermaid-cli, adds it as a <svg><style> node. This means it's part of the <svg> and will get exported correctly when creating a SVG file.

See https://developer.mozilla.org/en-US/docs/Web/API/SVGStyleElement for documentation on the WebAPI on the <svg><style> element.

Fixes #55, fixes #270

I've taken content from the following PRs, so I've added the authors as Co-authors:

Example: test-output/flowchart1-with-css.svg:
flowchart1-with-css

`flowchart1-red-background.svg`

📏 Design Decisions

I also added tests for --backgroundColor and I noticed that when using -o *.pdf, the PDF background color is always white. If we want to enable PDF background colors, we need to set the puppeteer option printBackground: true, see https://pptr.dev/api/puppeteer.pdfoptions.printbackground

The added --backgroundColor tests resolves #222.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
    • They're currently quite basic and rely on manual visual inspection.
      I've only added .png/.svg uploads to Percy, I haven't added anything with .pdf
  • 🔖 targeted master branch

aloisklink and others added 3 commits August 18, 2022 18:50
@aloisklink
fix: fix page.viewport() race-condition
8828c64 
For some reason, the tests in GitHub Actions are failing about 1/2
the time. I'm not 100% sure what the issue is, but I think it's a
race condition, and page.viewport() is supposed to be `await`-ed,
so maybe it's that?

I've also re-arannged the test-code in case it's that too.
@aloisklink
test: test whether '--backgroundColor' works
ade2bde 
Adds some simple tests that check whether the backgroundColor option
works.

It looks like it doesn't work for PDFs. If we want to enable PDF
background colors, we need to set the puppeteer option
`printBackground: true`,
see https://pptr.dev/api/puppeteer.pdfoptions.printbackground
@aloisklink @joelburton
@niclas-g @MindaugasLaganeckas
fix: add css as <style> node to SVG
e5370ff 
If a --cssFile is given to mermaid-cli, adds it as a `<svg><style>`
node. This means it's part of the `<svg>` and will get exported
correctly when creating a SVG file.

See https://developer.mozilla.org/en-US/docs/Web/API/SVGStyleElement
for documentation on the WebAPI on the `<svg><style>` element.

Co-authored-by: Joel Burton <joel@joelburton.com>
Co-authored-by: Niclas Gustafsson <niclas@bitfront.se>
Co-authored-by: Mindaugas Laganeckas <Mindaugas.Laganeckas@3shape.com>
@github-actions github-actions bot added the fix label Aug 18, 2022
@aloisklink
Copy link
Member Author

Ah, the percy upload didn't work, since I'm making this PR from a fork.

Maybe it's worth testing this on a local branch first, to check if the percy upload works.

@MindaugasLaganeckas
Copy link
Member

MindaugasLaganeckas commented Aug 19, 2022 •
edited
Loading

Yes, I see the problem. 😄
openmainframeproject/feilong#367

I have granted you Write access to the repository. I hope it is ok with you and it is enough to test the PR.

@MindaugasLaganeckas
Copy link
Member

I would really like to see Percy tests to succeed before accepting the PR 😄 Thank you again! Would you consider getting more access to the mermaid-cli repository and helping out from time to time? I have sent you an invite with the Write access to the repository. You are a very skilled developer and your contributions bring a lot of value to the community. 😄

@aloisklink
Copy link
Member Author

aloisklink commented Aug 19, 2022 •
edited
Loading

Would you consider getting more access to the mermaid-cli repository and helping out from time to time? I have sent you an invite with the Write access to the repository. You are a very skilled developer and your contributions bring a lot of value to the community. smile

Sure! I'll be happy to help out :)
I can't promise that I'll have a lot of time, but I'll try to make time.

Although I'll probably still ask you for code reviews, so it won't change too much! Maybe the only thing I'll change is close GitHub Issues once they are solved, since you can always re-open them if you disagree.

I would really like to see Percy tests to succeed before accepting the PR smile Thank you again!

I've managed to get the Percy upload working by pushing to a branch on this repo. All I had to do was git push git@github.com:mermaid-js/mermaid-cli.git fix/css-style-svg.

However, it looks like the SVG to PNG conversion has gone a bit weird, see:

image

Maybe it's due to a feature missing in convert-svg-to-png, I'll have a look and see what I can find out.

Edit: It looks like graph-with-br.mmd.png (converted from .svg) is also just a black box, see https://percy.io/Mermaid/mermaid-cli/builds/20777516/unchanged/1163166556?browser=firefox&browser_ids=22%2C23&subcategories=unreviewed%2Cchanges_requested&viewLayout=overlay&viewMode=new&width=784&widths=313
image

@MindaugasLaganeckas
Copy link
Member

I will not have access to a computer this weekend. I will get back to you on Monday. A conversion tool should not stand in a way to get a feature out, but is required for the regression testing.

I am very happy, that you have decided to use some of your time to help mermaid-cli. I have no expectation at all regarding how much time. Sometimes we are busy and sometimes we feel like contributing 🙂 in the latter moments you are welcome to close an issue or two 🙂 let's keep the pr process going. Now that you are on board I will stop commiting directly to master/ 😀

@aloisklink
ci: downgrade convert-svg-to-png to v0.5.0
8c484a4 
[Version 0.6.0 of convert-svg][1] introduced a fix for
[CVE-2021-23631][2]. Unfortunately, this fix also breaks
our mermaid flowchart SVGs.

[1]: https://github.com/neocotic/convert-svg/releases/tag/0.6.0
[2]: https://nvd.nist.gov/vuln/detail/CVE-2021-23631
@aloisklink
Copy link
Member Author

aloisklink commented Aug 20, 2022 •
edited
Loading

Fixed! It looks like version 0.6.0 of convert-svg introduced a fix for
https://github.com/advisories/GHSA-jv7g-9g6q-cxvw. Unfortunately, this fix also breaks our mermaid SVGs.

I've reported it as neocotic/convert-svg#90, but I doubt it will get fixed, since it's due to a security bug.

As long as we use convert-svg-to-png@0.5.0, everything works fine!

image

I think the error is because inline styles are forbidden by many browser's Content Security policy, see https://dontkry.com/posts/code/disable-inline-styles.html, so may be caused by mermaid-js/mermaid#856

@MindaugasLaganeckas MindaugasLaganeckas merged commit 79cdadd into mermaid-js:master Aug 22, 2022
@aloisklink aloisklink deleted the fix/css-style-svg branch August 22, 2022 06:36
@aloisklink aloisklink restored the fix/css-style-svg branch August 23, 2022 02:20
@aloisklink aloisklink deleted the fix/css-style-svg branch August 24, 2022 16:35
@aloisklink aloisklink mentioned this pull request Jul 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
2 participants
@aloisklink @MindaugasLaganeckas