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.

Sign up for GitHub

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

Jump to bottom

Pass all SVG attributes through #5714

Merged
merged 4 commits into from
Dec 25, 2015
Merged

Pass all SVG attributes through #5714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
232a47a
Pass SVG attributes through
gaearon Dec 23, 2015
251d6c3
Move SVG attribute deprecation warnings into a devtool
gaearon Dec 24, 2015
f27e3aa
Move the specific else if clause up
gaearon Dec 25, 2015
98a7100
Use JSX in the new tests
gaearon Dec 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/renderers/dom/client/__tests__/ReactDOMSVG-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,45 @@ describe('ReactDOMSVG', function() {
ReactDOMServer = require('ReactDOMServer');
});

it('creates initial markup for known hyphenated attributes', function() {
var markup = ReactDOMServer.renderToString(
<svg clip-path="url(#starlet)" />
);
expect(markup).toContain('clip-path="url(#starlet)"');
});

it('creates initial markup for camel case attributes', function() {
var markup = ReactDOMServer.renderToString(
<svg viewBox="0 0 100 100" />
);
expect(markup).toContain('viewBox="0 0 100 100"');
});

it('deprecates camel casing of hyphenated attributes', function() {
spyOn(console, 'error');
var markup = ReactDOMServer.renderToString(
<svg clipPath="url(#starlet)" />
);
expect(markup).toContain('clip-path="url(#starlet)"');
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain('clipPath');
expect(console.error.argsForCall[0][0]).toContain('clip-path');
});

it('creates initial markup for unknown hyphenated attributes', function() {
var markup = ReactDOMServer.renderToString(
<svg the-word="the-bird" />
);
expect(markup).toContain('the-word="the-bird"');
});

it('creates initial markup for unknown camel case attributes', function() {
var markup = ReactDOMServer.renderToString(
<svg theWord="theBird" />
);
expect(markup).toContain('theWord="theBird"');
});

it('creates initial namespaced markup', function() {
var markup = ReactDOMServer.renderToString(
<svg>
Expand Down
47 changes: 47 additions & 0 deletions src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,31 @@ var DOMPropertyOperations = {
return name + '=' + quoteAttributeValueForBrowser(value);
},

/**
* Creates markup for an SVG property.
*
* @param {string} name
* @param {*} value
* @return {string} Markup string, or empty string if the property was invalid.
*/
createMarkupForSVGAttribute: function(name, value) {
if (__DEV__) {
ReactDOMInstrumentation.debugTool.onCreateMarkupForSVGAttribute(name, value);
}
if (!isAttributeNameSafe(name) || value == null) {
return '';
}
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

if (propertyInfo) {
// Migration path for deprecated camelCase aliases for SVG attributes
var { attributeName } = propertyInfo;
return attributeName + '=' + quoteAttributeValueForBrowser(value);
} else {
return name + '=' + quoteAttributeValueForBrowser(value);
}
},

/**
* Sets the value for a property on a node.
*
Expand Down Expand Up @@ -183,6 +208,18 @@ var DOMPropertyOperations = {
}
},

setValueForSVGAttribute: function(node, name, value) {
if (__DEV__) {
ReactDOMInstrumentation.debugTool.onSetValueForSVGAttribute(node, name, value);
}
if (DOMProperty.properties.hasOwnProperty(name)) {
// Migration path for deprecated camelCase aliases for SVG attributes
DOMPropertyOperations.setValueForProperty(node, name, value);
} else {
DOMPropertyOperations.setValueForAttribute(node, name, value);
}
},

/**
* Deletes the value for a property on a node.
*
Expand Down Expand Up @@ -217,6 +254,16 @@ var DOMPropertyOperations = {
}
},

deleteValueForSVGAttribute: function(node, name) {
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
DOMPropertyOperations.deleteValueForProperty(node, name);
} else {
node.removeAttribute(name);
}
},

};

ReactPerf.measureMethods(DOMPropertyOperations, 'DOMPropertyOperations', {
Expand Down
13 changes: 13 additions & 0 deletions src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,8 @@ ReactDOMComponent.Mixin = {
if (propKey !== CHILDREN) {
markup = DOMPropertyOperations.createMarkupForCustomAttribute(propKey, propValue);
}
} else if (this._namespaceURI === DOMNamespaces.svg) {
markup = DOMPropertyOperations.createMarkupForSVGAttribute(propKey, propValue);
} else {
markup = DOMPropertyOperations.createMarkupForProperty(propKey, propValue);
}
Expand Down Expand Up @@ -858,6 +860,11 @@ ReactDOMComponent.Mixin = {
// listener (e.g., onClick={null})
deleteListener(this, propKey);
}
} else if (this._namespaceURI === DOMNamespaces.svg) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

DOMPropertyOperations.deleteValueForSVGAttribute(
getNode(this),
propKey
);
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
Expand Down Expand Up @@ -924,6 +931,12 @@ ReactDOMComponent.Mixin = {
propKey,
nextProp
);
} else if (this._namespaceURI === DOMNamespaces.svg) {
DOMPropertyOperations.setValueForSVGAttribute(
getNode(this),
propKey,
nextProp
);
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
Expand Down
9 changes: 9 additions & 0 deletions src/renderers/dom/shared/ReactDOMDebugTool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
'use strict';

var ReactDOMUnknownPropertyDevtool = require('ReactDOMUnknownPropertyDevtool');
var ReactDOMSVGDeprecatedAttributeDevtool =
require('ReactDOMSVGDeprecatedAttributeDevtool');

var warning = require('warning');

Expand Down Expand Up @@ -53,14 +55,21 @@ var ReactDOMDebugTool = {
onCreateMarkupForProperty(name, value) {
emitEvent('onCreateMarkupForProperty', name, value);
},
onCreateMarkupForSVGAttribute(name, value) {
emitEvent('onCreateMarkupForSVGAttribute', name, value);
},
onSetValueForProperty(node, name, value) {
emitEvent('onSetValueForProperty', node, name, value);
},
onSetValueForSVGAttribute(node, name, value) {
emitEvent('onSetValueForSVGAttribute', node, name, value);
},
onDeleteValueForProperty(node, name) {
emitEvent('onDeleteValueForProperty', node, name);
},
};

ReactDOMDebugTool.addDevtool(ReactDOMUnknownPropertyDevtool);
ReactDOMDebugTool.addDevtool(ReactDOMSVGDeprecatedAttributeDevtool);

module.exports = ReactDOMDebugTool;
37 changes: 0 additions & 37 deletions src/renderers/dom/shared/SVGDOMPropertyConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,46 +23,19 @@ var NS = {
var SVGDOMPropertyConfig = {
Properties: {
clipPath: MUST_USE_ATTRIBUTE,
cx: MUST_USE_ATTRIBUTE,
cy: MUST_USE_ATTRIBUTE,
d: MUST_USE_ATTRIBUTE,
dx: MUST_USE_ATTRIBUTE,
dy: MUST_USE_ATTRIBUTE,
fill: MUST_USE_ATTRIBUTE,
fillOpacity: MUST_USE_ATTRIBUTE,
fontFamily: MUST_USE_ATTRIBUTE,
fontSize: MUST_USE_ATTRIBUTE,
fx: MUST_USE_ATTRIBUTE,
fy: MUST_USE_ATTRIBUTE,
gradientTransform: MUST_USE_ATTRIBUTE,
gradientUnits: MUST_USE_ATTRIBUTE,
markerEnd: MUST_USE_ATTRIBUTE,
markerMid: MUST_USE_ATTRIBUTE,
markerStart: MUST_USE_ATTRIBUTE,
offset: MUST_USE_ATTRIBUTE,
opacity: MUST_USE_ATTRIBUTE,
patternContentUnits: MUST_USE_ATTRIBUTE,
patternUnits: MUST_USE_ATTRIBUTE,
points: MUST_USE_ATTRIBUTE,
preserveAspectRatio: MUST_USE_ATTRIBUTE,
r: MUST_USE_ATTRIBUTE,
rx: MUST_USE_ATTRIBUTE,
ry: MUST_USE_ATTRIBUTE,
spreadMethod: MUST_USE_ATTRIBUTE,
stopColor: MUST_USE_ATTRIBUTE,
stopOpacity: MUST_USE_ATTRIBUTE,
stroke: MUST_USE_ATTRIBUTE,
strokeDasharray: MUST_USE_ATTRIBUTE,
strokeLinecap: MUST_USE_ATTRIBUTE,
strokeOpacity: MUST_USE_ATTRIBUTE,
strokeWidth: MUST_USE_ATTRIBUTE,
textAnchor: MUST_USE_ATTRIBUTE,
transform: MUST_USE_ATTRIBUTE,
version: MUST_USE_ATTRIBUTE,
viewBox: MUST_USE_ATTRIBUTE,
x1: MUST_USE_ATTRIBUTE,
x2: MUST_USE_ATTRIBUTE,
x: MUST_USE_ATTRIBUTE,
xlinkActuate: MUST_USE_ATTRIBUTE,
xlinkArcrole: MUST_USE_ATTRIBUTE,
xlinkHref: MUST_USE_ATTRIBUTE,
Expand All @@ -73,9 +46,6 @@ var SVGDOMPropertyConfig = {
xmlBase: MUST_USE_ATTRIBUTE,
xmlLang: MUST_USE_ATTRIBUTE,
xmlSpace: MUST_USE_ATTRIBUTE,
y1: MUST_USE_ATTRIBUTE,
y2: MUST_USE_ATTRIBUTE,
y: MUST_USE_ATTRIBUTE,
},
DOMAttributeNamespaces: {
xlinkActuate: NS.xlink,
Expand All @@ -94,23 +64,16 @@ var SVGDOMPropertyConfig = {
fillOpacity: 'fill-opacity',
fontFamily: 'font-family',
fontSize: 'font-size',
gradientTransform: 'gradientTransform',
gradientUnits: 'gradientUnits',
markerEnd: 'marker-end',
markerMid: 'marker-mid',
markerStart: 'marker-start',
patternContentUnits: 'patternContentUnits',
patternUnits: 'patternUnits',
preserveAspectRatio: 'preserveAspectRatio',
spreadMethod: 'spreadMethod',
stopColor: 'stop-color',
stopOpacity: 'stop-opacity',
strokeDasharray: 'stroke-dasharray',
strokeLinecap: 'stroke-linecap',
strokeOpacity: 'stroke-opacity',
strokeWidth: 'stroke-width',
textAnchor: 'text-anchor',
viewBox: 'viewBox',
xlinkActuate: 'xlink:actuate',
xlinkArcrole: 'xlink:arcrole',
xlinkHref: 'xlink:href',
Expand Down
Loading