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

Don't append content in rendered output upon rerendering. #6513

Merged
merged 3 commits into from Jun 8, 2019

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 8, 2019

References

Fixes #6497, fixes #6505.

I would consider this a bug in the rendermime library that was uncovered by some of the less-aggressive updating of outputs in #6304.

Code changes

Empties any existing DOM node content in our existing renderers before re-rendering.

User-facing changes

Fixes re-displaying of already displayed data.

Backwards-incompatible changes

Possible backwards-incompatible behavior of re-rendering for subclasses of RenderedCommon if they were updating existing DOM nodes.

@ian-r-rose ian-r-rose added this to the 1.0 milestone Jun 8, 2019
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jun 8, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jun 8, 2019

Thanks, we should check to make sure that updatable outputs continue to work without needlessly creating/destroying DOM nodes.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 8, 2019

The VDOM renderer already overrides this method, which is, I believe, the only place where this comes up in core JLab. I'd be surprised if many subclasses of this which use this (incorrect, IMHO) behavior exist in the wild.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 8, 2019

Yes, a quick look over our existing renderers indicates that this really only comes up with the plain text renderer.

while (this.node.firstChild) {
this.node.removeChild(this.node.firstChild);
}

Copy link
Contributor

@jasongrout jasongrout Jun 8, 2019

Is it easier to just do this.node.textContent=''?

Copy link
Contributor

@jasongrout jasongrout Jun 8, 2019

huh, https://stackoverflow.com/questions/3955229/remove-all-child-elements-of-a-dom-node-in-javascript indicates your method (or maybe equivalently this.node.firstChild.remove()?) may be faster, and possibly more correct if there is, for example, svg content.

Copy link
Member Author

@ian-r-rose ian-r-rose Jun 8, 2019

Yes, I was looking at the same post. I otherwise don't have any reason to chose one method over the other.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 8, 2019

I'm trying to track down the commit that introduced this -- it doesn't look like it was #6304.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 8, 2019

The culprit is #6426, which switched the text renderer to append <pre> nodes rather than setting the inner HTML.

I still think this is a reasonable fix, since calling renderModel on a mimebundle should return the same rendered content regardless of the starting node, in my opinion.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 8, 2019

Agreed, thanks!

@blink1073 blink1073 merged commit 8d36127 into jupyterlab:master Jun 8, 2019
9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 8, 2019

The culprit is #6426, which switched the text renderer to append <pre> nodes rather than setting the inner HTML.

Ah, sorry, my mistake. Thanks for finding this!

weiji14 added a commit to weiji14/deepbedmap that referenced this issue Jun 21, 2019
@lock
Copy link

@lock 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants