-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Ensure void elements have closing slash in mermaid svg #15661
Ensure void elements have closing slash in mermaid svg #15661
Conversation
Thanks for making a pull request to jupyterlab! |
a6cc997
to
66b9da2
Compare
Upstream issue: mermaid-js/mermaid#1766 What effect would the |
Yeah, there's that one (not sure how breaking). I need to take a look at
various other control mechanisms. There's also a sandbox mode, which is
probably worth investigating, though it does target an iframe with html
srcdoc instead of SVG, so does more weird stuff to entities in
foreignobjects. Ending up with an img tag is still the most portable output.
|
Could we use the same splicing approach as we do with MathJax? See: jupyterlab/packages/rendermime/src/renderers.ts Lines 323 to 330 in e38e0b8
|
Kicking this PR to check if the labeller for mermaid now works (#15717), also FYI once you merge/rebase the tests for mermaid should now run on CI. |
Thanks. Alas, it might be a tick before I can get back to this... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amiss? In general I am not a fan of that many snapshots but I am ok with these being here as long as they are isolated to the diagrams alone (no other UI).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of all the things a screenshot is good at, diagram renders right seems like a legit use case vs all the "whole screen" shots that are sensitive to individual pixel skew.
At any rate, these are now as isolated as they can be... the "screenshot element" function doesn't handle bigger-than-viewport stuff very well, and there are two examples diagrams that are larger than the viewport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a fix for this using workaround from microsoft/playwright#13486 (an upstream fix is apparently not planned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! Pretty weird its a non-priority upstream, as this hardly seems to that unique. As commented on the new file, perhaps this belongs someplace more prominent/easier to find?
packages/mermaid/src/manager.ts
Outdated
svg = svg.replace(Private.RE_VOID_ELEMENT, Private.replaceVoidElement); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see this unit tested. For example input '<br/>'
(which I understand we do not expect from the current version of marked but I guess user or future versions could introduce it) results in '<br / >'
which strictly speaking is invalid XHTML. Basically:
new DOMParser().parseFromString('<br>'.replace(RE_VOID_ELEMENT, replaceVoidElement), 'image/svg+xml')
works, but:
new DOMParser().parseFromString('<br/>'.replace(RE_VOID_ELEMENT, replaceVoidElement), 'image/svg+xml')
fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will add some coverage.
I'm semi-disappointed we still don't have any property-based testing or overarching frontend coverage that combines jest
and puppeteer
outputs, but certainly won't be wading into the stack any more than i have to on this PR.
Ok, all of the screenshots are now working (and less sensitive to scroll skew). Will merge upstream, and see if anything pops out, but no further work planned. |
…mermaid-newlines
…nes' into jupyterlabgh-15660-mermaid-newlines
); | ||
|
||
const clean = MermaidManager.cleanMermaidSvg(raw); | ||
const parsed = parser.parseFromString(clean, SVG_MIME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would now error if the cleaned string is not valid XML... it's harder to verify that the browser would go into the "fallback" rendering mode (the most likely failure case).
Because of that, it's probably not worth roundtripping inside the implementation, which is already pretty heavy.
* for elements larger than viewport, derived from: | ||
* https://github.com/microsoft/playwright/issues/13486#issuecomment-1112012053 | ||
*/ | ||
async function resizePageAndScreenshot(locator: Locator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is something that should be hoisted up outside of a deeply-nested utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could add it as a galata helper in the future if it turns out useful elsewhere too. For now it's easier to do it here because this way we don't need to worry about it becoming a public API yet ;)
hooray all 🟢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @bollwyvl!
Thanks. Alas, the wheels turn: since this was merged, https://github.com/mermaid-js/mermaid/releases/tag/v10.8.0 😿 I don't see any hard blockers/features that would require going out, but will take a look, especially to kick the tires on the screenshot approach. |
References
Code changes
<br>
tags into closed<br/>
when generating theimg
tag containing mermaid SVGUser-facing changes
Security/Compatibility considerations
hr
andbr
style="border-color: red"
attribute can be used on anhr
to change its color<input type="checkbox" />
andradio
can be used to do weird things<img/>
approachimg
tags<script>
tagsonclick="javascript:alert('pwnd')"
attributes do appear to pass through on the playground, and in4.1.0b1
if a user goes through right-click, open in new tab and then interacts with the diagramBackwards-incompatible changes