Fix/1871 line color spill

[dwhelan]

📑 Summary

Resolves #1871 and also resolves #3267, resolves #3433, resolves #1318, which had the same root cause.

The root cause of this bug is that marker ids are not unique. So, a marker url in a path element will select the first marker on the page.

📏 Design Decisions

The main decision was to create unique ids for markers by prefixing the marker name with the id of its parent SVG element. To make this consistent across drawings a markers.ts was added directly under src. The appendMarker(elem, name) function is used to append a marker with a unique id and the markerUrl(elem, name) is used to reference the marker for a path. I updated all diagrams to use these new functions.

I wanted to write unit tests but found it difficult when d3 and dagre-d3 were mocked out. So I removed all mocks and updated vitest setup to require these as actual modules. I believe this is important as it allows us to now unit test against the DOM with d3. and dagre-d3.

I am unsure of the purpose of arrowMarkerAbsolute. If it was introduced to avoid marker id conflicts, then perhaps it could be removed.

I added demo pages (demos/1871-*.html) to allow you to check out the fixes on diagrams. You can toggle the source
via html comments to see the changes vs v9.1.7:

    <script src="./mermaid.js"></script>
    <!-- <script src="//cdn.jsdelivr.net/npm/mermaid/dist/mermaid.js"></script> -->

I intend to delete the pages as part of this PR.

📋 Tasks

Make sure you

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

[knsv]

Thank, this looks solid. I will go through it properley later this week!

[knsv]

First of all! I had almost given up on that issue, (1871), so your PR is a gift, thank you! There are many ways of using Mermaid and many diagrams so it can be hard to notice issues. Fortunately we have our regression tests which tests the diagram by performing image comparisons.

That stops many issues from slipping though and it found some things from your PR:

• The arrow heads on sequence diagrams are missing (that was a massive cause of failed tests) :)
• er symbols also had a symbol go missing:

If you want to tests this locally you can do that:

• Start with generating a baseline by checking out develop
• Start the dev server
• Run: cypress run (or pnpm run cypress). You can actually do it all in on by running pnpm e2e but when testing myself I stick to som manual tests.
  Once the baseline is in place switch to you branch, do you changes and run the e2e again if things have changed the cypress tests will signal errors.

I also suggest you merge in develop first as we have some e2e fixes in there since after your PR.

[dwhelan]

Hey thanks for the great feedback @knsv!

I will definitely try all the things you mentioned. I didn't catch these errors locally because I could not get a clean e2e run with a fresh checkout of develop (iMac M1, macOS 12.4). I will merge in the latest develop branch and work on getting the e2e tests to pass.

[jgreywolf]

@dwhelan Are you still working on this PR?

[nirname]

This PR appears to be abandoned, and the only unresolved issue at the time of writing this comment is #3433. There hasn't been much activity for a while, and I believe the number of conflicts will prevent anyone from continuing. I think this PR has served its purpose as a valuable reference, so we will close it after some time unless someone is willing to revive it.