Properly handling <br> tags

[daladim]

See issue #74

<br> tags are valid HTML, but SVG requires valid XML (where <br> is written <br/>)

I'm sorry, but I'm new to node and I haven't been able to test this commit :-(
(after yarn and source copy_modules.sh, the doc instructs me to run node index.bundle.js -i test/state1.mmd but there is no index.bundle.js, I have no idea whether it is actually missing or whether I have failed to generate it).

A test case is described in the linked issue in case you want to test it yourself (but I'd be happy someone tells me what I am missing to test this)

[MindaugasLaganeckas]

@daladim thank you so much for your PR! 😄 I will update the doc. This is what I propose:

Before I can accept your PR, I need at least one automated test. Please, get the latest changes from master and implement an e2e test in the run-tests.sh. Get inspired from the existing tests. I propose this testing strategy:

1. Create an mmd file with <br> and <br/> tags in test-positive/
2. Create an svg from the diagram using mmdc.
3. Convert svg to png using one of Linux utilities (Percy test framework does not work with svg files). I propose using
   docker run -it flungo/inkscape /usr/bin/inkscape --help

That is it! Hope it help! 😄

[daladim]

I have added e2e tests.
But unfortunately inkscape merely prints warnings but does not return any error code on a malformed SVG, so it may be hard to catch these errors (unless grepping the output for parsing error, but you probably have a better idea :-p )

docker run -v /home/jerome/src/external/mermaid-cli:/data flungo/inkscape /usr/bin/inkscape -e /data/test-positive/graph-with-br.mmd-converted.png /data/test-positive/graph-with-br.mmd.svg
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Opening and ending tag mismatch: br line 1 and div
e="display: inline-block; white-space: nowrap;">Line 1<br>Line 2<br>Line 3</div>
                                                                               ^
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Opening and ending tag mismatch: br line 1 and foreignObject
ne-block; white-space: nowrap;">Line 1<br>Line 2<br>Line 3</div></foreignObject>
                                                                               ^
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Opening and ending tag mismatch: div line 1 and g
lock; white-space: nowrap;">Line 1<br>Line 2<br>Line 3</div></foreignObject></g>
                                                                               ^
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Opening and ending tag mismatch: foreignObject line 1 and g
; white-space: nowrap;">Line 1<br>Line 2<br>Line 3</div></foreignObject></g></g>
                                                                               ^
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Opening and ending tag mismatch: g line 1 and svg
">Line 1<br>Line 2<br>Line 3</div></foreignObject></g></g></g></g></g></g></svg>
                                                                               ^
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Premature end of data in tag g line 1
">Line 1<br>Line 2<br>Line 3</div></foreignObject></g></g></g></g></g></g></svg>
                                                                               ^
/data/test-positive/graph-with-br.mmd.svg:1: parser error : Premature end of data in tag svg line 1
">Line 1<br>Line 2<br>Line 3</div></foreignObject></g></g></g></g></g></g></svg>
                                                                               ^
parsing error: 1:1925:could not recognize next production
parsing error: 1:1954:while parsing rulset: current char must be a '}'
parsing error: 1:1953:while parsing declaration: next property is malformed
WARNING: unknown type: svg:foreignObject
WARNING: unknown type: svg:foreignObject
Background RRGGBBAA: ffffff00
Area 0:0:150:140 exported to 150 x 140 pixels (96 dpi)
Bitmap saved as: /data/test-positive/graph-with-br.mmd-converted.png

Also, it looks like other test files (such as sequence.mmd) lead to a complete crash of Inkscape.

[daladim]

Indeed, docker run -v $INPUT_DATA:/data flungo/inkscape /usr/bin/inkscape -e /data/test-positive/sequence.mmd-converted.png /data/test-positive/sequence.mmd.svg crashes (I have attached the failing .svg file).

parsing error: 1:2195:could not recognize next production
parsing error: 1:2224:while parsing rulset: current char must be a '}'
parsing error: 1:2223:while parsing declaration: next property is malformed

Emergency save activated!
Emergency save completed. Inkscape will close now.
If you can reproduce this crash, please file a bug at www.inkscape.org
with a detailed description of the steps leading to the crash, so we can fix it.
** Message: Error: Inkscape encountered an internal error and will close now.

As they suggest, I will file a report to Inkscape (unless you tell me there is a good reason not to ?)

sequence-svg.zip

[MindaugasLaganeckas]

@daladim : Could you suggest another (preferably dockerized) way of converting SVGs to PNGs? I would also suggest to implement only one test for your feature, that uses the following file: graph-with-br.mmd. More for loops we have, more time it takes to run them.
Take a look at this one. https://github.com/neocotic/convert-svg/tree/master/packages/convert-svg-to-png
You are welcome to update the workflow file.

Thanks a lot for your help! 😄

[daladim]

I found the Nu Html Checker (v.Nu)
It actually catches the <br> as invalid SVG, and accepts <br/>.

They provide a Docker image, but it only exposes a web server that validates files, without a CLI invocation possible.
They also provide a Node package, but having no experience with npm, I could not make it work, could you give me a hint here?
I have successfully run npm install -g vnu-jar, but then no vnu-jar command is available (there is a /usr/local/lib/node_modules/vnu-jar folder, but I'm not sure how to run the validator from there)

[MindaugasLaganeckas]

I will get back to you. I am bit busy at the moment 😄

[MindaugasLaganeckas]

@daladim Please, merge in master. Then in your test create .svg file. The .svg file should be created in the same location as .mmd in test-positive. I have configured a tool, that will take all svgs in that folder and convert to pngs. Ping me, when you are done, I will finish the review and merge into master.
Thanks again for your contribution! 😄

[daladim]

Thanks for your message. I'm still not able to really test locally, so I have split the test and its fix, to check I actually fixed something :-)
I'll push the fix once the CI for the test has failed

[daladim]

Hmm... It looks like the convert-svg-to-png NPM tool does not choke on invalid <br> tags: https://github.com/mermaid-js/mermaid-cli/pull/79/checks?check_run_id=1556111271#step:12:115

I have not pushed the fix commit yet, since there will be no way to check it actually fixes something

[MindaugasLaganeckas]

@daladim : I am sorry for all confussion. Let's try to figure out things together.
I am looking into this zip file: https://github.com/mermaid-js/mermaid-cli/suites/1734759781/artifacts/32875016

The package contains images built from your PR.
Here is what I see in my Firefox 84.0.1 on Windows 10 when I open svg image:

Here is how png looks like:

As far as I understand your PR, the purpose of it is that both outputs look like the one in png format.
Also you rename all png images to png.bak, which makes them unusable for image comparison with Percy. Please, have a look at my review comments.

Having said that, thanks a lot for looking into this issue 😄

[MindaugasLaganeckas]

Please, remove this line.

[daladim]

Done; This means graph-with-br.mmd.png that was created by run-tests.shwill be overwritten by the one created in this step, is that all right?
(I couldn't find what "automatic inspection" the percy step is performing, so I'm not exactly sure which PNG files I am supposed to upload to it :-S )

[daladim]

@daladim : I am sorry for all confussion. Let's try to figure out things together.
I am looking into this zip file: https://github.com/mermaid-js/mermaid-cli/suites/1734759781/artifacts/32875016

The package contains images built from your PR.
Here is what I see in my Firefox 84.0.1 on Windows 10 when I open svg image:

That is strange, I don't have the same rendering :-)
Here are the renderings I get:
Firefox 84.0.1 (64-bit), Win10 20H2

Edge on the same Windows

Firefox 84.0 on Ubuntu 20.04

But you're right, the goal of all this is to have this rendering in all software:

I have pushed a review commit to resolve your remarks. Please tell me whether they'e OK.
(I couldn't manage to really understand the "automatic inspection" step, that's why I may have not written the tests you've been expecting)

Thanks!

[daladim]

Hmmm... convert-svg-to-png failed: Error: EACCES: permission denied, open '/home/runner/work/mermaid-cli/mermaid-cli/test-positive/graph-with-br.mmd.png'
convert-svg-to-png wants to overwrite the already existing graph-with-br.mmd.png, which is owned by root (because it was created from within a Docker container).

Now I'd need your help :D
This is caused by my removal from this remark, but what does exactly the "Upload diagrams for automatic inspection" step do ? And what PNG files does it require? Should we keep/rename/delete the previously generated PNG that will conflict with this SVG-to-PNG conversion step?

[MindaugasLaganeckas]

@daladim : thanks a lot! I will get back to you (I am a bit busy at the moment).

[daladim]

Hi @MindaugasLaganeckas , do you have any suggestions for this MR ? :-)

[loganmc10]

Any update on this? I have to use sed to fix up the
tags everytime I generate an SVG

[MindaugasLaganeckas]

@daladim : please, integrate with master. I will have a look. Thanks for your help so far! :)

[daladim]

I've just rebased on master. Thanks for your feedback!

[MindaugasLaganeckas]

@daladim : I still get the same problem as before in Firefox on Windows 10:

The latest build: https://github.com/mermaid-js/mermaid-cli/suites/2843635940/artifacts/63551527
Does your fix solve the problem to you in your usecase? If YES, than I will accept your PR and we will close the case. :) Thank you for your help and patience!

[daladim]

There is actually an error in the artifacts you gave the link of, I have the same error as yours. That's strange though, because that was the whole point of this MR.

I don't know how to test it anymore. I tried a docker build and a docker run -it -v /tmp/test:/tmp/test my-built-image -i /tmp/test/graph-with-br.mmd -o /tmp/test/output.svg, but I have a strange IO/permissions error

Error: EACCES: permission denied, open '/tmp/test/output.svg'
    at Object.openSync (node:fs:583:3)
    at Object.writeFileSync (node:fs:2144:35)
    at /home/mermaidcli/node_modules/@mermaid-js/mermaid-cli/index.bundle.js:160:8
    at Generator.next (<anonymous>)
    at step (/home/mermaidcli/node_modules/@mermaid-js/mermaid-cli/index.bundle.js:4:191)
    at /home/mermaidcli/node_modules/@mermaid-js/mermaid-cli/index.bundle.js:4:361
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  errno: -13,
  syscall: 'open',
  code: 'EACCES',
  path: '/tmp/test/output.svg'
}

So, I could not test whether something broke since last December :-(

Did you check before merging? Do you want me to test something?