SVGDOM implementation (uncomplete) #1445

Fuzzyma · 2016-04-10T13:20:33Z

jsdom does not implement SVG elements atm. I would like to change that and thats why I try to kickoff the implementation here.

Its my first day contributing to jsdom so I am sure I did some mistakes. Please correct me whenever you spot something odd.
I will add commits to this PR whenever I finished an element.

The first commit only adds the SVGElement as basic implementation.

This PR is related to #1423

domenic · 2016-04-10T22:01:55Z

Thanks, this is very much appreciated! I'll try to review and see why Travis is failing.

domenic · 2016-04-10T22:04:11Z

lib/jsdom/living/nodes/SVGElement.idl

@@ -0,0 +1,6 @@
+interface SVGElement : Element {


Can you use the interface from https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement instead, commenting out the things that are not implemented yet? Also add a comment at the top of the file like // https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement, and don't forget a newline at the end of the file.

Yes, will do. However: May I ask why these interfaces are that different?

The changes are mostly additive. I believe there is a general desire to make SVG elements more like HTML elements where possible.

What to do with the interfaces which are no longer present in the draft you linked (e.g. SVGStylable)?

They can be omitted. Apparently the SVG working group thinks removing them is an acceptable change for SVG2: https://svgwg.org/svg2-draft/changes.html#types

domenic · 2016-04-10T22:05:58Z

Travis is failing because some lint errors.

This will also need some tests before it can be merged. See https://github.com/tmpvar/jsdom/blob/master/Contributing.md#web-platform-feature-tests

Fuzzyma · 2016-04-10T22:11:45Z

Yes, I saw the errors. I forgot a semicolon somewhere.
I will add tests as soon as I understand how that works.
Thanks for reviewing!

Fuzzyma · 2016-04-11T18:59:18Z

I could need a bit help with these tests.
I dont know where to start and especially where to place my tests.

domenic · 2016-04-11T19:02:37Z

You can place them in a new subdirectory of https://github.com/tmpvar/jsdom/tree/master/test/web-platform-tests/to-upstream called "svg".

Fuzzyma · 2016-04-12T20:54:52Z

How can I test objects which have no constructor?
In this case I want to write tests for the interface SVGStringList.
The normal conctructor cant be used (illegal constructor). So how to do that?

domenic · 2016-04-12T20:55:40Z

Well, are there any APIs that return a SVGStringList on the platform? If so, use them. If not, then there's no reason to add SVGStringList :).

Fuzzyma · 2016-04-12T21:01:25Z

Well yes. SVGTests returns a stringlist (SVGGraphics implements SVGTests). But I feel uncomfortable using such a long way to get to the strings instead of just testing them straight forward

domenic · 2016-04-12T21:02:30Z

Nah, it's fine :)

Fuzzyma · 2016-04-12T21:14:00Z

This is from the specs about requiredExtensions:

If the attribute is not present, then it implicitly evaluates to "true". If a null string or empty string value is given to attribute ‘requiredExtensions’, the attribute evaluates to "false".

https://svgwg.org/svg2-draft/struct.html#RequiredExtensionsAttribute

I checked. In FF an empty Stringlist is returned. No false and no true. I am not sure which is the correct behavior.

PS: Sorry for bothering you with that :D

domenic · 2016-04-12T21:19:08Z

No problem! I think the confusion there is that it is talking about the requiredExtensions content attribute, i.e. the thing in the SVG markup itself: requiredExtensions="". The requiredExtensions IDL attribute is defined here: https://svgwg.org/svg2-draft/types.html#__svg__SVGTests__requiredExtensions

This gets kind of complicated, and needs the rules in https://svgwg.org/svg2-draft/types.html#TermReflect :-/.

domenic · 2016-04-12T21:31:39Z

The "evaluates to true/false" bit is presumably about how other parts in the spec will say something like "if the requiredExtensions attribute for this evaluates to true", which is a spec-ese way of saying "if all the required extensions are supported." I think.

Fuzzyma · 2016-04-13T00:12:20Z

I read through this and I think I understand it. However one question remains: how does the Object knows which attribute it is reflecting. Same goes for readonly. How does e.g. a list knows if it should be readonly?

Sebmaster · 2016-04-13T01:33:25Z

How does e.g. a list knows if it should be readonly?

It doesn't need to. readonly doesn't mean you can't modify the list, it means you can't modify the reference to the list as in:

obj.list = new List(); // illegal
obj.list.clear(); // totally fine

The webidl2js layer ensures that readonly are enforced automatically, so you won't have to explicitly pay attention to that.

Fuzzyma · 2016-04-13T01:55:50Z

That makes totally sense. Thanks.
Left is the question about the reflectable List. Is this also covered from the layer?

Sebmaster · 2016-04-13T02:14:48Z

Do you mean how we know which properties are also reflected as attributes?

That's sadly not in the webidl language, but rather hidden in the spec language, so actual reading required 😕

Chromium/WebKit has their own proprietary attribute extension which introduces [Reflect] as an extra argument into webidl as you can see here. This argument is also supported by webidl2js, so with some creative merging of the spec IDL, spec language and chromium idl, this is also taken care of mostly.

Fuzzyma · 2016-04-13T02:43:42Z

no. I know that the specs lay out which attributes have to be reflected.
I will try an exampel:
The attribute requiredExtensions has to be reflected. It is a StringList so adding one item to the list whould also add one Extension to the requiredExtensions attribute and the other way round.
My question is where i implement this behavior. The List is aware of beeing edited (insertItemBefore, removeItem... called) but it has no knowledge about which attribute it is representing. So where is the logic placed for actual updating the dom?

edemaine · 2016-07-28T16:06:17Z

Hi folks! I'm very interested in getting this patch into jsdom, so that I can use svg.js to build SVG in Node (see svgdotjs/svg.js#464 as well as this article). Anything I can do to help? More test writing or are there other issues?

Fuzzyma · 2016-07-28T16:24:42Z

hey edemaine, I actually felt a bit lost in this huge specifications with all this reflection stuff. However - feel free to add your knowledge here. I would be really happy about that!

domenic · 2016-07-28T17:16:19Z

I think the main things left to do for landing this initial patch are:

Make sure it conforms to the SVG2 spec. (See above discussions about attribute reflection and things that have been removed above.)
Add to-upstream tests for every new piece of code, in web platform tests format. See Contributing.md for more details.

Feel free also to implement a subset, e.g. a basic SVGSVGElement class in a single patch, instead of @Fuzzyma's SVGTests and SVGStringList. That might be an easier patch to land.

edemaine · 2016-08-05T20:11:06Z

OK, I finally read through this discussion. Fuzzyma's main question seems to be how to make something like elt.requiredExtensions.appendItem('extension') tell elt to update its requiredExtensions. I don't quite understand the issue in this case, because elt.requiredExtensions presumably is just a pointer to a single SVGStringList object, so changing that object will affect elt (and it's not like we're actually rendering SVG so there's no UI to update in this case). So my sense is that reflection is "free" as far as dom.js is concerned. (If this were an issue, we could subclass SVGStringList to store the element it belongs to and pass updates along... but this doesn't seem necessary.) Am I missing something?

Also, I'm a little new to Github -- is the proper way to extend a pull request to clone the new repository, make my own commits, and then submit another pull request?

domenic · 2016-08-06T03:49:54Z

Also, I'm a little new to Github -- is the proper way to extend a pull request to clone the new repository, make my own commits, and then submit another pull request?

Yeah, that sounds good.

Don't take this to serious. I guess it has some major flaws

tsiege · 2016-12-20T21:09:08Z

Hello! I'm currently working on a project that I'd prefer to use jsdom over phantomjs for. What's the status on this PR/what could I do to help move it along?

edemaine · 2016-12-20T21:20:21Z

Sorry, I haven't had time to look more / work on this yet. But looks like @Fuzzyma has had some time to write additional tests. Perhaps he could report on what still remains?

tsiege · 2016-12-20T21:48:04Z

@edemaine no worries. It looks like @nhunzaker is also working on this now. Maybe we could all figure out an MVP for this PR since the W3 spec for SVGs is huge and divide up the work? I'm particularly interesting in seeing #getBBox get implemented which relies on SVGRect, but some of the other classes that were started in here would be nice as well.

CC @Fuzzyma @domenic @Sebmaster

nhunzaker · 2016-12-21T05:22:39Z

Digging more into this, I think figuring out a baseline to unblock all of our specific desires is a great starting point.

I simply want SVGElement to be defined on the global namespace with basic DOM operations. However I'd be happy to help collaborate on other pieces.

Fuzzyma · 2016-12-21T09:40:26Z

As far as I can remember my code, the "SVGElement is defined" goal was already achieved^^

edemaine · 2016-12-21T14:45:57Z

@nhunzaker Agreed, it'd be nice to release this in pieces, if possible. Personally, I just want enough for svg.js to run, which I think is very little. Then we can break up the remaining work by sections of the SVG DOM spec.

graingert · 2017-10-10T16:07:26Z

@Fuzzyma can you mark this is fixing #2001 ?

Fuzzyma · 2017-10-10T16:21:00Z

Might fix #2001

PS: I am not sure if that is the right way :D

TimothyGu · 2017-10-10T17:22:27Z

I'm working on creating a cleaned-up version of the SVG-support PRs. Hopefully will finish today!

Closes jsdom#1445 by superceding it; also supercedes jsdom#1946. Fixes jsdom#1423. Fixes jsdom#1528. Fixes jsdom#1938. Fixes jsdom#2001. Includes only the <svg> element, but this brings along with it the following interfaces: * SVGElement * SVGGraphicsElement * SVGSVGElement * SVGTests * SVGAnimatedString * SVGNumber * SVGStringList

Fuzzyma added 2 commits April 10, 2016 15:14

SVGElement implementation, first draft

129a438

implementation ownerSVGElement, viewportElement

326ab4a

domenic reviewed Apr 10, 2016
View reviewed changes

domenic added the needs tests label Apr 10, 2016

Fuzzyma added 4 commits April 11, 2016 02:06

fix lint and interface, implement closestMultiple

ab14ee1

SVGGraphicsElement, SVGSVGElement, SVGTests

6d0e945

Removed feature which is not implemented yet

63ffad9

SVGStringList, fixed not implemented require

3be5a80

kevva mentioned this pull request Sep 14, 2016

Add support for SVG images kevva/resize-img#1

Closed

Fuzzyma mentioned this pull request Oct 10, 2016

Separate out DOM-dependent code from svg.js svgdotjs/svg.js#528

Closed

niieani mentioned this pull request Nov 19, 2016

SVGElement is not properly polyfilled aurelia/pal-nodejs#17

Closed

nhunzaker mentioned this pull request Dec 19, 2016

SVGElement is not implemented #1423

Closed

Additional changes made for the PR

8b31a89

Don't take this to serious. I guess it has some major flaws

domenic force-pushed the master branch from 8908e70 to 2161116 Compare February 26, 2017 05:47

herodrigues mentioned this pull request Aug 27, 2017

Unable to require() to use with Jest c3js/c3#2129

Closed

TimothyGu mentioned this pull request Oct 10, 2017

Last release breaks the build when using it together with React DOM #2001

Closed

TimothyGu mentioned this pull request Oct 11, 2017

A Better SVG implementation #2011

Merged

domenic closed this in e2ea031 Nov 10, 2017

marinho mentioned this pull request Jan 25, 2018

SVGPathElement (and similars) not implemented #2126

Closed

SVGDOM implementation (uncomplete) #1445

SVGDOM implementation (uncomplete) #1445

Conversation

Fuzzyma commented Apr 10, 2016

domenic commented Apr 10, 2016

domenic Apr 10, 2016

Choose a reason for hiding this comment

Fuzzyma Apr 10, 2016

Choose a reason for hiding this comment

domenic Apr 10, 2016

Choose a reason for hiding this comment

Fuzzyma Apr 10, 2016

Choose a reason for hiding this comment

domenic Apr 10, 2016

Choose a reason for hiding this comment

domenic commented Apr 10, 2016

Fuzzyma commented Apr 10, 2016

Fuzzyma commented Apr 11, 2016

domenic commented Apr 11, 2016

Fuzzyma commented Apr 12, 2016

domenic commented Apr 12, 2016

Fuzzyma commented Apr 12, 2016

domenic commented Apr 12, 2016

Fuzzyma commented Apr 12, 2016

domenic commented Apr 12, 2016

domenic commented Apr 12, 2016

Fuzzyma commented Apr 13, 2016

Sebmaster commented Apr 13, 2016

Fuzzyma commented Apr 13, 2016

Sebmaster commented Apr 13, 2016

Fuzzyma commented Apr 13, 2016

edemaine commented Jul 28, 2016

Fuzzyma commented Jul 28, 2016

domenic commented Jul 28, 2016

edemaine commented Aug 5, 2016

domenic commented Aug 6, 2016

tsiege commented Dec 20, 2016

edemaine commented Dec 20, 2016

tsiege commented Dec 20, 2016

nhunzaker commented Dec 21, 2016

Fuzzyma commented Dec 21, 2016

edemaine commented Dec 21, 2016

graingert commented Oct 10, 2017

Fuzzyma commented Oct 10, 2017 • edited

TimothyGu commented Oct 10, 2017

Fuzzyma commented Oct 10, 2017 •

edited