Don't set children attribute on SVG nodes #6164
Conversation
| @@ -912,20 +914,21 @@ ReactDOMComponent.Mixin = { | |||
| deleteListener(this, propKey); | |||
| } | |||
| } else if (isCustomComponent(this._tag, nextProps)) { | |||
| if (propKey === CHILDREN) { | |||
| nextProp = null; | |||
There was a problem hiding this comment.
I was not sure what is the point of setting nextProp to null here. It caused removeAttribute() call but it’s not like this attribute existed in the first place? Since this didn’t break any tests (:wink::wink:), I just put it behind an if.
|
This is similar to #5093 so I would appreciate if @jimfb, @zpao and @sebmarkbage could also take a look. I can remove the change to web component code and keep it as |
|
We probably also want to skip dangerouslySetInnerHTML. Maybe suppressContentEditableWarning too now that we invented that… |
|
Maybe just add a list/object at the top with those keys to skip over? Otherwise this seems good. |
|
@spicyj I added an object. |
| ).toBe(false); | ||
| }); | ||
|
|
||
| it('should skip dangerouslySetInnerHTML on web components', function() { |
There was a problem hiding this comment.
Made this a separate test because can’t check children and dangerouslySetInnerHTML at the same time.
|
@gaearon updated the pull request. |
1 similar comment
|
@gaearon updated the pull request. |
| var STYLE = keyOf({style: null}); | ||
| var HTML = keyOf({__html: null}); | ||
| var RESERVED_PROPS = keyMirror({ |
There was a problem hiding this comment.
No need for keyMirror since we're not using the values.
|
(Otherwise looks great.) |
|
@gaearon updated the pull request. |
Don't set children attribute on SVG nodes
| if (propKey === CHILDREN) { | ||
| nextProp = null; | ||
| if (!RESERVED_PROPS.hasOwnProperty(propKey)) { | ||
| DOMPropertyOperations.setValueForAttribute( |
There was a problem hiding this comment.
@gaearon this implementation doesn't allow to pass values other than strings to a custom element. How do I pass an array, object or function?
This fixes a bug introduced in #5714.
Closes #6165 (yes, an issue from the future, I know!)
The code paths are now similar to web components where we work around the same issue (#5093).
Reviewers: @spicyj