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

Integrated Katex typesetting into flowcharts #2885

Merged
merged 69 commits into from
Feb 13, 2024

Conversation

NicolasNewman
Copy link
Contributor

📑 Summary

This PR integrates the Katex math typesetter into flowcharts(v2), allowing for complex mathematical formulas to be typed inside of charts

Resolves #2776

📷 Example

graph LR
    A["$$f(\relax{x}) = \int_{-\infty}^\infty \hat{f}(\xi)\,e^{2 \pi i \xi x}\,d\xi$$"] -->|"$$\Bigg(\bigg(\Big(\big((\frac{-b\pm\sqrt{b^2-4ac}}{2a})\big)\Big)\bigg)\Bigg)$$"| B("$$1+\frac{e^{-2\pi}} {1+\frac{e^{-4\pi}} {1+\frac{e^{-6\pi}} {1+\frac{e^{-8\pi}} {1+\cdots}}}}$$")
    A -->|"$$\overbrace{a+b+c}^{\text{note}}$$"| C("$$\phase{-78^\circ}$$")
    B --> D("$$x = \begin{cases} a &\text{if } b \\ c &\text{if } d \end{cases}$$")
    C --> E("$$x(t)=c_1\begin{bmatrix}-\cos{t}+\sin{t}\\ 2\cos{t} \end{bmatrix}e^{2t}$$")

image

📏 Design Decisions

  • The implementation functions using a similar method to how icons are handled. While a node is being processed, if there is a regex match for $$.*$$, the text is passed down to the Katex renderer, which returns the results as an HTML string.
  • The dependency fontfaceobserver was added to ensure the fonts are loaded before the parser begins running. This prevents the parser from running before the fonts are loaded, resulting in the computed size of nodes being incorrect.
  • The Katex stylesheet is imported globally in mermaid.js. The Webpack configs were updated accordingly to handle it. Since this could result in the stylesheet being imported again by projects that require mermaid as a dependency, there are two additional options, each with their own pros and cons:
    • Use css-to-string-loader to keep the styles local to each chart. This would have the unfortunate side effect of still needing a global import of the fonts so they can be loaded in time by fontfaceobserver. This would also add a manual step every time the Katex dependency is updated to ensure there was no changes to the fonts that need to be reflected in the font file we create.
    • Explicitly make an option to enable Katex typesetting, and require users to import the Katex stylesheet themselves. This option won't be ideal since users would then need to handle font loading themselves.
  • If an syntax error was generated by Katex, the output created by errorRender was changed to show the nature of the syntax error.

🥅 Future Goals

  • Update the documentation to mention this feature.
  • Integrate Katex into additional charts.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted develop branch

@NicolasNewman NicolasNewman marked this pull request as draft April 4, 2022 16:28
@knsv
Copy link
Collaborator

knsv commented Apr 7, 2022

Hi Nicolas!

Well done, that looks spectacular!

I am eager to merge this PR but I am a bit concerned about the font part and the consequences in mermaid. I will need to evaluate this in more depth to make sure this does not cause issues for projects downstream.

Thanks again!

@knsv
Copy link
Collaborator

knsv commented Apr 12, 2022

FYI @NicolasNewman Reviewing this PR is not forgotten but a little delayed, due to easter time-crunch. It will happen next week.

@NicolasNewman
Copy link
Contributor Author

FYI @NicolasNewman Reviewing this PR is not forgotten but a little delayed, due to easter time-crunch. It will happen next week.

No problem. Thank you for the heads up!

@knsv
Copy link
Collaborator

knsv commented Apr 21, 2022

I have started to look at this PR and again I must say it looks great!
There are some issues though. 😢

The main problem is that the size of mermaid grows quite a lot with these changes, 330k minimized (not gzipped), this is an increase of almost 30% which is too much. We are already struggling with mermaids weight and are trying to decrease it.

Pulling out the fonts and the katex css is one thing that could be done but I suspect it will only go so far.

There is nothing wrong with your code, I find the font handling elegant but the size increase is a showstopper. (Out of curiosity, why do you need the sass-loader when you are only loading css files?)

I don't want to give up just yet though. There is an item in the roadmap to add lazy loading into mermaid. I think we should bump up the priority of lazy loading and make this lazy loaded, alternatively having a different bundle with this mermaid-katex.min.js. I like the lazy loading better though.

@NicolasNewman
Copy link
Contributor Author

The size is an unfortunate side effect of math typesetters due to the large number of characters they add. Until MathML gets implemented in every browser there's unfortunately not much that can be done to reduce Katex's size. Sadly other then Firefox, MathML is very low on vendor's backlog.

Sass-loader should be able to be removed. I'm not as familiar with Webpack so while I was troubleshooting why the CSS file wasn't getting included, I tried adding sass-loader. I noticed it was being referenced in the config but not installed as a dependency. If there's no other reason for it to be installed, I should be able to remove it. I'm assuming it should still be referenced in the config file as before?

In terms of solutions, I agree with lazy loading being the ideal approach. Depending on how long that could take, do you think it would be worth while to have a separate bundle in the meantime?

@knsv
Copy link
Collaborator

knsv commented Apr 28, 2022

yes, we could add an experimental bundle. while the regular bundles stay the same

@NicolasNewman
Copy link
Contributor Author

Is there anything I would need to do on my end to make that happen?

@jgreywolf
Copy link
Contributor

@knsv @NicolasNewman

Any update on this item?

@NicolasNewman
Copy link
Contributor Author

@knsv @NicolasNewman

Any update on this item?

MathML was very recently added into Chromium which means we don't need to rely on Katex's Stylesheet. I plan on fixing merge conflicts later this week. Few Chrome and Edge users have updated though so it will still take time until we can expect most users to be using a browser with built-in support. I go into more detail about it in #2776

Unfortunately we still won't have MathML support for Opera, Samsung Internet, UC, or IE so a discussion will need to be had on how to support potential users on those browsers.

@NicolasNewman
Copy link
Contributor Author

It's not and I can't track down what is causing it at a quick glance. The only part that stood out in the diff was line 673 of svgDraw where the result of the call to drawText was no longer stored in textElem for non-KaTeX rendering. Unfortunately changing that made no difference.

What seems strange to me is the successful test should render a simple sequence diagram and the failed test should render a sequence diagram when useMaxWidth is true (default) both take an identical input chart so I don't know why one is failing and the other isn't if the default of useMaxWidth is true for every chart.

I'll take another look over it again tomorrow in case I missed something.

@npapnet
Copy link

npapnet commented Jan 10, 2024

Any update on the uptake of this amazing feature?

* develop: (130 commits)
  Cleanup e2e.yml
  Ignore push events on merge queue
  Remove ::
  Remove ::
  Remove ::
  Debug
  Debug
  Remove spec selector
  Handle edge cases in E2E
  Always run combineArtifacts
  Fix error message
  Fix cache save
  Update cache key
  Notify users
  Flatten uploaded images
  Flatten uploaded images
  Use pixelmatch for image comparison
  Run all tests
  Fix lint
  Update lockfile
  ...
@sidharthv96
Copy link
Member

There are the failing test cases.

Sequence-diagram-auth-width-scaling-should-render-long-notes-left-of-actor diff
Sequence-diagram-auth-width-scaling-should-render-long-notes-wrapped-(inline)-left-of-actor diff
Sequence-diagram-background-rects-should-render-with-an-init-directive diff
Sequence-diagram-should-handle-different-line-breaks diff
Sequence-diagram-should-handle-line-breaks-and-wrap-annotations diff
Sequence-diagram-should-render-a-sequence-diagram-with-boxes diff
Sequence-diagram-should-render-a-simple-sequence-diagram diff
Sequence-diagram-svg-size-should-render-a-sequence-diagram-when-useMaxWidth-is-false diff
Sequence-diagram-svg-size-should-render-a-sequence-diagram-when-useMaxWidth-is-true-(default) diff

* develop: (47 commits)
  Changes to gantt.html 1. Added a Gantt diagram that demonstrates to users that hashtages and semicolons can be added to titles, sections, and task data. Changes to gantt.spec.js 1. Added unit tests to ensure that semicolons and hashtags didn't break the functionality of the gantt diagram when used in titles, sections or task data. Changes to /parser/gantt.spec.js 1. Added rendering tests to ensure that semicolons and hashtags in titles, sections, and task data didn't break the rendering of Gantt diagrams.
  Lint
  Remove echo
  RefTest
  Echo event
  Update cypress
  Fix applitools
  docs: fix lint
  docs: move community to Discord
  Swap condition blocks to avoid using negation
  Reposition const declaration to ideal place
  Change repetitive values into consts
  docs: fix swimm link
  add sequenceDiagram link e2e test
  Fix Update Browserslist
  Use pnpm/action-setup@v2
  Fix lint
  Delete docs/syntax/gantt.html
  Add more detailed docs for Gantt tasks
  Update docs
  ...
@sidharthv96
Copy link
Member

sidharthv96 commented Jan 27, 2024

The fix was minor, but it took a lot of time to figure it out. If this file had originally been typed fully, this error wouldn't have occurred at all. This is a prime example of why we should focus some more on TS conversion, and ensure new files added are fully typed.

Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

Finally, the journey has not been short but I think we are ready at last!

This is a great feature and I look forward to merging it.

@knsv knsv enabled auto-merge February 13, 2024 09:13
@knsv knsv added this pull request to the merge queue Feb 13, 2024
Merged via the queue into mermaid-js:develop with commit fe1cff3 Feb 13, 2024
19 of 20 checks passed
Copy link

mermaid-bot bot commented Feb 13, 2024

@NicolasNewman, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.

@NicolasNewman
Copy link
Contributor Author

I've recently graduated and started working so things have been very busy but I'm glad to see this finally be merged in! It was certainly quite the journey. I greatly appreciate everyone's support on making this feature a reality over the past two years.

I'll keep on eye on requests for additional support in other charts but I won't have as much time to spend on it as I once did.

@abernier
Copy link

abernier commented Feb 25, 2024

Do you know when it comes available into GitHub markdown syntax:

graph LR
    A["$$f(\relax{x}) = \int_{-\infty}^\infty \hat{f}(\xi)\,e^{2 \pi i \xi x}\,d\xi$$"] -->|"$$\Bigg(\bigg(\Big(\big((\frac{-b\pm\sqrt{b^2-4ac}}{2a})\big)\Big)\bigg)\Bigg)$$"| B("$$1+\frac{e^{-2\pi}} {1+\frac{e^{-4\pi}} {1+\frac{e^{-6\pi}} {1+\frac{e^{-8\pi}} {1+\cdots}}}}$$")
    A -->|"$$\overbrace{a+b+c}^{\text{note}}$$"| C("$$\phase{-78^\circ}$$")
    B --> D("$$x = \begin{cases} a &\text{if } b \\ c &\text{if } d \end{cases}$$")
    C --> E("$$x(t)=c_1\begin{bmatrix}-\cos{t}+\sin{t}\\ 2\cos{t} \end{bmatrix}e^{2t}$$")

current gh version seems to be:

info

@praseodym
Copy link

There hasn't been a new Mermaid release since this feature has been merged. The Katex feature has been merged on 13 February 2024, and the current latest release of Mermaid (v10.8.0) was on 2 February 2024.

Once a new Mermaid release has been made, we'll have to wait for GitHub to update its Mermaid version. Hopefully that won't be too long!

@npapnet
Copy link

npapnet commented Mar 10, 2024

Thank you for your great work. This closes a gap that existed for so long.

@abernier
Copy link

abernier commented Mar 29, 2024

ok, it was released on v10.9.0

Github is currently

info

Let's wait GitHub to update, so this can be properly rendered:

graph LR
    A["$$f(\relax{x}) = \int_{-\infty}^\infty \hat{f}(\xi)\,e^{2 \pi i \xi x}\,d\xi$$"] -->|"$$\Bigg(\bigg(\Big(\big((\frac{-b\pm\sqrt{b^2-4ac}}{2a})\big)\Big)\bigg)\Bigg)$$"| B("$$1+\frac{e^{-2\pi}} {1+\frac{e^{-4\pi}} {1+\frac{e^{-6\pi}} {1+\frac{e^{-8\pi}} {1+\cdots}}}}$$")
    A -->|"$$\overbrace{a+b+c}^{\text{note}}$$"| C("$$\phase{-78^\circ}$$")
    B --> D("$$x = \begin{cases} a &\text{if } b \\ c &\text{if } d \end{cases}$$")
    C --> E("$$x(t)=c_1\begin{bmatrix}-\cos{t}+\sin{t}\\ 2\cos{t} \end{bmatrix}e^{2t}$$")

@abernier
Copy link

abernier commented Apr 12, 2024

gh version is now 10.9.0 🙌🏻

unfortunately, katex does not render correctly. tested on mobile(ios)/desktop(macos) Chrome/Safari

image

@NicolasNewman
Copy link
Contributor Author

gh version is now 10.9.0 🙌🏻

unfortunately, katex does not render correctly. tested on mobile(ios)/desktop(macos) Chrome/Safari

That's unfortunate. I'll start looking into what's going on.

@NicolasNewman
Copy link
Contributor Author

Bug report opened: #5482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Adding LaTeX math support via Katex
10 participants