svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.01k stars 1.07k forks source link

Transforming SVG in an <object> doesn't work on Chrome but does in Firefox #1177

Closed sjmurdoch closed 2 years ago

sjmurdoch commented 3 years ago

Bug report

Fiddle

Please see the demonstration at https://repl.it/@sjmurdoch/svgjsdemo#index.html

Explanation

When requesting to transform an SVG rectangle referred to from an <object> tag using svg.js nothing happens in Chrome (and Safari), but does with Firefox.

In Chrome and Safari, the rectangle remains unchanged when modified through svg.js. Performing the same transformation through setAttribute directly works in all of Chrome, Safari, and Firefox.

None

Fuzzyma commented 3 years ago

We dont really support object tags. However, what works in plain javascript should work in svg.js

I cant load your example though. It shows nothing

sjmurdoch commented 3 years ago

Did you click on the green "Run" button at the top?

My limited understanding is that this should work even for SVG's included in an <object> tag and I can make it work with code that I think isn't far off from what svg.js is doing. I wasn't able to work out why there's a difference.

Here's what I see in Firefox

svgjs-firefox

and Chrome

svgjs-chrome
Fuzzyma commented 3 years ago

Sorry I thought I answered already. You are right, I didnt press the run button -.- I can see, that the transform attribute is not set in Chrome. I debugged it and found, that we do a node instanceof window.Node check when creating a new svgjs wrapper. This check is neccessary for convinience because its also possible to pass a plain attr object to initialize a new node. However, Chromes implementation of the object tag seems to create a different window than the "main" window and therefore their Node-Classes are not the same. The check fails, a new Node is created and this node is changed instead (you never see it because you never attach it).

I am not sure what the specs here say but it seems that one browser is right and the other one is wrong. We might get around the issue in svg.js if we check if the plain object has no prototype but this can also go south for other reasons instead...

sjmurdoch commented 3 years ago

Thanks for investigating this. I'd be happy to submit a bug report to the relevant browser, but I don't know which is the correct behaviour either.

Fuzzyma commented 3 years ago

Haha, no worries - they will point you to the right one, if you guessed wrong :D

wuyuanyi135 commented 2 years ago

I encountered this issue today. If <object> is not supported by SVG.js, could you recommend the best practice to load a SVG file served statically?

Fuzzyma commented 2 years ago

@wuyuanyi135 you can use ajax and pass the response to the SVG constructor and add it somewhere to your page. Alternatively you can just dump the svg into the dom

wuyuanyi135 commented 2 years ago

@Fuzzyma thanks!

visionm45 commented 2 years ago

So then no workaround for the object tag? Just exploring my options and if possible that is what id like to do.

I did a bit of digging. Looks to me that Chrome is displaying nonstandard behavior. if I am reading this correctly "the object element can represent an external resource, which, depending on the type of the resource, will either be treated as an image, as a nested browsing context, or as an external resource to be processed by a plugin" seems to me to say that there should not be a nested browsing context there when a <object data="example.svg" type="image/svg+xml" ></object> is present.

also this seems to be saying the same thing.

Fuzzyma commented 2 years ago

at least for now, no fix is planned

visionm45 commented 2 years ago

after playing around with the nodeornew all it is is that check. if it returned true return node instanceof globals.window.Node ? node : create(name); all it would take would be to change that to something else. What could that be changed to that would make this work?

possibly this return node.nodeType === Node.ELEMENT_NODE ? node : create(name); not quite there but one moment

visionm45 commented 2 years ago

okay so for all of the tests that I have done this works

function nodeOrNew(name, node) { return node && node instanceof node.ownerDocument.defaultView.Node ? node : create(name); } // Adopt existing svg elements

its as if they knew we would be looking "For example, checking if a Node is a SVGElement in a different context, you can use myNode instanceof myNode.ownerDocument.defaultView.SVGElement."

here is a project that I made to test everything I could think of to be sure that it works.

i'm sure I missed something but everything works here including manipulating an svg loaded inside of an object on chrome

edit. ah well dang I downloaded the source and made the changes and it fails a lot of tests. so I guess that wont do it. Any ideas what I'm missing

these are the test results test results.txt

if you care to see the edit made to svg.js source it is here.

edit. well actually I ran the dom test and it became obvious what the problem was. I modified the function to

export function nodeOrNew (name, node) { return (node && 'ownerDocument' in node && node instanceof node.ownerDocument.defaultView.Node) ? node : create(name) }

and now all but 1 test passes. `Firefox 92.0 (Mac OS 10.15) textable.js x() sets the value of all lines FAILED Expected 201 to be 200.

@spec/spec/modules/core/textable.js:27:26 each@src/elements/Dom.js:84:13 <- src/elements/Dom.js:12:202 @spec/spec/modules/core/textable.js:26:12 Expected 201 to be 200. @spec/spec/modules/core/textable.js:27:26 each@src/elements/Dom.js:84:13 <- src/elements/Dom.js:12:202 @spec/spec/modules/core/textable.js:26:12 ` my [branch](https://github.com/visionm45/svg.js) will reflect these changes. I think I almost have it if not already. I'm just not sure what that last error is about. edit oh well actually i just ran the tests on the master branch and I got the same error. I think that I got it.
visionm45 commented 2 years ago

sorry for making so many edits to that last comment. just to be clear I believe that I did find a solution that passes all tests except for one which is unrelated. the code is

export function nodeOrNew (name, node) { return (node && 'ownerDocument' in node && node instanceof node.ownerDocument.defaultView.Node) ? node : create(name) }

and my branch reflects these changes.

I hope that this can be merged at some point.

Fuzzyma commented 2 years ago

Was the failing test failing before you made the change? Can you also test how it affects performance by running the tests in /bench?

visionm45 commented 2 years ago

yes I cloned master and ran the tests there and I got a the failing test mentioned above. okay so I ran the benchmark of master and my branch and I attached the results. I ran each test 3 times because there appeared to me to be quite a bit of variation in the results between each test. okay so i ran npm run build first then i ran the benchmarks. That is all for that correct? branch benchmark take 2.txt master benchmark take 2.txt

Fuzzyma commented 2 years ago

@visionm45 please create a pull request

visionm45 commented 2 years ago

done.