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

Problems with SVG files containing <script> #546

Closed
kimikage opened this issue Oct 8, 2020 · 10 comments
Closed

Problems with SVG files containing <script> #546

kimikage opened this issue Oct 8, 2020 · 10 comments
Assignees
Labels
enhancement New feature or request frontend Concerning the HTML editor

Comments

@kimikage
Copy link

kimikage commented Oct 8, 2020

First of all, I don't know much about the Pluto inside or JavaScript. Please correct me if there is any misunderstanding.

For example, the SVGJS backend of Gadfly outputs a <svg> with embedded <script>s (cf. http://gadflyjl.org/v1.3/man/backends/#Interactive-SVGs-1). However, there are some problems in using the interactive feature.

  1. Static <img>

Since Pluto uses <img> to display SVG images, interactive features are not available. This problem can be solved by changing the output format to MIME"text/html" (in the packages or user code).

  1. SVGScriptElement

SVG's <script> element (SVGScriptElement) is similar to HTML's <script>, but they are not identical in the specifications.
The problem is that SVGScriptElement does not have the src attribute. (Some implementations may support it, though.) Thus, node.src in the following lines may be undefined, and (node.src != "") will be true.

// Run scripts sequentially
for (let node of script_nodes) {
root_node.currentScript = node
if (node.src != "") {

This problem should be solved by changing the condition to if (node.src). Of course, we can do a more strict check.

  1. Separation of environments by ID

An SVG image may contain multiple <script> elements. They are usually assumed to share an environment. However, the separation is currently done based on node.id.

let script_id = node.id

I don't know of a versatile solution to this, but I think that we could loosen up the scope a bit more, as shown below.

let script_id = node.id ? node.id : node.parentNode.id
  1. AMD & RequireJS support

The SVGJS backend of Gadfly uses Snap.svg. Snap.svg uses the AMD (Asynchronous Module Definition) for module management.
Now that ES Modules are widely supported, the AMD is an old technology, but some JavaScript libraries still use the AMD and RequireJS. This can lead to the following problem.

  1. Fallback using the global environment

The libraries which use the AMD usually register a module object in the global environment when the AMD is not available. In general, this can cause problems with conflicts between multiple versions, but as far as the libraries are used in a single Pluto notebook, I think they rarely cause conflicts.
However, on the Pluto front-end, the default environment (this) is undefined. This can cause an error in code which assumes that this is the global environment, i.e., window.

this: script_id ? previous_results_map.get(script_id) : undefined,

I don't think polluting the global environment is a good idea. However, it would be better to share a Node object of some kind (e.g. root_node) with the same script_id instead of undefined.

  1. innerHTML

I think textContent is more appropriate than innerHTML.

code: node.innerHTML,

@dralletje
Copy link
Collaborator

Do you have an example notebook using Gadfly?
Rendering svg's in img tags has several benefits we are not keen on getting rid of, and I want to see how much is actually broken when rendering Gadfly as html.

@dralletje
Copy link
Collaborator

Okay tried it myself 😁
Your feedback it totally valid, we're still on a bit of a crossroad between "Supporting all inline html and script" and "Make writing html and scripts in Pluto itself fun". Most likely we will end on the side of "Supporting all inline html and script" though, and when doing that your points will be very helpful :)

Until we have this covered, you can wrap the snippet in <html> tags, so Pluto will render it in an iframe

# ╔═╡ af27fc22-0964-11eb-0382-8343faea368e
using Gadfly

# ╔═╡ 09a1b2ba-0965-11eb-2f11-73708c4a8bd9
HTMLDocument(plot(y=[1,2,3]))

# ╔═╡ 0751415a-0966-11eb-07d0-69316a9b61c8
struct HTMLDocument
	embedded
end

# ╔═╡ 062f3880-0967-11eb-3c2c-1db7aa9d40b8
function Base.show(io::IO, mime::MIME"text/html", doc::HTMLDocument)
	println(io, "<html>")
	show(io, mime, doc.embedded)
	println(io, "</html>")
end

@kimikage
Copy link
Author

kimikage commented Oct 8, 2020

Oh, sorry. I mentioned Gadfly just because I thought it was a well-known package, but as for "1. Static <img>", Gadfly should already output SVGs as HTML, as you get. 😅

Perhaps the only essential issue is AMD support.

@dralletje
Copy link
Collaborator

Yeah, I was expecting all AMD packages to be imported with <script src="...">, never seen an AMD package just placed in a <script>...</script> tag 😂 I'll keep you posted

@fonsp
Copy link
Owner

fonsp commented Nov 1, 2020

1: we will not change this behaviour. Your suggestion is correct: a package that wants scripts to work should output the html mime type.

2: fixed in 694526b

3: I am not sure what you mean with "environment", but our ID tracking is only used for a pluto-specific (additional) feature. It should be unrelated

4 & 5: this is a good point. Let's make that the focus of this issue

6: you must have misread something

fonsp added a commit that referenced this issue Nov 1, 2020
@kimikage
Copy link
Author

kimikage commented Nov 1, 2020

Thank you for your response and the fix.

6: you must have misread something

Please explain in a little more detail. The innerHTML is for HTML and should not be used for anything other than HTML.

@fonsp
Copy link
Owner

fonsp commented Nov 1, 2020

Ah it is I who misread. Still I don't think this matters.

@kimikage
Copy link
Author

kimikage commented Nov 2, 2020

Still I don't think this matters.

Yes. So I placed it at the end (i.e. 6.). However, in SVG, an XML format, <script> is often used with CDATA, and innerHTML causes unintentional escaping.

### A Pluto.jl notebook ###
# v0.12.6

using Markdown
using InteractiveUtils

# ╔═╡ 10303500-1cc2-11eb-22fc-6711d6ea7162
struct MyStruct end

# ╔═╡ 2f160ee0-1cc2-11eb-0db8-9f9c1b645189
Base.showable(::MIME"text/html", ::MyStruct) = true

# ╔═╡ 54186492-1cc2-11eb-109d-c31ef1da0c3c
function Base.show(io::IO, ::MIME"text/html", ::MyStruct)
    write(io, """
        <html>
        <body>
          <svg width="20mm" height="15mm" viewBox="0,0,20,15">
            <script><![CDATA[
              if (1 > 0) {
                console.log(document.getElementById("my-circle"));
              }
            ]]></script>
            <circle fill="green" cx="10" cy="7" r="5" id="my-circle" />
          </svg>
        </body>
        </html>
        """)
end

# ╔═╡ 051d1660-1cdb-11eb-3e3f-01545ab6cb41
MyStruct()

# ╔═╡ Cell order:
# ╠═10303500-1cc2-11eb-22fc-6711d6ea7162
# ╠═2f160ee0-1cc2-11eb-0db8-9f9c1b645189
# ╠═54186492-1cc2-11eb-109d-c31ef1da0c3c
# ╠═051d1660-1cdb-11eb-3e3f-01545ab6cb41

@fonsp
Copy link
Owner

fonsp commented Nov 2, 2020

Ah okay, your original wording made it sound like a style comment. We'll change it

@fonsp fonsp closed this as completed in e268b99 Nov 5, 2020
@fonsp
Copy link
Owner

fonsp commented Nov 5, 2020

We reverted to this === window for classic <script>s with no id set. And innerHTML 👉 innerText

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Concerning the HTML editor
Projects
None yet
Development

No branches or pull requests

3 participants