Pass all SVG attributes through #5714

gaearon · 2015-12-22T20:20:13Z

Update: this was reverted in favor of a different approach in #6243.

If any attribute is missing please file an issue and we'll add it.

All attributes defined on SVG elements will now be passed directly regardless of the whitelist.
The casing specified by user will be preserved, and setAttribute() will be used.

In the future we will remove support for the camel case aliases to the hyphenated attributes. For example, we currently map strokeWidth to stroke-width but this is now deprecated behind a warning. When we remove support for this, we can remove some of the code paths introduced in this commit.

The purpose of this change is to stop maintaining a separate SVG property config.
The config still exists for two purposes:

Allow a migration path for deprecated camelcased versions of hyphenated SVG attributes
Track special namespaced attributes (they still require a whitelist)

However it is no longer a blocker for using new non-namespaced SVG attributes, and users don't have to ask us to add them to the whitelist.

Fixes #1657

Reviewers: @spicyj @jimfb @sebmarkbage

sophiebits · 2015-12-23T00:40:06Z

This seems good.

Can we make the warning here SVG-specific? Maybe "SVG attribute clipPath is deprecated; use the original attribute name clip-path for SVG tags instead."
Instead of a new "deprecated" flag, can we just compare the attribute name in createMarkupForSVGAttribute and warn if it's different (and there's no namespace)? If I'm missing some reason that won't work, this way is fine too.

I thought I had a third thing too but I forget now.

sophiebits · 2015-12-23T00:40:21Z

src/renderers/dom/shared/DOMProperty.js

@@ -29,6 +29,7 @@ var DOMPropertyInjection = {
  HAS_NUMERIC_VALUE: 0x10,
  HAS_POSITIVE_NUMERIC_VALUE: 0x20 | 0x10,
  HAS_OVERLOADED_BOOLEAN_VALUE: 0x40,
+  IS_DEPRECATED_PROPERTY: 0x100,


If we keep this, it should be 0x80.

gaearon · 2015-12-23T14:21:49Z

Amended the commit based on your comments. I added a completely separate code path for the SVG warning so it's easy to remove later and we don't have to think about getPossibleStandardName being broken in this case.

facebook-github-bot · 2015-12-23T14:25:01Z

@gaearon updated the pull request.

facebook-github-bot · 2015-12-23T14:39:57Z

@gaearon updated the pull request.

gaearon · 2015-12-23T17:44:38Z

I updated the PR with what I believe to be the complete implementation, but I haven't tested it in the wild yet. Does it look good to go otherwise?

facebook-github-bot · 2015-12-23T17:45:45Z

@gaearon updated the pull request.

facebook-github-bot · 2015-12-23T17:48:34Z

@gaearon updated the pull request.

gaearon · 2015-12-23T17:58:42Z

One thing that concerned me is whether I should add new DOMPropertyOperations methods to

ReactPerf.measureMethods(DOMPropertyOperations, 'DOMPropertyOperations', {
  setValueForProperty: 'setValueForProperty',
  setValueForAttribute: 'setValueForAttribute',
  deleteValueForProperty: 'deleteValueForProperty',
});

at the bottom.

zpao · 2015-12-23T20:07:56Z

src/renderers/dom/shared/DOMPropertyOperations.js

+      return '';
+    }
+    var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
+        DOMProperty.properties[name] : null;


nit: 2 space indent (you have a couple of this exact 2 lines doing the same thing - maybe have a helper method?)

I was trying to follow the existing style in this file.

Here are a few places I haven't touched where indentation is the same as mine, and they also don't bother to extract the helper function:

https://github.com/gaearon/react/blob/013340a44dbf99e5855962e68688453eab324f5f/src/renderers/dom/shared/DOMPropertyOperations.js#L159-L160

https://github.com/gaearon/react/blob/013340a44dbf99e5855962e68688453eab324f5f/src/renderers/dom/shared/DOMPropertyOperations.js#L229-L230

https://github.com/gaearon/react/blob/013340a44dbf99e5855962e68688453eab324f5f/src/renderers/dom/shared/DOMPropertyOperations.js#L302-L303

I'm happy to re-indent all of them at once and add a helper for getting the property, if you'd like! Just wanted to avoid introducing inconsistencies. Also I didn't want to overgeneralize because this code will later be likely removed when we kill the deprecation path and just start treating SVG pretty much like custom components.

Ok, let's just leave it as is then 👍

All attributes defined on SVG elements will now be passed directly regardless of the whitelist. The casing specified by user will be preserved, and setAttribute() will be used. In the future we will remove support for the camel case aliases to the hyphenated attributes. For example, we currently map `strokeWidth` to `stroke-width` but this is now deprecated behind a warning. When we remove support for this we can remove some of the code paths introduced in this commit. The purpose of this change is to stop maintaining a separate SVG property config. The config still exists for two purposes: * Allow a migration path for deprecated camelcased versions of hyphenated SVG attributes * Track special namespaced attributes (they still require a whitelist) However it is no longer a blocker for using new non-namespaced SVG attributes, and users don't have to ask us to add them to the whitelist. Fixes #1657

facebook-github-bot · 2015-12-24T17:20:17Z

@gaearon updated the pull request.

gaearon · 2015-12-24T17:25:04Z

I updated the PR to merge cleanly on top of #5590 but unlike #5590 it still uses the old warning system.
I'll look into updating this PR to use devtool but this can be done separately.

In #5590 a new system was introduced for tracking dev-time warnings. This commit uses it for reporting SVG attribute deprecation warnings.

gaearon · 2015-12-24T17:54:03Z

I moved the warnings into a separate devtool to align with #5590.
I'm adding @jimfb to reviewers so he can evaluate whether I'm using it correctly.

facebook-github-bot · 2015-12-24T17:56:29Z

@gaearon updated the pull request.

sophiebits · 2015-12-25T05:58:14Z

src/renderers/dom/shared/ReactDOMComponent.js

@@ -862,6 +864,11 @@ ReactDOMComponent.Mixin = {
          DOMProperty.properties[propKey] ||
          DOMProperty.isCustomAttribute(propKey)) {
        DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey);
+      } else if (this._namespaceURI === DOMNamespaces.svg) {


Did you mean for this to be above the previous case? I think it works out the same either way, so I guess it doesn't really matter.

Document changes in #5714

kumarharsh · 2016-03-09T14:17:22Z

I see that this change is not in the 0.14.x branch. Will this be backported?

zpao · 2016-03-09T18:37:49Z

No this will not be backported.

…-attributes" This reverts commit 53dabe7, reversing changes made to 82fe64a.

SimenB · 2016-12-11T11:53:34Z

This was reverted, so could the OP be updated to reflect that? Lots of issues for SVG point to this one, it confused me before more googling led me to #6243

gaearon · 2016-12-11T12:01:56Z

Updated

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 22, 2015

sophiebits added GH Review: needs-revision and removed GH Review: review-needed labels Dec 23, 2015

sophiebits reviewed Dec 23, 2015
View reviewed changes

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Dec 23, 2015

gaearon changed the title ~~[WIP] Pass all SVG attributes through~~ Pass all SVG attributes through Dec 23, 2015

zpao reviewed Dec 23, 2015
View reviewed changes

This was referenced Dec 23, 2015

Fix #5726 #5727

Closed

Add startOffset to supported SVG Attributes #5210

Closed

Move SVG attribute deprecation warnings into a devtool

251d6c3

In #5590 a new system was introduced for tracking dev-time warnings. This commit uses it for reporting SVG attribute deprecation warnings.

gaearon self-assigned this Dec 24, 2015

gaearon mentioned this pull request Dec 24, 2015

Support SVG stroke-linejoin attribute #5726

Closed

sophiebits reviewed Dec 25, 2015
View reviewed changes

cerisier mentioned this pull request Dec 25, 2015

Add support for SVG DOM property: filter #5613

Closed

This was referenced Dec 25, 2015

SVG attributes and ARIA attributes with different pattern #5617

Closed

SVG attributes support is missing the "filter" attribute #5612

Closed

Support fontWeight svg attribute #4625

Closed

gaearon added a commit that referenced this pull request Dec 26, 2015

Merge pull request #5736 from gaearon/document-svg-passthrough

713401f

Document changes in #5714

zpao mentioned this pull request Jan 1, 2016

SVG attributes getting ignored #5763

Closed

jimfb mentioned this pull request Jan 20, 2016

React does not support svg attributes. #5889

Closed

IL55 mentioned this pull request Feb 10, 2016

svg clipPath has wrong namespace #6012

Closed

zpao added a commit to zpao/react that referenced this pull request Mar 9, 2016

Revert "Merge pull request facebook#5714 from gaearon/passthrough-svg…

08fa7fe

…-attributes" This reverts commit 53dabe7, reversing changes made to 82fe64a.

zpao added a commit to zpao/react that referenced this pull request Mar 11, 2016

Revert "Merge pull request facebook#5714 from gaearon/passthrough-svg…

9f4f88b

…-attributes" This reverts commit 53dabe7, reversing changes made to 82fe64a.

macqueen mentioned this pull request Apr 13, 2016

use initial avatars instead of 'mystery person' when gravatar not set getsentry/sentry#3043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass all SVG attributes through #5714

Pass all SVG attributes through #5714

gaearon commented Dec 22, 2015 •

edited

Loading

sophiebits commented Dec 23, 2015

sophiebits Dec 23, 2015

gaearon commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

gaearon commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

gaearon commented Dec 23, 2015

zpao Dec 23, 2015

gaearon Dec 23, 2015

zpao Dec 23, 2015

facebook-github-bot commented Dec 24, 2015

gaearon commented Dec 24, 2015

gaearon commented Dec 24, 2015

facebook-github-bot commented Dec 24, 2015

sophiebits Dec 25, 2015

kumarharsh commented Mar 9, 2016

zpao commented Mar 9, 2016

SimenB commented Dec 11, 2016

gaearon commented Dec 11, 2016

Pass all SVG attributes through #5714

Pass all SVG attributes through #5714

Conversation

gaearon commented Dec 22, 2015 • edited Loading

Update: this was reverted in favor of a different approach in #6243.

If any attribute is missing please file an issue and we'll add it.

sophiebits commented Dec 23, 2015

sophiebits Dec 23, 2015

Choose a reason for hiding this comment

gaearon commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

gaearon commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

facebook-github-bot commented Dec 23, 2015

gaearon commented Dec 23, 2015

zpao Dec 23, 2015

Choose a reason for hiding this comment

gaearon Dec 23, 2015

Choose a reason for hiding this comment

zpao Dec 23, 2015

Choose a reason for hiding this comment

facebook-github-bot commented Dec 24, 2015

gaearon commented Dec 24, 2015

gaearon commented Dec 24, 2015

facebook-github-bot commented Dec 24, 2015

sophiebits Dec 25, 2015

Choose a reason for hiding this comment

kumarharsh commented Mar 9, 2016

zpao commented Mar 9, 2016

SimenB commented Dec 11, 2016

gaearon commented Dec 11, 2016

gaearon commented Dec 22, 2015 •

edited

Loading