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

_repr_svg_ broken WRT classic notebook #6295

Closed
Carreau opened this issue May 2, 2019 · 8 comments · Fixed by #6469
Closed

_repr_svg_ broken WRT classic notebook #6295

Carreau opened this issue May 2, 2019 · 8 comments · Fixed by #6469
Labels
bug good first issue pkg:rendermime status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@Carreau
Copy link
Contributor

Carreau commented May 2, 2019

The _repr_svg_ rendering seem to be incorrect:

Screen Shot 2019-05-01 at 9 20 19 PM

Works in classic notebook (taken from ipython-in-depth tutorial)

from IPython.display import (
    display_svg
)
class MyCircle(object):

    def __init__(self, center=(0.0,0.0), radius=1.0, color='blue'):
        self.center = center
        self.radius = radius
        self.color = color
        
    def _repr_svg_(self):
        return """<svg width="100px" height="100px">
           <circle cx="50" cy="50" r="20" stroke="black" stroke-width="1" fill="blue"/>
        </svg>"""

display_svg(MyCircle())

Both in latest alpha and 0.35.

@Carreau Carreau added this to the 1.0 milestone May 2, 2019
@jasongrout
Copy link
Contributor

Because we use an image tag, rather than embedding the svg inline, you must include the xmlns attribute:

from IPython.display import (
    display_svg
)
class MyCircle(object):

    def __init__(self, center=(0.0,0.0), radius=1.0, color='blue'):
        self.center = center
        self.radius = radius
        self.color = color
        
    def _repr_svg_(self):
        return """<svg xmlns="http://www.w3.org/2000/svg" width="100px" height="100px">
           <circle cx="50" cy="50" r="20" stroke="black" stroke-width="1" fill="blue"/>
        </svg>"""

display_svg(MyCircle())

Should we try to modify the user code to add that, or should we rely on the user providing that?

@jasongrout
Copy link
Contributor

Should we try to modify the user code to add that

If it's not too error-prone, we should probably modify the svg data, otherwise there may be lots of notebooks from the past that won't render svg. We may be able to get away with just replacing an initial <svg with <svg xmlns="http://www.w3.org/2000/svg" if we can't find the appropriate string in the svg data.

@jasongrout
Copy link
Contributor

Setting to good first issue, since I think a workaround is straightforward, as noted in the last comment above.

The code to be modified is at

export function renderSVG(options: renderSVG.IRenderOptions): Promise<void> {
// Unpack the options.
let { host, source, trusted, unconfined } = options;
// Clear the content if there is no source.
if (!source) {
host.textContent = '';
return Promise.resolve(undefined);
}
// Display a message if the source is not trusted.
if (!trusted) {
host.textContent =
'Cannot display an untrusted SVG. Maybe you need to run the cell?';
return Promise.resolve(undefined);
}
// Render in img so that user can save it easily
const img = new Image();
img.src = `data:image/svg+xml,${encodeURIComponent(source)}`;
host.appendChild(img);
if (unconfined === true) {
host.classList.add('jp-mod-unconfined');
}
return Promise.resolve();
}

@Carreau
Copy link
Contributor Author

Carreau commented May 4, 2019

If we don't add this workaround, It would be great to have a warning printed. At least we should update all the tutorial material; examples, and reach to libraries to actually include the xmlns="http://www.w3.org/2000/svg"

@ian-r-rose
Copy link
Member

I suspect using something like DOMParser might be more robust than a find/replace regex that we would write.

@bollwyvl
Copy link
Contributor

bollwyvl commented May 5, 2019

I am SO torn here: on the one hand, I love being able to do things with the actual SVG DOM (like tweaking style from generated things). On the other, generated SVG has a nasty habit of including classes and ids used in fascinating ways (e.g. a gradient is a nest of references to refs) that will eventually conflict providing hilarious results.

@ian-r-rose the issue with trying to parse first, namespace later, is that an <svg> without that namespace isn't actually well-formed, and so might parse in exciting ways. Adding a namespace dynamically is also peril-fraught. So in this case, regex might indeed be the right way to go.

@saulshanabrook
Copy link
Member

Quick fix: Replace <[ ]*svg[ ] with <svg xmlns="http://www.w3.org/2000/svg" and hopefully this still works if it ends up there twice.

zerline added a commit to zerline/jupyterlab that referenced this issue Jun 4, 2019
zerline added a commit to zerline/jupyterlab that referenced this issue Jun 4, 2019
jasongrout added a commit that referenced this issue Jun 6, 2019
@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
@jasongrout jasongrout added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug good first issue pkg:rendermime status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants