Skip to content

Commit

Permalink
Move namespace check to isCustomAttribute. Add caveat for stack.
Browse files Browse the repository at this point in the history
  • Loading branch information
@nhunzaker
nhunzaker committed Aug 29, 2017
1 parent af7d035 commit 9c0751f
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 87 deletions.
57 changes: 34 additions & 23 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Expand Up @@ -40,10 +40,10 @@ if (__DEV__) {
var ReactDOMUnknownPropertyHook = require('ReactDOMUnknownPropertyHook');
var {validateProperties: validateARIAProperties} = ReactDOMInvalidARIAHook;
var {
validateProperties: validateInputPropertes,
validateProperties: validateInputProperties,
} = ReactDOMNullInputValuePropHook;
var {
validateProperties: validateUnknownPropertes,
validateProperties: validateUnknownProperties,
} = ReactDOMUnknownPropertyHook;
}

Expand All @@ -70,10 +70,10 @@ if (__DEV__) {
time: true,
};

var validatePropertiesInDevelopment = function(type, props) {
validateARIAProperties(type, props);
validateInputPropertes(type, props);
validateUnknownPropertes(type, props);
var validatePropertiesInDevelopment = function(type, namespace, props) {
validateARIAProperties(type, props, namespace);
validateInputProperties(type, props);
validateUnknownProperties(type, props, namespace);
};

var warnForTextDifference = function(serverText: string, clientText: string) {
Expand Down Expand Up @@ -307,8 +307,7 @@ var ReactDOMFiberComponent = {
}
if (namespaceURI === HTML_NAMESPACE) {
if (__DEV__) {
var isCustomComponentTag =
isCustomComponent(type, props) && namespaceURI === HTML_NAMESPACE;
var isCustomComponentTag = isCustomComponent(type, props, namespaceURI);
// Should this check be gated by parent namespace? Not sure we want to
// allow <SVG> or <mATH>.
warning(
Expand Down Expand Up @@ -369,11 +368,13 @@ var ReactDOMFiberComponent = {
rawProps: Object,
rootContainerElement: Element | Document,
): void {
var isCustomComponentTag =
isCustomComponent(tag, rawProps) &&
domElement.namespaceURI === HTML_NAMESPACE;
var isCustomComponentTag = isCustomComponent(
tag,
rawProps,
domElement.namespaceURI,

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon Aug 29, 2017

Collaborator

I intentionally didn't do this because we didn't need to read it from the DOM except the rare case when the first check is positive. Can we restructure so that we don't read it unless necessary?
);
if (__DEV__) {
validatePropertiesInDevelopment(tag, rawProps);
validatePropertiesInDevelopment(tag, domElement.namespaceURI, rawProps);
if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) {
warning(
false,
Expand Down Expand Up @@ -544,7 +545,11 @@ var ReactDOMFiberComponent = {
rootContainerElement: Element | Document,
): null | Array<mixed> {
if (__DEV__) {
validatePropertiesInDevelopment(tag, nextRawProps);
validatePropertiesInDevelopment(
tag,
domElement.namespaceURI,
nextRawProps,
);
}

var updatePayload: null | Array<any> = null;
Expand Down Expand Up @@ -741,12 +746,16 @@ var ReactDOMFiberComponent = {
lastRawProps: Object,
nextRawProps: Object,
): void {
var wasCustomComponentTag =
isCustomComponent(tag, lastRawProps) &&
domElement.namespaceURI === HTML_NAMESPACE;
var isCustomComponentTag =
isCustomComponent(tag, nextRawProps) &&
domElement.namespaceURI === HTML_NAMESPACE;
var wasCustomComponentTag = isCustomComponent(
tag,
lastRawProps,
domElement.namespaceURI,
);
var isCustomComponentTag = isCustomComponent(
tag,
nextRawProps,
domElement.namespaceURI,
);
// Apply the diff.
updateDOMProperties(
domElement,
Expand Down Expand Up @@ -786,10 +795,12 @@ var ReactDOMFiberComponent = {
rootContainerElement: Element | Document,
): null | Array<mixed> {
if (__DEV__) {
var isCustomComponentTag =
isCustomComponent(tag, rawProps) &&
domElement.namespaceURI === HTML_NAMESPACE;
validatePropertiesInDevelopment(tag, rawProps);
var isCustomComponentTag = isCustomComponent(
tag,
rawProps,
domElement.namespaceURI,
);
validatePropertiesInDevelopment(tag, domElement.namespaceURI, rawProps);
if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) {
warning(
false,
Expand Down
52 changes: 29 additions & 23 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Expand Up @@ -2189,33 +2189,39 @@ describe('ReactDOMComponent', () => {
});
});

describe('Hyphenated SVG elements', function() {
it('the font-face element is not a custom element', function() {
spyOn(console, 'error');
var el = ReactTestUtils.renderIntoDocument(
<font-face x-height={false} />,
);
if (ReactDOMFeatureFlags.useFiber) {
describe('Hyphenated SVG elements', function() {
it('the font-face element is not a custom element', function() {
spyOn(console, 'error');
var el = ReactTestUtils.renderIntoDocument(
<svg><font-face x-height={false} /></svg>,
);

expect(el.hasAttribute('x-height')).toBe(false);
expect(el.querySelector('font-face').hasAttribute('x-height')).toBe(
false,
);

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Invalid DOM property `x-height`. Did you mean `xHeight`',
);
});
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Invalid DOM property `x-height`. Did you mean `xHeight`',
);
});

it('the font-face element does not allow unknown boolean values', function() {
spyOn(console, 'error');
var el = ReactTestUtils.renderIntoDocument(
<font-face whatever={false} />,
);
it('the font-face element does not allow unknown boolean values', function() {
spyOn(console, 'error');
var el = ReactTestUtils.renderIntoDocument(
<svg><font-face whatever={false} /></svg>,
);

expect(el.hasAttribute('whatever')).toBe(false);
expect(el.querySelector('font-face').hasAttribute('whatever')).toBe(
false,
);

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received `false` for non-boolean attribute `whatever`.',
);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received `false` for non-boolean attribute `whatever`.',
);
});
});
});
}
});
8 changes: 4 additions & 4 deletions src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js
View file
Expand Up @@ -145,8 +145,8 @@ function warnInvalidARIAProps(type, props, debugID) {
}
}

function validateProperties(type, props, debugID /* Stack only */) {
if (isCustomComponent(type, props)) {
function validateProperties(type, props, namespace, debugID /* Stack only */) {
if (isCustomComponent(type, props, namespace)) {
return;
}
warnInvalidARIAProps(type, props, debugID);
Expand All @@ -158,12 +158,12 @@ var ReactDOMInvalidARIAHook = {
// Stack
onBeforeMountComponent(debugID, element) {
if (__DEV__ && element != null && typeof element.type === 'string') {
validateProperties(element.type, element.props, debugID);
validateProperties(element.type, element.props, null, debugID);
}
},
onBeforeUpdateComponent(debugID, element) {
if (__DEV__ && element != null && typeof element.type === 'string') {
validateProperties(element.type, element.props, debugID);
validateProperties(element.type, element.props, null, debugID);
}
},
};
Expand Down
9 changes: 5 additions & 4 deletions src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js
View file
Expand Up @@ -14,6 +14,7 @@
var DOMProperty = require('DOMProperty');
var EventPluginRegistry = require('EventPluginRegistry');
var isCustomComponent = require('isCustomComponent');
var DOMNamespaces = require('DOMNamespaces');

if (__DEV__) {
var warning = require('fbjs/lib/warning');
Expand Down Expand Up @@ -208,8 +209,8 @@ var warnUnknownProperties = function(type, props, debugID) {
}
};

function validateProperties(type, props, debugID /* Stack only */) {
if (isCustomComponent(type, props)) {
function validateProperties(type, props, namespace, debugID /* Stack only */) {
if (isCustomComponent(type, props, namespace)) {
return;
}
warnUnknownProperties(type, props, debugID);
Expand All @@ -221,12 +222,12 @@ var ReactDOMUnknownPropertyHook = {
// Stack
onBeforeMountComponent(debugID, element) {
if (__DEV__ && element != null && typeof element.type === 'string') {
validateProperties(element.type, element.props, debugID);
validateProperties(element.type, element.props, null, debugID);
}
},
onBeforeUpdateComponent(debugID, element) {
if (__DEV__ && element != null && typeof element.type === 'string') {
validateProperties(element.type, element.props, debugID);
validateProperties(element.type, element.props, null, debugID);
}
},
};
Expand Down
22 changes: 8 additions & 14 deletions src/renderers/dom/shared/utils/isCustomComponent.js
View file
Expand Up @@ -11,23 +11,17 @@

'use strict';

// https://www.w3.org/TR/SVG/eltindex.html
var DashedSVGElements = {
'color-profile': true,
'font-face': true,
'font-face-format': true,
'font-face-name': true,
'font-face-src': true,
'font-face-uri': true,
'missing-glyph': true,
};
var DOMNamespaces = require('DOMNamespaces');
var HTML_NAMESPACE = DOMNamespaces.Namespaces.html;

function isCustomComponent(tagName, props) {
if (DashedSVGElements[tagName]) {
return false;
function isCustomComponent(tagName, props, namespace) {
if (tagName.indexOf('-') >= 0 || props.is != null) {
// TODO: We always have a namespace with fiber. Drop the first
// check when Stack is removed.
return namespace == null || namespace === HTML_NAMESPACE;
}

return tagName.indexOf('-') >= 0 || props.is != null;
return false;
}

module.exports = isCustomComponent;
23 changes: 12 additions & 11 deletions src/renderers/dom/stack/client/ReactDOMComponent.js
View file
Expand Up @@ -498,8 +498,11 @@ ReactDOMComponent.Mixin = {
namespaceURI = Namespaces.html;
}
if (__DEV__) {
var isCustomComponentTag =
isCustomComponent(this._tag, props) && namespaceURI === Namespaces.html;
var isCustomComponentTag = isCustomComponent(
this._tag,
props,
namespaceURI,
);
}
if (namespaceURI === Namespaces.html) {
if (__DEV__) {
Expand Down Expand Up @@ -699,8 +702,7 @@ ReactDOMComponent.Mixin = {
var markup = null;
if (
this._tag != null &&
isCustomComponent(this._tag, props) &&
this._namespaceURI === Namespaces.html
isCustomComponent(this._tag, props, this._namespaceURI)
) {
if (!DOMProperty.isReservedProp(propKey)) {
markup = DOMMarkupOperations.createMarkupForCustomAttribute(
Expand Down Expand Up @@ -884,9 +886,11 @@ ReactDOMComponent.Mixin = {
}

assertValidProps(this, nextProps);
var isCustomComponentTag =
isCustomComponent(this._tag, nextProps) &&
this._namespaceURI === Namespaces.html;
var isCustomComponentTag = isCustomComponent(
this._tag,
nextProps,
this._namespaceURI,
);
this._updateDOMProperties(
lastProps,
nextProps,
Expand Down Expand Up @@ -960,10 +964,7 @@ ReactDOMComponent.Mixin = {
} else if (registrationNameModules.hasOwnProperty(propKey)) {
// Do nothing for event names.
} else if (!DOMProperty.isReservedProp(propKey)) {
if (
isCustomComponent(this._tag, lastProps) &&
this._namespaceURI === Namespaces.html
) {
if (isCustomComponent(this._tag, lastProps, this._namespaceURI)) {
DOMPropertyOperations.deleteValueForAttribute(getNode(this), propKey);
} else {
DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey);
Expand Down
13 changes: 5 additions & 8 deletions src/renderers/shared/server/ReactPartialRenderer.js
View file
Expand Up @@ -45,10 +45,10 @@ if (__DEV__) {
var {
validateProperties: validateUnknownPropertes,
} = require('ReactDOMUnknownPropertyHook');
var validatePropertiesInDevelopment = function(type, props) {
validateARIAProperties(type, props);
var validatePropertiesInDevelopment = function(type, namespace, props) {
validateARIAProperties(type, namespace, props);
validateInputPropertes(type, props);
validateUnknownPropertes(type, props);
validateUnknownPropertes(type, namespace, props);
};

var describeComponentFrame = require('describeComponentFrame');
Expand Down Expand Up @@ -278,10 +278,7 @@ function createOpenTagMarkup(
propValue = createMarkupForStyles(propValue, instForDebug);
}
var markup = null;
if (
isCustomComponent(tagLowercase, props) &&
namespace === Namespaces.html
) {
if (isCustomComponent(tagLowercase, props, namespace)) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
markup = DOMMarkupOperations.createMarkupForCustomAttribute(
propKey,
Expand Down Expand Up @@ -778,7 +775,7 @@ class ReactDOMServerRenderer {
}

if (__DEV__) {
validatePropertiesInDevelopment(tag, props);
validatePropertiesInDevelopment(tag, props, namespace);
}

assertValidProps(tag, props);
Expand Down

0 comments on commit 9c0751f

Please sign in to comment.