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.

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

Allow numbers after data- in custom attributes fixes #2329 #5216

Merged
merged 1 commit into from
Dec 16, 2015
Merged

Allow numbers after data- in custom attributes fixes #2329 #5216

merged 1 commit into from
Dec 16, 2015

Conversation

nLight
Copy link

@nLight nLight commented Oct 19, 2015

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
Copy link

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
Copy link
Contributor

jimfb commented Nov 4, 2015

@nLight Can you update as per @Prinzhorn

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

@sophiebits
Copy link
Contributor

It's not even done yet.

@nLight
Copy link
Author

nLight commented Nov 5, 2015

I'll update

@jimfb
Copy link
Contributor

jimfb commented Nov 9, 2015

@nLight Ping. Would love to get this in.

@facebook-github-bot
Copy link

@nLight updated the pull request.

@nLight
Copy link
Author

nLight commented Nov 10, 2015

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
Copy link

@nLight updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Nov 10, 2015

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

@sophiebits
Copy link
Contributor

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

image

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
Copy link
Author

nLight commented Nov 10, 2015

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

screen shot 2015-11-10 at 21 51 50

@sophiebits
Copy link
Contributor

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

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

to match anything…

@nLight
Copy link
Author

nLight commented Nov 10, 2015

Chrome Version 46.0.2490.80 (64-bit)

screen shot 2015-11-10 at 22 18 36

@sophiebits
Copy link
Contributor

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

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
Copy link
Author

nLight commented Nov 10, 2015

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

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

@zpao
Copy link
Member

zpao commented Nov 10, 2015

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

@jimfb
Copy link
Contributor

jimfb commented Nov 10, 2015

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
Copy link
Author

nLight commented Nov 10, 2015

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

@nLight
Copy link
Author

nLight commented Nov 10, 2015

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
Copy link
Contributor

Good with me.

@facebook-github-bot
Copy link

@nLight updated the pull request.

@@ -39,7 +39,7 @@ if (ExecutionEnvironment.canUseDOM) {

var HTMLDOMPropertyConfig = {
isCustomAttribute: RegExp.prototype.test.bind(
/^(data|aria)-[a-z_][a-z\d_.\-]*$/
/^(data|aria)-[:\-.0-9A-Z_a-z\uB7\u00C0-\u00D6\u00D8-\u00F6\u00F8-\u02FF\u0370-\u037D\u037F-\u1FFF\u200C-\u200D\u2070-\u218F\u2C00-\u2FEF\u3001-\uD7FF\uF900-\uFDCF\uFDF0-\uFFFD\u0300-\u036F\u203F-\u2040]*$/
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

var DOMProperty = require('DOMProperty');

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@facebook-github-bot
Copy link

@nLight updated the pull request.

@facebook-github-bot
Copy link

@nLight updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Nov 12, 2015

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
Copy link

What's the status on this guy?

@sophiebits
Copy link
Contributor

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

@jimfb
Copy link
Contributor

jimfb commented Dec 16, 2015

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

jimfb added a commit that referenced this pull request Dec 16, 2015
Allow numbers after `data-` in custom attributes fixes #2329
@jimfb jimfb merged commit c9c3c33 into facebook:master Dec 16, 2015
@nLight nLight deleted the number-in-data-attrs-2329 branch September 27, 2017 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants