Core: Issue while creating dash-delimited nodes #1987

Closed
LeonardoBraga opened this Issue Jan 3, 2015 · 8 comments

Comments

Projects
None yet
5 participants
@LeonardoBraga
Contributor

LeonardoBraga commented Jan 3, 2015

JQuery fails in some cases to create dash-delimited notes:

// Works fine and creates the element:
$('<div-d></div-d>');

// Does not create the element:
$('<tr-d></tr-d>');

// ... and it should, since document.createElement works fine too:
document.createElement('tr-d');

Reported first here: angular/angular.js#10617

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 3, 2015

Member

So we would be on same page here:
tr-d is not a valid html tag name, but since it has a hyphen in it, it is now considered as custom element and custom elements are part of web components.

Characters that could be used in tag name of custom elements are described in xml specification. Like _ and . are also included in that class. So if we fully support custom elements do we need to add tests for names with _ and . characters?

But this discussion could be deferred and i let @dmethvin answer that.

Apart from that, this looks like real use-case and real bug, there is two problems with this particular case: first with rsingleTag regexp and second one with rtagName one.

Your commit, mentioned in this ticket, would resolve <tr-d></tr-d> case, but not <tr-d></tr-d><tr-d></tr-d>.

This is why even div-d fails rsingleTag check, it still works, since buildFragment doesn't feel the need to wrap it with special parent tag.

Would you like to send us PR for both?

Member

markelog commented Jan 3, 2015

So we would be on same page here:
tr-d is not a valid html tag name, but since it has a hyphen in it, it is now considered as custom element and custom elements are part of web components.

Characters that could be used in tag name of custom elements are described in xml specification. Like _ and . are also included in that class. So if we fully support custom elements do we need to add tests for names with _ and . characters?

But this discussion could be deferred and i let @dmethvin answer that.

Apart from that, this looks like real use-case and real bug, there is two problems with this particular case: first with rsingleTag regexp and second one with rtagName one.

Your commit, mentioned in this ticket, would resolve <tr-d></tr-d> case, but not <tr-d></tr-d><tr-d></tr-d>.

This is why even div-d fails rsingleTag check, it still works, since buildFragment doesn't feel the need to wrap it with special parent tag.

Would you like to send us PR for both?

@markelog markelog added the Core label Jan 3, 2015

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 3, 2015

Member

Would you like to send us PR for both?

Preferable with two commits in it, one for core and one for manipulation module, don't forget to read http://contribute.jquery.org/ if you feeling up to it

Member

markelog commented Jan 3, 2015

Would you like to send us PR for both?

Preferable with two commits in it, one for core and one for manipulation module, don't forget to read http://contribute.jquery.org/ if you feeling up to it

@LeonardoBraga

This comment has been minimized.

Show comment
Hide comment
@LeonardoBraga

LeonardoBraga Jan 3, 2015

Contributor

Thanks for the instructions, and yes, I'd like to submit a PR for it.

Just to be clear and fair, issue was initially reported and patched by @gkalpak on Angular.js repo. It just happened that I bumped into it at the same time and tried to share the load.

Contributor

LeonardoBraga commented Jan 3, 2015

Thanks for the instructions, and yes, I'd like to submit a PR for it.

Just to be clear and fair, issue was initially reported and patched by @gkalpak on Angular.js repo. It just happened that I bumped into it at the same time and tried to share the load.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Jan 3, 2015

To be even more clear and fair, @elaijuh first reported it on the AngularJS repo.
I submitted a PR for AngularJS (as did @richardaday).

@LeonardoBraga: I am glad you are sharing the load (since I am not familiar with jQuery's contributing conventions 😃).

(Seems like a whole lot of people are involved into this one-character addition 😆 How cool is that ?)

gkalpak commented Jan 3, 2015

To be even more clear and fair, @elaijuh first reported it on the AngularJS repo.
I submitted a PR for AngularJS (as did @richardaday).

@LeonardoBraga: I am glad you are sharing the load (since I am not familiar with jQuery's contributing conventions 😃).

(Seems like a whole lot of people are involved into this one-character addition 😆 How cool is that ?)

@elaijuh

This comment has been minimized.

Show comment
Hide comment
@elaijuh

elaijuh Jan 4, 2015

cool to see this, thanks all for helping on this.

elaijuh commented Jan 4, 2015

cool to see this, thanks all for helping on this.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Jan 16, 2015

Do we know in which version this is going to be included ?
(Sorry, I am not familiar with jQuery's release process.)

gkalpak commented Jan 16, 2015

Do we know in which version this is going to be included ?
(Sorry, I am not familiar with jQuery's release process.)

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 16, 2015

Member

Do we know in which version this is going to be included ?

3.0

Member

markelog commented Jan 16, 2015

Do we know in which version this is going to be included ?

3.0

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 7, 2016

Member

This has been backported to 1.12.0 & 2.2.0 so I changed the milestone.

Member

mgol commented Mar 7, 2016

This has been backported to 1.12.0 & 2.2.0 so I changed the milestone.

@mgol mgol added this to the 1.12.0/2.2.0 milestone Mar 7, 2016

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jquery jquery locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.