-
Notifications
You must be signed in to change notification settings - Fork 20.6k
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
Add support to tag-hyphenated elements #1988
Conversation
@@ -627,6 +627,7 @@ test("jQuery('html')", function() { | |||
|
|||
ok( jQuery("<div></div>")[0], "Create a div with closing tag." ); | |||
ok( jQuery("<table></table>")[0], "Create a table with closing tag." ); | |||
ok( jQuery("<tr-d></tr-d>")[0], "Create a dash-delimited tag." ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean to be pedantic, but that's not "delimited". Suggest: "hyphenated".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem! I changed as suggested. Please let me know if anything requires adjustments.
@@ -1620,6 +1620,8 @@ function testHtml( valueObj ) { | |||
tmp = " " + tmp.slice( 1 ); | |||
equal( div.html(valueObj( tmp )).html().replace( />/g, ">" ), tmp, "Escaped html, leading space" ); | |||
|
|||
equal( jQuery("<tr-d></tr-d><tr-d></tr-d>").length, 2, "Create tag-hyphenated elements." ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want more assertions for this, so it would be more thorough, like check for nodeName and for other elements like this. No need to do this for core module though.
And i would prefer if those assertions would be in different test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe a case with two (or more) hyphens, just in case. It's obvious current implementation will work in the same way but it's better to be future-proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 the more tests the better, especially if this involves regexps
Could break this pull on two commits? One for core, other for manipulation, i think it would be best way to do this. You can squash other commits, don't need them. Also please verify these changes would work in |
I'm implementing the requested changes and will update this PR soon, however I noticed that |
@LeonardoBraga i guess you missed my comment - #1988 (comment) |
@markelog Applied and tested the changes locally on |
|
||
var j = jQuery("<tr-d></tr-d>"); | ||
ok( j[0], "Create a tag-hyphenated elements" ); | ||
equal( j[0].nodeName, "TR-D", "Tag-hyphenated element has expected node name" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use jQuery.nodeName
here
In #1988 (comment), i meant all of them, like here - https://github.com/jquery/jquery/blob/66e1b6b8d49812239b5712d65922ff94c60f7b02/test/unit/manipulation.js#L264-268, just to be safe |
That would be preferable |
@gkalpak makes a valid point... in fact, HTML tag names must start with an ASCII letter, but can then include almost anything. Let's update all three regular expressions accordingly, so that the capture consists of a case-insensitive a–z and any number of non-whitespace non-slash non-greater-than non-NULL characters. |
Wow, so ASCII letters are just lowercased and pretty much any other non-space character is allowed. That's mighty liberal. So it does sound like we need to adjust our regex-en (or whatever the plural of regex is). |
I'll update this PR to also address the concerns regarding valid tag names. Regarding the previous state, it's not clear to me how we stand on:
Thank you - I'm glad this exposed some other cases that needed attention as well. I'll try to wrap up this quickly. |
I stand by my previous position, because ":" is special.
Where the statements now refer to acceptance of the character, rather than its literal appearance in the regex. |
I think we should merge this as is, this PR solves real problem that users faced, "almost anything" is nice to have and could be solved in another PR |
This, and i can't believe we forgot about it, doesn't work in IE8, remember why we have the shim? Right, now i do too. My first thought was to revert, now i think we should blacklist those hyphenated tests for the old one. Since if user uses custom elements he definitely doesn't use it with IE8. |
😈 I don't think we need to revert, we can just skip it for IE8. |
Will do that. @LeonardoBraga note that you would have to do the same thing for #2005 |
Do you mean to blacklist the tests I'll write for IE8? If so, is there a place where this is already done in the code so I can follow the same pattern? |
See the 87bb713 |
Isn't it enough to |
Yes.
Even though this is unlikely use-case, we still can't do that, oldIE will identify only those elements that created in the scope of the document it first added to, which is in our case, is internal |
Agreed. As long as our code is solid, it's OK for some edge cases to fail on IE8. |
If wouldn't do something crazy like find those elems in user string and create them on our |
Why is this only being fixed in version 3.0.0? For anyone using jQuery for template-like support across multiple browsers, this change is essential for full support of custom elements. I think it should be back ported to 1.x and 2.x. This shouldn't be hard since it's a small, non-breaking change. |
…round Note jQuery bug at jquery/jquery#1988.
This has been backported to |
I moved the milestone from this PR to the issue #1987. |
Fixes #1987