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

Use lower case for HTML attributes #11110

Closed
wants to merge 3 commits into from
Closed

Use lower case for HTML attributes #11110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3620fe
Add a test case for lowercase HTML expectation
gaearon Oct 5, 2017
be96bb9
Emit lowercase attributes in HTML namespace
gaearon Oct 5, 2017
850550c
Add a regression test with namespace nesting
gaearon Oct 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/renderers/dom/shared/DOMMarkupOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@
'use strict';

var DOMProperty = require('DOMProperty');
var {Namespaces} = require('DOMNamespaces');

var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');

if (__DEV__) {
var warning = require('fbjs/lib/warning');
}

var HTML_NAMESPACE = Namespaces.html;

// isAttributeNameSafe() is currently duplicated in DOMPropertyOperations.
// TODO: Find a better place for this.
var VALID_ATTRIBUTE_NAME_REGEX = new RegExp(
Expand Down Expand Up @@ -85,13 +88,16 @@ var DOMMarkupOperations = {
* @param {*} value
* @return {?string} Markup string, or null if the property was invalid.
*/
createMarkupForProperty: function(name, value) {
createMarkupForProperty: function(name, value, namespace) {
var propertyInfo = DOMProperty.getPropertyInfo(name);
if (propertyInfo) {
if (shouldIgnoreValue(propertyInfo, value)) {
return '';
}
var attributeName = propertyInfo.attributeName;
if (namespace === HTML_NAMESPACE) {
attributeName = attributeName.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this happen once, when the properties are injected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That wouldn't help with properties that aren't in the whitelist.

Copy link
Contributor

Choose a reason for hiding this comment

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

But line 93 ensures that this code only runs when we have property info (which I believe is always from the whitelist)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I see. I guess we should update the custom ones too in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait. Why is this necessary? This already gets lower cased in the injection here:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/DOMProperty.js#L83

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like tests pass when I remove this statement! (98-100). Totally need the toLowerCase below though.

}
if (
propertyInfo.hasBooleanValue ||
(propertyInfo.hasOverloadedBooleanValue && value === true)
Expand All @@ -107,6 +113,9 @@ var DOMMarkupOperations = {
if (value == null) {
return '';
}
if (namespace === HTML_NAMESPACE) {
name = name.toLowerCase();
}
return name + '=' + quoteAttributeValueForBrowser(value);
}
return null;
Expand Down
29 changes: 29 additions & 0 deletions src/renderers/dom/shared/__tests__/ReactServerRendering-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,4 +695,33 @@ describe('ReactDOMServer', () => {
'HTML tags in React.',
);
});

it('uses lowercase tags in HTML output', () => {
expect(ReactDOMServer.renderToString(<div tabIndex="-1" />)).toContain(
'tabindex=',
);
});

it('keeps tracks of attribute case sensitivity based on the namespace', () => {
const html = ReactDOMServer.renderToString(
<div itemProp="name">
<svg textRendering="optimizeLegibility" baseProfile="full">
<foreignObject externalResourcesRequired={true} tabIndex="1">
<img srcSet="wow" />
<svg viewBox="0 0 0 0" />
</foreignObject>
</svg>
<input minLength="10" />
</div>,
);
// SVG is case sensitive
expect(html).toContain('text-rendering=');
expect(html).toContain('baseProfile=');
expect(html).toContain('tabindex=');
expect(html).toContain('viewBox=');
// HTML is not, but people expect lowercase output
expect(html).toContain('itemprop=');
expect(html).toContain('srcset=');
expect(html).toContain('minlength=');
});
});
6 changes: 5 additions & 1 deletion src/renderers/shared/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,11 @@ function createOpenTagMarkup(
);
}
} else {
markup = DOMMarkupOperations.createMarkupForProperty(propKey, propValue);
markup = DOMMarkupOperations.createMarkupForProperty(
propKey,
propValue,
namespace,
);
}
if (markup) {
ret += ' ' + markup;
Expand Down