Allow numbers after data- in custom attributes fixes #2329

[nLight]

Hi guys,

I decided to start with a good first bug: #2329

With that fix it's now possible to have a number after a data- i.e. data-1000-number in a custom attribute. That should be it, am I missing something?

Thanks,
Dmitriy

[Prinzhorn]

Well, while we're at it: it's not just numbers. If this gets fixed, then it should not just add another special case but allow all valid attributes.

NameStartChar ::= ":" | [A-Z] | "_" | [a-z] | [#xC0-#xD6] | [#xD8-#xF6] | [#xF8-#x2FF] | [#x370-#x37D] | [#x37F-#x1FFF] | [#x200C-#x200D] | [#x2070-#x218F] | [#x2C00-#x2FEF] | [#x3001-#xD7FF] | [#xF900-#xFDCF] | [#xFDF0-#xFFFD] | [#x10000-#xEFFFF]
NameChar ::= NameStartChar | "-" | "." | [0-9] | #xB7 | [#x0300-#x036F] | [#x203F-#x2040]

http://www.w3.org/TR/xml/#NT-NameStartChar

As I said in #2329 data- is the start of the attribute name. So all characters defined in NameChar are allowed, not just those in NameStartChar

[jimfb]

@nLight Can you update as per @Prinzhorn

@spicyj This PR has been open for nearly two weeks. Any objections to us merging?

[sophiebits]

It's not even done yet.

[nLight]

I'll update

[jimfb]

@nLight Ping. Would love to get this in.

[facebook-github-bot]

@nLight updated the pull request.

[nLight]

Sorry it took so long, guys. I have no idea how to test this range \u10000-\uEFFFF but everything else seems to be working.

[facebook-github-bot]

@nLight updated the pull request.

[jimfb]

Looks good to me. I'll leave it open for a day so everyone has a chance to see this before merging.

[sophiebits]

The code points U+10000 and beyond are represented in UCS-2 as two Unicode characters:

See https://mathiasbynens.be/notes/javascript-encoding for more details. I think you should be able to test that emoji in attributes like "data-😊" works. I don't think your current code works with this. If you can't get this to work properly, I would be okay dropping the \u10000-\uEFFFF range (since it doesn't work like this) and adding a TODO to handle surrogate pairs.

[nLight]

Hey @spicyj, I've read this article. Turns out the code works:

[sophiebits]

Well now I'm really confused. I did not expect

/^[\u10000-\uEFFFF]*$/.exec('😊')

to match anything…

[nLight]

Chrome Version 46.0.2490.80 (64-bit)

[sophiebits]

Yes, mine does that too. That's why I'm confused. But @zpao is confused about why you're not hitting this check:

react/src/renderers/dom/shared/DOMPropertyOperations.js

Lines 23 to 45 in d6a547f

	var VALID_ATTRIBUTE_NAME_REGEX = /^[a-zA-Z_][\w\.\-]*$/;
	var illegalAttributeNameCache = {};
	var validatedAttributeNameCache = {};
	function isAttributeNameSafe(attributeName) {
	if (validatedAttributeNameCache.hasOwnProperty(attributeName)) {
	return true;
	}
	if (illegalAttributeNameCache.hasOwnProperty(attributeName)) {
	return false;
	}
	if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) {
	validatedAttributeNameCache[attributeName] = true;
	return true;
	}
	illegalAttributeNameCache[attributeName] = true;
	warning(
	false,
	'Invalid attribute name: `%s`',
	attributeName
	);
	return false;
	}

[nLight]

Because this function doesn't test props name only tag name to contain '-'

react/src/renderers/dom/shared/ReactDOMComponent.js

Line 494 in d6a547f

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

[zpao]

<p {...{'data-😬': seconds}}>{message}</p> will trigger that warning. It seems like it only occurs for updates, which your test didn't trigger.

[jimfb]

Ugg, this is why I wish we didn't have to do our own validation.

Ok, sounds to me like the next step is to write two unit tests (one for a div, one for a custom-component) which performs initial render and an update render, verify no warnings are emitted. That will likely include expanding the whitelist that @spicyj and @zpao mentioned.

[nLight]

Working on the attribute name check function, tests will follow.

[nLight]

After patching the function I discovered that Chrome, Safari and Firefox don't like data-😬 and throw

Uncaught DOMException: Failed to execute 'setAttribute' on 'Element': 'data-😬' is not a valid attribute name.

Simple code to reproduce:

var d = document.createElement("div");
d.setAttribute("data-😬", "123");

though data-Á works fine. I'd say we ignore \u10000-\uEFFFF for now, how do you think?

[sophiebits]

Good with me.

[facebook-github-bot]

@nLight updated the pull request.

[jimfb]

Is this different from ATTRIBUTE_NAME_CHAR? Should that variable be shared?

[nLight]

It's the same. Any suggestions where to put this variable? Both of them require DOMProperty

var DOMProperty = require('DOMProperty');

[jimfb]

Idk, I'd probably put it into HTMLDOMPropertyConfig (since it feels sort of config like). DOMPropertyOperations can then require HTMLDOMPropertyConfig.ATTRIBUTE_NAME_CHAR.

cc @spicyj for thoughts

[sophiebits]

DOMProperty is fine. DOMPropertyOperations shouldn't depend on HTMLDOMPropertyConfig directly.

[jimfb]

👍

[facebook-github-bot]

@nLight updated the pull request.

[facebook-github-bot]

@nLight updated the pull request.

[jimfb]

Ok, this looks good to me. @spicyj Any comments? If not, let's leave it open for another day and then we can merge.

[sergical]

What's the status on this guy?

[sophiebits]

I'm fine with this if @jimfb wants to merge.

[jimfb]

Looks good, thanks everyone! I know this one had a bunch of back and forth, but you did a great job @nLight, Thanks!