Clone without assigning new IDs

phbernard commented 3 years ago

This PR is related to https://github.com/svgdotjs/svg.js/issues/1160 .

Currently, Dom.clone() (so Svg.clone(), Container.clone()...) assigns new IDs to cloned elements. This is usually what the user expects, since not assigning new IDs could lead to unexpected, hard-to-track behaviors. For example, if clones are transformed independently and displayed together, they might "share" some properties due to IDs clash.

However, when the user knows what he does, not assigning new IDs on clone can be an easy workaround to https://github.com/svgdotjs/svg.js/issues/1160 . This is because Dom.clone() does not update all references (eg. fill="url(#someId)") when assigning new IDs.

This PR introduces a new boolean optional parameter assignNewIds to Dom.clone():

true (default): No change in behavior. Which means this PR does not break anything.
false: IDs are re-used as is in the returned clone. No more broken references... but risk of ID clash.

TypeScript .d.ts file was updated accordingly.

coveralls commented 3 years ago

Coverage increased (+0.0001%) to 99.889% when pulling 04bbe3607beaf4485ddbda273edf48c03fd3fbe9 on Illustratious:master into 99e176f76332b47a6c026dbd9dcf7742191b8b29 on svgdotjs:master.

Fuzzyma commented 3 years ago

References cant be changed because they always link to valid targets even after clone. The old elements are still in the page and references in the cloned element still refer to those. Changing that will often go against what the user expects.

What do you want with cloned elements that have an already used id? You cant insert them into the dom like this

phbernard commented 3 years ago

I realize my usage of SVG.js is unusual. The fact that this issue has not been reported until now is a sign of this. You can perfectly ignore this PR if you think it brings more confusion than clarity.

Two remarks:

1) The old elements are not always in the page. You can remove the original SVG from the DOM while keeping the clone. See for example https://jsfiddle.net/pbernard/1o56prkb/52/ . In addition, unless I missed something in the docs (which I obviously read from introduction to conclusion :) ), nothing warns the user about these loose dependencies. So he could rightfully wonder how originalSvg.remove() can affect clonedSvg somewhere else in the page.

2) The cloned SVG contains unreferenced elements, because of the ID allocation. This is harmless, but a bit weird. I'm using SVG.js to modify these kind of elements. I was surprised that changeStuff(originalSvg) was working but not changeStuff(clonedSvg).

When investigating https://github.com/svgdotjs/svg.js/issues/1160 , my first intent was to make DOM.clone() update references such as url(#someId). But that would lead to a much more complex implementation (think: DOM.clone(deep = true, updateReferences = false)). So I end up suggesting DOM.clone(deep = true, assignNewIds = true) instead :)

Fuzzyma commented 2 years ago

@phbernard sorry for the late reply. Actually you made a quite convincing argument. And since it's an optional parameter I don't see any harm with it.

Tbh the use case to have a cloned svg in memory and the original removed is a bit weird to me because you always have the original in the memory as well and could just reappend that one. However, I am sure there are other usecases

edemaine commented 2 years ago

FWIW, I just ran into a desire for exactly this. My use case is:

My app interactively renders SVG according to user inputs. I don't want to mess this SVG up.
User clicks "Download" button. I clone the SVG and add some extra necessary data (e.g. add <style> tag from document root) and do some other sanitization (e.g. changing colors from hsl to rgb for better support), then use .svg() to write an SVG file.
Then I remove the copy.

This approach normally works well for me, but now that I'm using masks, the clone has new <mask> ids but the mask="url(...)" URLs are the old ones. This option would work for me, as the copy is just for temporary manipulation. Alternatively, an option to rewrite urls to the new ids (or even a way to return the old-to-new id mapping) would work for my purposes.

Currently I'm just using copy = SVG(svg.node.cloneNode(true)) as a workaround (essentially duplicating the relevant code of clone).

cbn-falias commented 1 year ago

When exporting the SVG you can pass a "export modifier" function which lets you modify/remove any node.

I guess it highly depends on the modifications one has to do but for me it's exactly what I was looking for, without running in any of those clone issues.

Example code:

const exportedSvgString = mySvg.svg(node => {
  // reset viewbox on root svg-node
  if (node.hasClass('root')) {
    node.viewbox(0,0,100,100);
  }

  // remove nodes I don't want to export
  if (node.hasClass('selection-overlay')) {
    return false;
  }
});

Fuzzyma commented 1 year ago

Sorry for the long wait!

svgdotjs / svg.js

Clone without assigning new IDs #1161