outputarea.js: Wrap inline SVGs inside an iframe

[pablooliveira]

When multiple inline SVGs are included in a single document,
they share the same parse tree. Therefore, style collisions and
use id collisions can occur and upset the rendering.

This patch wraps each SVG inside an individual iframe, ensuring
that SVG's declarations do not collide.

(The SVG representation is kept as XML and not converted to a binary
format, so I do not think this approach precludes the use of d3.js)

Tested on:

• Chrome Version 29.0.1547.57 Debian 7.1 (217859)
• Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130806 Firefox/17.0 Iceweasel/17.0.8

Closes #1866

This PR wraps SVG figures inside an iframe (originally suggested by @Carreau in #1866).

[pablooliveira]

@stefanv, @Carreau, @minrk : could you please review this PR ?

[ellisonbg]

But hold on, isn't this also true about HTML/JavaScript output as well? In
general, anything that gets put on the notebook page has to be a respectful
page member by using ids and classes that won't collide. Am I
misunderstanding things? Can someone detail the downsides of using iframes?

On Sun, Sep 22, 2013 at 4:14 AM, Pablo de Oliveira Castro <
notifications@github.com> wrote:

@stefanv https://github.com/stefanv, @Carreauhttps://github.com/Carreau,
@minrk https://github.com/minrk : could you please review this PR ?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4250#issuecomment-24880168
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[pablooliveira]

@ellisonbg

Yes I believe you can also have collisions with HTML and JavaScript generated in cells.

(Sorry most of my examples are in R, because that is what I'm more familiar with)

The difference, I think, is that usually when using HTML or Javascript, the user finely
controls the HTML or JS output. Either they generate it directly or use a package (such as
R xtable to generate an html table). In both cases the user or the package expect
the generated content to be inlined in a larger document, so they are good citizens
and do not pollute the namespace.

The problem with SVG is that most of the time, it's not inlined but embeded (as an external image).
Therefore the packages do not particularly care about generating unique ids.
Let me give you a concrete example, when using R in the notebook I may type:

%%R
require(ggplot2)
ggplot(data = data.frame(x = runif(1000)), aes(x=x)) + geom_histogram()

to generate an histogram of a 1000 uniform sampling.
If the default device is svg, the produced SVG is going to contain a set of
symbol definitions such as

<symbol overflow="visible" id="glyph0-1">
<path style="stroke:none;" d="M 2.640625 -6.796875 ... "/>
</symbol>

Now if in the next cell I plot a second figure, the name glyph0-1 is going to be reused by
ggplot, because it does not know that the SVGs are inlined in the same document
and does not need to produce unique identifiers. The user has no control over this.

The same problem may happen when the user includes multiple SVG figures from a foreign URL link.
Because different foreign sources created the SVGs, it's highly possible to have id collisions.

A thourough discussion of the issue is available in the original issue #1866

[Carreau]

I'm torn. one one end this is nice to have a workaround.
On another side, d3.js for example recommend to people to use CSS to style svg on a full page, which is nice.
The discussion about conflicting id can be extended to html too, with tools like Cython publishing html/css (cython annotate) that conflict when embedding.

I'm pondering also the fact that iframing might prevent people to use SVG+js to have dynamic graphs.

[pablooliveira]

I agree that there is a trade-off here.
Maybe we could have the best of both worlds by configuring the desired svg scoping in the IPython.core.display.SVG DisplayObject ?

Something like:

SVG(url="http://example.com/mysvg.svg", scoped=True)

But this would require to add an extra parameter to the SVG format in the notebook.

[pablooliveira]

Updated, now SVG scoping is disabled by default but it can be enabled using the proposed scoped=True syntax.

[Carreau]

instead of adding a class, what do you think to 'just' make make the scope attribute be part of the metadata sent along the svg like we do with size for Image ?
Still trying to design a not-ipython-specific thing.

[pablooliveira]

Sure, excellent idea. I did not realize that _repr_svg_ could return an obj, md tuple, nor that Julia was using the notebook format independently. Thanks for your explanation in #1866.

60b728b uses metadata instead of a class.

[Carreau]

No problem, I'm feeling better to have this like that.

I'm then wondering; (I know I can be annoying sometimes) if it makes sens in other cases. (html/js) as pointed by @ellisonbg . eg, cython --annotate leaks some css/js in global space that prevent from toggling output in second annotate output. (fix in progress on cython side).

Probably can bring it up in Thursday live meeting.

[pablooliveira]

@Carreau: Could you please bring this up in tomorrow live meeting if time allows ? Thanks :)

[Carreau]

Has been discused durring hangout, recap is we will add a global metada flag to all output type :

Add a scoped/isolate/sandbox metadata key to all output types.
If it is set, put the output in an iframe.
False by default

The spec should not say that element are in iframes, but are isolated (we keep the possibility to change mechanism later),hence we won't give easy access to sandbox and seamless attributes. Output that leak into main namespace are still considered bugs.

@pablooliveira
Would you like to take a shot at doing that ?

[pablooliveira]

@pablooliveira Would you like to take a shot at doing that?

Sure! I'm currently traveling but I can work on it this coming week.

[Carreau]

No hurry :-)

[stefanv]

Mathias, could you also give a brief outline of the thinking to get to this
solution, and how it compares to the other options?

[Carreau]

The rational was mostly that the problem of scoping does not apply only to svg, but things like html/javascript (example, cython -a leaks css into main namespace), and might be frontend specific. Even if we do consider this as bugs in underlying library, the use case is high enough to add this option. The need to make the flag global also come from the fact that we will allow arbitrary mimetype also.

There was arguing that then people just had to publish iframe themselves, but then you cannot use thoses iframes in nbconvert except for html.

Seeing that publishing with display can carry along arbitrary metadata, we think that the notebook frontend can look at those metadata, and if the metadata tells the output need "protection" it will be wrapped in an iframe.

It compares to other solution in the sens that :

• it is global for all display types.
• it can completely get ignored by the frontend.
• it does not change the payload.
• it does not assume a specific way an element will be protected. for now, we will use iframes.

[stefanv]

Thank you, that is very helpful.

[pablooliveira]

I have committed a first tentative implementation for supporting "isolated" metadata on any content.

I have tried it successfully on Firefox and Chrome:

• Chrome Version 29.0.1547.57 Debian 7.1 (217859)
• Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130806 Firefox/17.0 Iceweasel/17.0.8

I have tried isolating: svgs, html, javascript and cython annotations.
My test notebook is available at http://www.sifflez.org/misc/tmp/test-isolated.ipynb

There are two problems left:
1# Firefox adds extra space at the end of small iframes (so bottom margins are larger on isolated content).
2# When the iframe content changes dynamically (expanding cython annotations for example), the iframe is not resized.

2# can fixed with a timer that resizes the iframe periodically, but it seems ugly and a battery eater. Any suggestions ? I cannot find an event that is fired on the inner iframe that would allow me to avoid polling.

Thanks for your reviews!

[stefanv]

http://stackoverflow.com/questions/16297675/resize-iframe-after-content-height-changes

[pablooliveira]

@stefanv:
The SO post you referenced links to http://codecopy.wordpress.com/2013/02/22/ghost-iframe-crossdomain-iframe-resize/. That solution works because the iframe explicitly asks for a resize. We cannot do that because we do not control the javascript that is included in the iframe.

[stefanv]

I meant to link to the first entry:
https://github.com/Sly777/Iframe-Height-Jquery-Plugin

Which seems to have the ability to watch for changes.

On Mon, Oct 7, 2013 at 1:03 PM, Pablo de Oliveira Castro <
notifications@github.com> wrote:

@stefanv https://github.com/stefanv:
The SO post you referenced links to
http://codecopy.wordpress.com/2013/02/22/ghost-iframe-crossdomain-iframe-resize/.
That solution works because the iframe explicitly asks for a resize. We
cannot do that because we do not control the javascript that is included in
the iframe.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4250#issuecomment-25799988
.

[pablooliveira]

@stefanv: I just had a look at the Iframe-Height-Jquery-Plugin. It uses a periodic timer to update the iframe.

[pablooliveira]

c6211c5 fixes issue 1#.

Only issue 2# remains: should we resize iframe when the content changes dynamically ?

In the current implementation, if the content becomes larger after page load, a scroll bar appears
and the user needs to scroll to navigate the iframe.

Iframe can be resized but this needs to set up a periodic timer that check the iframe content.
I do not like the periodic timer because: 1) it is innefficient, 2) does not seem to work well
in Firefox (flickering observed).

I'm not sure we want to support 2#. Dynamic resizable isolated content is a rare use-case and can be avoided
if the user sets the maximal size from the start. Thoughts?

[pablooliveira]

@Carreau, @stefanv: If we decide that we do not support issue 2#, this is ready in my opinion. Anything else we should do? I would like to merge this so we can ressurect #1865 :-)

[pablooliveira]

Added a CasperJS test for "isolated" and rebased on master.

[minrk]

Added a CasperJS test for "isolated" and rebased on master.

That's what I like to hear!

[ivanov]

Added a CasperJS test

That's what I like to hear!

me too, given how much I spent wrestling with JS on that, glad to see it put to use so quickly. :)

I tried this out and it seems sensible to me, so I'm 👍. The only caveat is that having IFrames like this allows for a breaking of our regular layout a bit (if the SVG is wide enough, for example, the code cell doing the execution also becomes much wiser. I don't think there's much we can do about that, and I presume (though not certain) that this would also be the case if one were to make an HTML repr that created an IFrame (without asking for isolation), that it would do the same thing.

[stefanv]

Javascript tests in IPython! I am crying liquid golden tears of purest joy.

[pablooliveira]

@ivanov :

me too, given how much I spent wrestling with JS on that, glad to see it put to use so quickly. :)

Congratulations, the CasperJS integration is very nice!

I tried this out and it seems sensible to me, so I'm 👍. The only caveat is that having IFrames like this allows for a breaking of our regular layout a bit (if the SVG is wide enough, for example, the code cell doing the execution also becomes much wiser. I don't think there's much we can do about that, and I presume (though not certain) that this would also be the case if one were to make an HTML repr that created an IFrame (without asking for isolation), that it would do the same thing.

The layout breaks for a very large iframe, but for me it also breaks when I use a very large SVG without iframe, so this is not specific to the "isolated" support. I'm not sure, as you said, there is much we can do about that.

(EDIT: btw, this seems to happen only in Firefox, in Chrome the code cell size is not affected by the SVG size.)

[pablooliveira]

21f1b06 fixes the issue reported by @ivanov, about large isolated objects breaking the layout in Firefox.
The idea is to set the iframe width to the maximum width of the parent cell.
Height is still automatically computed.

[pablooliveira]

In my opinion this PR is ready for merge. Any bugs or improvements left?

[takluyver]

@Carreau : Are you happy with this implementation of what we decided?

[ivanov]

hang on,

from IPython.core.display import SVG, display_svg
s1 = '''<svg width='1cm' height='1cm' viewBox='0 0 1000 500'><defs><style>rect {fill:red;}; </style></defs><rect id='r1' x='200' y='100' width='600' height='300' /></svg>'''
s2 = '''<svg width='1cm' height='1cm' viewBox='0 0 1000 500'><rect id='r2' x='200' y='100' width='600' height='300' /></svg>'''
display_svg(SVG(s1), metadata=dict(isolated=True))
display_svg(SVG(s2), metadata=dict(isolated=True))

works as advertised, but the following does not:

from IPython.display import display
display(SVG(s1), metadata=dict(isolated=True))
display(SVG(s2), metadata=dict(isolated=True))

the problem is that the _svg calls create a MIME type-keyed metadata dict, i.e. {image/svg+xml': {'isolated': True}}, whereas the plain display() calls create just a dict with 'isolated' as the key, i.e. '{isolated': True}. This PR makes the former work, but not the latter, and I wonder if it's worth fixing that here or as a seperate PR, since looking at the display related code it would benefit from refactoring, so I can start to work on that

[pablooliveira]

@ivanov: that is correct.

This PR triggers isolation for mime-specific metadata such as {'image/svg+xml': {'isolated': True}}
or {'application/javascript': {'isolated': True}}.

The semantics are not clear for non-mime-specific metadata such as {'isolated': True}, should we isolate
every object separately, or put all the objects together in the same iframe ? I'm not sure we want to
support it.

But I agree it's not nice that display(SVG(s1), metadata=dict(isolated=True)) does not work.
The thing is that display takes a tuple of objects, therefore it's not clear to whom the metadata should apply.

I think one good way to fix this would be to add a metadata keyword to the DisplayObject class constructor.
Then we could write: display(SVG(s1, metadata=dict(isolated=True)), Image(png, metadata=dict(isolated=False))). Also, I'm not sure the metadata keyword argument in display makes sense. I agree with you that this is orthogonal and should probably have its own PR, if you need any help, just ping me :)

[ivanov]

The problem I see with adding to the class constructor is that we're sort of hard-coding how they should be displayed into the objects themselves, which isn't so nice. The approach I consider reasonable for isolated=True passed to display() is to just recreate a metadata dict with the appropriate keys and isolate each of the displayed objects.

[pablooliveira]

This should work nicely for isolated.
Will this work for other metadata information? Are there some metadata keys passed to display that should remain non-metadata-specific?

[ivanov]

Ok, I'm merging this and will start working on refactoring the display code so that metadata is passed around to the individual mime-keyed entries (i.e. as supported by the implementation in this PR)

[pablooliveira]

Thanks for the merge and review !