Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add full support for SVG 1.1 #938

Closed
wants to merge 3 commits into from
Closed

Add full support for SVG 1.1 #938

wants to merge 3 commits into from

Conversation

brycekahle
Copy link

Adds all the missing elements from http://www.w3.org/TR/SVG/eltindex.html
Adds all the missing attributes, except for the event related ones (starting with 'on') from http://www.w3.org/TR/SVG/attindex.html

Updates documentation.

Adds support for namespaced attributes that need setAttributeNS

I have signed the CLA

@vjeux
Copy link
Contributor

vjeux commented Jan 19, 2014

Wow thanks!

We hold off on adding all of them in the past because it's a lot of bytes. Do you mind telling us what's the gzipped size of react.min.js before and after your change?

@brycekahle
Copy link
Author

Before: 28667
After: 31723

The property name mapping is likely most of that. Using flags for naming rules might save a lot of space since there are only a couple different methods. There would be a performance tradeoff most likely though. Maybe do some sort of name transform result caching?

  • Lowercase
  • Dasherize (alignmentBaseline to alignment-baseline)
  • Preserve

@brycekahle
Copy link
Author

I tested a method where I used flags to determine naming rules. It reduced the size down to 30979.

@brycekahle
Copy link
Author

I could pull all styling attributes out since those can be set via CSS or the style attribute. That might help reduce the size even more.

@syranide
Copy link
Contributor

Something to keep in mind is that this reserves a lot of names for JSX, that people most likely have never heard of or expect to have been reserved. It kind of feels like this should be namespaced using React.DOM.svg.* or something like that (which would work if my namespacing PR is accepted).

To me it also kind of seems like the best approach to something big like this, that most likely won't use, would be if it could be injected instead.

@brycekahle
Copy link
Author

@syranide Only the element names would have conflicts with naming. Since the elements are all camelCased and it is good practice to uppercase your own components, I don't see any real conflict. IMHO I don't think it is worth removing almost HTML/SVG markup parity for such an easily avoidable scenario.

feTurbulence: false,
filter: false,
font: false,
'font-face': false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that this even works? I'm not sure if the transform actually will parse hyphenated tags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And while it may work here, the transform step definitely doesn't allow hyphenated nodes - <font-face> gets transformed to React.DOM.font-face.

@zpao
Copy link
Member

zpao commented Jan 20, 2014

@syranide - I like the idea of injection. We need to make the transform step injectible though too to make this really viable. jsx --with-svg src dest or something like that. I don't know that it'll happen right now though...

I guess we should probably do this as-is for the time being, even though I'm not wild about the noise. So a couple things I'd like to see:

  • tests for the nsAttr setting
  • see if we can get the post-gzip size down (a 10% increase isn't trivial)
  • some indication that these changes are compatible with closure compiler advanced

@syranide
Copy link
Contributor

@zpao I guess, if we still require the docblock, it would perhaps make sense to just make it svg: true or similar. Having it as a cli-option would be bad I feel (if anything using SVG, all files must be using it, and you would have to know it).

@brycekahle
Copy link
Author

I updated the PR by removing all the attributes that can be set via CSS. That is everything listed here http://www.w3.org/TR/SVG/propidx.html

   raw     gz Sizes
391219  77854 build/JSXTransformer.js
1127520196962 build/react-test.js
545037 112374 build/react-with-addons.js
116769  33033 build/react-with-addons.min.js
510276 104984 build/react.js
109797  31095 build/react.min.js

@zpao
Copy link
Member

zpao commented Jan 23, 2014

In addition to everything I mentioned above, I would like you to actually build and use this. It's apparent that you haven't.

TypeError: DOMProperty.mustUseNamespacedAttribute is undefined
TypeError: DOMProperty.getAttributeNamespace is undefined

@Frozenlock
Copy link

I don't know if this PR still has a future, but I would really like a complete support of SVG!

@brycekahle
Copy link
Author

@ericflo If you want to help with the tasks outlined in this PR, I would welcome patches to my fork to finish this off.

@plievone
Copy link
Contributor

#1009 may split DefaultDOMPropertyConfig to HTMLDOMPropertyConfig and SVGDOMPropertyConfig, so they can be made injectable. And these various lists could be compressed by using createObjectFrom, merge and similar?

@mhart
Copy link

mhart commented Apr 7, 2014

Would love to get access to some of the missing SVG elements somehow - whether it's this PR or via #1009 - any progress updates on either of these?

@zpao
Copy link
Member

zpao commented Apr 7, 2014

#1009 isn't going to add anything, just reorganize some code.

@mhart
Copy link

mhart commented Apr 7, 2014

@zpao hence the "via" in my comment - I'm assuming that #1009 will make it easier to add the SVG properties, etc required.

@zpao
Copy link
Member

zpao commented Apr 7, 2014

Won't really make it easier, just slightly more clear where they go.

@mhart
Copy link

mhart commented Apr 7, 2014

Ah ok, maybe change the description then - the first sentence literally says "This makes it a little easier to add SVG properties."

@vjeux
Copy link
Contributor

vjeux commented Apr 8, 2014

Ping, what's the status here?

@yayitswei
Copy link

+1, need support for the <image> tag.

@syranide
Copy link
Contributor

syranide commented May 9, 2014

#760 is now decided (JSX member expressions, <foo.bar.baz />), but I'm not sure if it feels like a great fit, you would be required to prefix all tags as <svg.polygon /> or <s.polygon /> (if you feel like mapping it like that). Which many would probably oppose, but it does keep it neatly separated.

But personally I feel like there are just too many generic SVG tags (image, text, animate, etc) to make them default. Unless we definitively settle on <UpperCase />-style for all of our own non-HTML components. Though it will become an issue if React ever wants to be something more than just for HTML as-is.

@yayitswei
Copy link

For what it's worth here's my branch that adds the <image> tag rebased off the current master: https://github.com/yayitswei/react/tree/image-tag

Works well enough for me, but I'd love to see proper SVG support at some point.

@JustinTulloss
Copy link

I forked this and got it up to date with the latest, diff is here: JustinTulloss/react@facebook:master...svg-support

It still doesn't work correctly with namespaced attributes and I'm not going to spend the time right now to fix it, but if somebody else wants to put in any effort my branch is at least a little more up to date and works at all (the original pull request wasn't quite complete).

@sophiebits
Copy link
Collaborator

Going to close this out as it's broken in its current form. Happy to look at new PRs, though we'll need a good answer to what to do with namespaced attributes.

@sophiebits sophiebits closed this Jun 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants