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

Usage of duplicate IDs across diagrams has potential to break some diagrams #1318

Closed
facelessuser opened this issue Mar 26, 2020 · 17 comments
Closed

Comments

@facelessuser
Copy link

Describe the bug
Class and State Diagrams all share <def> elements with the same id instead of making them unique. If given two diagrams that share <def> elements with the same id, and the first gets hidden, it can affect all other diagrams that share the same <def> ids.

In this example, notice that when the Class Diagram is hidden, the State Diagram underneath loses its arrows.

wDLLe3E7Zu

This is because both the State Diagram and the Class Diagram define <def> elements that use the exact same id. Below is an example of the <dev> id that is generated in these SVGs. It is not unique like other diagrams generate. It would be expected to have dependencyEnd<some_number>, but instead all Class and State Diagrams use the exact same id.

<marker id="dependencyEnd" refX="19" refY="7" markerWidth="20" markerHeight="28" orient="auto"><path d="M 19,7 L9,13 L14,7 L9,1 Z"></path></marker>

This only works as long as the first doesn't get hidden (the only ID that the browser cares about, as IDs are required to be unique).

I imagine stylesheets would also have to get cleaned up as they are currently using things like:

#dependencyStart {
  @include composition;
}

#dependencyEnd {
  @include composition;
}

You could maybe get away with something like:

[id^="dependencyEnd] {
  @include composition;
}

On a side note, using IDs in general like this could mistakenly break things in a site if a user happens to use the same ID in their document. At the very least, it may make sense to kind of namespace the ids to reduce the chance of a user accidentally using the same ID __mermaid_dependencyEnd<number>. Something to think about.

Expected behavior

All <dev> elements should be generated with unique ids.

Screenshots

See above

Desktop (please complete the following information):

Not OS or Browser specific

  • Mermaid Version: 8.4.8
@facelessuser facelessuser added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Mar 26, 2020
@knsv
Copy link
Collaborator

knsv commented Mar 28, 2020

Good point. I am in the middle of rewriting the rendering engine for flowcharts and state diagrams and I will start using this approach there.

@facelessuser
Copy link
Author

Look forward to it! Great project by the way.

@ismarslomic
Copy link

Maybe this is another feature request, but it would be nice if Classes had an Id property, such as Nodes in Flowchart Diagram. That would allow to draw two Classes with identical name but different Ids in same Class Diagram

@TheSonOfThomp
Copy link

FWIW, consider the specificity of targeting [id^="dependencyEnd] vs #dependencyEnd. This may (or may not) cause issues with styling

@ismarslomic
Copy link

Again, there is already an solution for Nodes in Flowchart Diagram, which provides possibility to name two Nodes identically, but refer and style them separately. Not sure what makes applying the same solution for Class Diagram more difficult. Or is just that different contributors works on different parts of Mermaid and do not reuse solutions across for some specific reason?

@facelessuser
Copy link
Author

The opening suggestions is mearly a suggestion, any solution that solves the issue is fine with me, but sequence diagrams use unique ids an do not suffer from the problem the opening statement shows.

Sharing the ids works for class diagrams except when the first element is hidden in something like a details element or as shown with the tab implementation above.

As for the [id^="dependencyEnd"], this probably wouldn't pose a problem assuming it is scoped under the Mermaid SVG element, and mermaid isn't defining conflicting IDs. I feel this is all under Mermaid's control, so if Mermaid doesn't control it properly, it could cause issues, but if they thoughtfully assign good ID names then it could probably work. Or they could rework the styling. Also, as suggested earlier, it probably wouldn't hurt to namespace some of this under Mermaid to avoid ID collisions: [id^="__mermaid-dependencyEnd"].

These are all just suggestions, but out of the two implementations (shared ids vs unique ids) unique ids don't exhibit issues.

@facelessuser
Copy link
Author

So I spent some time on this today, and found that if create my own loader that wraps each diagram in a shadow DOM, I no longer get non-unique ID problems as the individual diagrams no longer are shared.

encapsulate

I don't know if this is a direction that Mermaid would like to go in, but it may be something to think about if anyone has general issues with ID conflicts. I find this solves most of my issues.

@fralau
Copy link

fralau commented Aug 26, 2020

For more information on fralau/mkdocs-mermaid2-plugin/issues/8: with the mermaid2 plugin for mkdocs we are indeed in the process of implementing @facelessuser's custom loader, as a complement to the standard loader, to solve that issue.

mermaid is a wonderful tool and superbly simple to use 👏. Unfortunately, things can still get a little thorny and complex, once people with moderate knowledge of javascript and css try to "open the box" and follow the details.

Let's say that it was a little like Alice's jump into a rabbit hole 🤷‍♀️🐇🎩: very instructive, but time-consuming and occasionally bewildering ("curiouser and curiouser"). Thanks to @facelessuser's incredible patience, we had 68 comments over eight days.

I know this might sound a little self-centered but I would be overjoyed if I could use mermaid, without worrying about how it works and corner cases (encapsulation). So, if we had all the javacript in a "one-stop-shop", that would make the plugin's code so much simpler. 😊

@lishid
Copy link
Contributor

lishid commented Jun 6, 2021

@knsv Just curious if there was any progress on this - I'm doing a routine bug sweep and it seems that this bug is still a common encounter among users.

@Willowlark
Copy link

Would like to bump the above question, have encountered the described bug myself as a new user.

@ghost
Copy link

ghost commented Sep 14, 2021

So I spent some time on this today, and found that if create my own loader that wraps each diagram in a shadow DOM, I no longer get non-unique ID problems as the individual diagrams no longer are shared.

encapsulate

I don't know if this is a direction that Mermaid would like to go in, but it may be something to think about if anyone has general issues with ID conflicts. I find this solves most of my issues.

Can you guide me how to fix the arrow issue? I'm using mermaid in Obsidian, and when i export note to PDF, the arrow of diagram disappear. Like this:
image

Sorry my English is not good.

@dwhelan
Copy link

dwhelan commented Oct 3, 2022

I just checked Obsidian PDF export of:

graph TD;
A --> B --> C
Loading

And the PDF looks like:

image

So I think this is no longer an issue.

@dwhelan
Copy link

dwhelan commented Oct 3, 2022

This will be fixed by #1871 which created unique marker ids for each marker.

@fidel-ruiz
Copy link

@dwhelan I'm having an issue that might be caused by the ids issue although I'm not certain., I didn't wanted to spam another issue if this one already cover it

I reported an issue to mkdocs-material that was linked above which seems to be related

When clicking the participant test it should open up the actor popup menu. the event is not triggered. I'm using the native support with mermaid with plain mkdocs.

image

I got told

The error manifests when the browser tries to parse and render the SVG markup generated by Mermaid. From the message, I read that Mermaid generated a NaN value, which is invalid for dy. I understand that this might be solely related to Mermaid.js running in Shadow DOM, but since this is a #1318, I need to close this issue as not fixable.

thanks in advance for any insights

@arnowelzel
Copy link

arnowelzel commented Nov 11, 2023

This will be fixed by #1871 which created unique marker ids for each marker.

Very nice. But since already a year passed since then - what is the status, since this issue here is still open? Thanks for an update.

@mejo-
Copy link

mejo- commented Nov 14, 2023

Very nice. But since already a year passed since then - what is the status, since this issue here is still open? Thanks for an update.

Seems like this got fixed by #4825 and released with mermaid 10.5.0. So I guess the issue can be closed.

@jgreywolf
Copy link
Contributor

Closing as resolved

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

Successfully merging a pull request may close this issue.

13 participants