SVGDOM implementation (uncomplete)

[Fuzzyma]

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]

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

[domenic]

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.

[Fuzzyma]

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

[domenic]

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

[Fuzzyma]

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

[domenic]

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]

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]

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]

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

[domenic]

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]

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]

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]

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]

Nah, it's fine :)

[Fuzzyma]

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]

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]

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]

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]

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]

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

[Sebmaster]

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]

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]

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]

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]

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]

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]

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.

[tsiege]

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]

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]

@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]

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]

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

[edemaine]

@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]

@Fuzzyma can you mark this is fixing #2001 ?

[Fuzzyma]

Might fix #2001

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

[TimothyGu]

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