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

SVG height attribute absent with 9.1.7 #3659

Closed
silverwind opened this issue Oct 12, 2022 · 4 comments · Fixed by #3668
Closed

SVG height attribute absent with 9.1.7 #3659

silverwind opened this issue Oct 12, 2022 · 4 comments · Fixed by #3668
Assignees
Labels
Type: Bug / Error Something isn't working or is incorrect

Comments

@silverwind
Copy link
Contributor

silverwind commented Oct 12, 2022

Describe the bug
Since 9.1.7, it seems that the SVG that mermaid.mermaidAPI.render no longer produces a height attribute on the <svg> root node. Having a known height is important for page layout for example when rendering into a <iframe> which generally requires a height.

Opening SVG Tag in 9.1.6:

<svg viewBox="0 0 124.61666870117188 236" style="max-width: 124.61666870117188px;" height="236" aria-labelledby="chart-title-mermaid chart-desc-mermaid" role="img" xmlns="http://www.w3.org/2000/svg" width="100%" id="mermaid">

Opening SVG Tag in 9.1.7:

<svg viewBox="0 0 124.61666870117188 236" style="max-width: 124.61666870117188px;" aria-labelledby="chart-title-mermaid chart-desc-mermaid" role="img" xmlns="http://www.w3.org/2000/svg" width="100%" id="mermaid">

To Reproduce
Render this source using mermaid.mermaidAPI.render("mermaid", source, callback):

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

Expected behavior
The height attribute to be present.

I think it may have regressed in https://github.com/mermaid-js/mermaid/pull/3364/files#diff-3274f1a37032fb0ae4e2823def0007c634e869ae0dfc304ff6a12c36513c3a52.

@silverwind silverwind added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Oct 12, 2022
@knsv knsv self-assigned this Oct 13, 2022
@knsv knsv removed the Status: Triage Needs to be verified, categorized, etc label Oct 13, 2022
@knsv
Copy link
Collaborator

knsv commented Oct 13, 2022

Hi! The height attribute was removed as a part of fixing #3262.

You can see a reference to the fixed bug below. I am returning the height in the for the case where you are not using maxWidth.

When using maxWidth though the width is set to 100% and the height is calculated from that by the browser after the diagram is rendered. Here the browser sets the real height after the rendering. I can add an aspect ratio style for this case.

@silverwind can you have useMaxWidth set to false in you context?

When using useMaxWidth: true in place you got rendering like this (crop of the top of the diagram) for the diagram below:
image

The diagram after the fix:

graph TD
0(node 0)
0 --> 1
1(node 1)
1 --> 2
2(node 2)
2 --> 3
3(node 3)
3 --> 4
3 --> 5
3 --> 6
4(node 4)
4 --> 7
4 --> 8
4 --> 9
4 --> 10
4 --> 11
5(node 5)
5 --> 14
6(node 6)
7(node 7)
7 --> 17
7 --> 29
8(node 8)
8 --> 30
9(node 9)
9 --> 13
9 --> 17
9 --> 31
10(node 10)
10 --> 17
10 --> 24
10 --> 52
11(node 11)
11 --> 12
11 --> 17
11 --> 32
12(node 12)
12 --> 17
13(node 13)
13 --> 14
13 --> 15
13 --> 19
14(node 14)
14 --> 17
15(node 15)
15 --> 16
15 --> 17
15 --> 36
15 --> 59
16(node 16)
16 --> 17
17(node 17)
17 --> 18
18(node 18)
18 --> 19
19(node 19)
19 --> 20
20(node 20)
20 --> 21
20 --> 22
21(node 21)
21 --> 27
22(node 22)
22 --> 23
23(node 23)
23 --> 24
23 --> 25
24(node 24)
24 --> 25
25(node 25)
25 --> 26
26(node 26)
26 --> 27
27(node 27)
27 --> 28
27 --> 42
28(node 28)
28 --> 29
28 --> 30
28 --> 31
28 --> 32
28 --> 39
29(node 29)
29 --> 35
29 --> 40
29 --> 44
29 --> 64
30(node 30)
30 --> 33
30 --> 64
31(node 31)
31 --> 34
31 --> 40
31 --> 44
31 --> 64
32(node 32)
32 --> 48
32 --> 64
33(node 33)
33 --> 49
34(node 34)
34 --> 37
34 --> 38
34 --> 43
35(node 35)
35 --> 36
35 --> 39
36(node 36)
36 --> 42
37(node 37)
37 --> 41
38(node 38)
38 --> 43
38 --> 50
38 --> 59
39(node 39)
39 --> 40
39 --> 44
39 --> 64
40(node 40)
40 --> 49
40 --> 72
41(node 41)
41 --> 47
42(node 42)
42 --> 45
43(node 43)
43 --> 55
44(node 44)
44 --> 46
44 --> 48
45(node 45)
45 --> 64
45 --> 71
46(node 46)
46 --> 71
46 --> 72
47(node 47)
47 --> 50
47 --> 59
48(node 48)
48 --> 64
49(node 49)
49 --> 51
49 --> 56
50(node 50)
50 --> 53
51(node 51)
51 --> 52
52(node 52)
52 --> 54
53(node 53)
53 --> 64
53 --> 72
54(node 54)
54 --> 55
54 --> 56
55(node 55)
55 --> 58
55 --> 59
56(node 56)
56 --> 57
57(node 57)
57 --> 60
58(node 58)
58 --> 61
59(node 59)
59 --> 61
60(node 60)
60 --> 62
60 --> 63
61(node 61)
61 --> 64
62(node 62)
62 --> 65
63(node 63)
63 --> 66
64(node 64)
65(node 65)
65 --> 67
65 --> 70
66(node 66)
66 --> 68
66 --> 69
67(node 67)
67 --> 69
68(node 68)
68 --> 70
69(node 69)
69 --> 71
70(node 70)
70 --> 72
71(node 71)
72(node 72)

@silverwind
Copy link
Contributor Author

silverwind commented Oct 13, 2022

My use case can be seen here. The SVG is inserted in an <iframe> with 100% width and as far as I know, iframes need a explicit height in all cases, e.g. it's impossible to let them naturally "expand" with content.

My current workaround is to parse the height out of viewBox. I will try if useMaxWidth has any effect. I'd definitely prefer to parse height out of height instead of viewBox because viewBox pixels would not correspond to real pixels in case the SVG is being scaled.

@silverwind
Copy link
Contributor Author

silverwind commented Oct 13, 2022

I just tried with flowchart: {useMaxWidth: false} and it does change width from previously 100% to a pixel value. But it does not help my use case, because I want 100% width, with an pixel value for height so I can size the <iframe>.

viewBox does seem to give what I want currently, so I'm personally happy with the workaround as long as mermaid keeps viewPort units to pixels.

Generally speaking, I think mermaid should not concern itself with ever setting 100% in any dimension and just always deliver pixel values in both dimensions. Users can size their SVGs how they want via CSS, it should not be a concern to mermaid imho.

@knsv
Copy link
Collaborator

knsv commented Oct 14, 2022

Yes, that would have been a more clear separation of concern. 😄

Glad that you get what you need from the viewport. It was still good to look at this as the height should be set when not using maxWidth, will update that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants