Don't append content in rendered output upon rerendering. #6513
Conversation
|
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
|
Thanks, we should check to make sure that updatable outputs continue to work without needlessly creating/destroying DOM nodes. |
|
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. |
|
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); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is it easier to just do this.node.textContent=''?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, I was looking at the same post. I otherwise don't have any reason to chose one method over the other.
|
I'm trying to track down the commit that introduced this -- it doesn't look like it was #6304. |
|
The culprit is #6426, which switched the text renderer to append I still think this is a reasonable fix, since calling |
|
Agreed, thanks! |
Ah, sorry, my mistake. Thanks for finding this! |
Bumps [jupyterlab](https://github.com/jupyterlab/jupyterlab) from 1.0.0a5 to 1.0.0a10. Been meaning to update as the newline rendering was driving me crazy, since fixed in jupyterlab/jupyterlab#6513. - [Release notes](https://github.com/jupyterlab/jupyterlab/releases/tag/v1.0.0a10) - [Changelog](https://github.com/jupyterlab/jupyterlab/blob/master/docs/source/getting_started/changelog.rst) - [Commits](jupyterlab/jupyterlab@v1.0.0a5...v1.0.0a10)
|
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. |
jasongrout
added
the
status:resolved-locked
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
RenderedCommonif they were updating existing DOM nodes.