Manipulation: Bring tagname regexes up to spec #2634

LeonardoBraga · 2015-10-07T05:00:21Z

Closes gh-2005

Implements the recommendations from @gibson042 in this comment.

mgol · 2015-10-07T10:33:27Z

You have to exclude NULL (\0) as well, @gibson042 mentioned that in his comment.

LeonardoBraga · 2015-10-07T22:46:55Z

@mzgol Added the missing NULL (\0) character to the exclusion list.

gibson042 · 2015-10-08T16:26:54Z

src/core/var/rsingleTag.js

Some comments on this regular expression:

\s is too broad—we should instead use \x20\t\r\n\f (assuming Recognize carriage return as a tag name terminator whatwg/html#242 is accepted)

It shouldn't start accepting :, which is special in XHTML—such text should go through the namespace-aware innerHTML path

It would be smaller if it were case-insensitive rather than explicitly matching A-Z. Altogether, I got good results with /^<([a-z][^:\/\0>\x20\t\r\n\f]*)[\x20\t\r\n\f]*\/?>(?:<\/\1>|)$/i.

Sure. I'll apply the aforementioned changes.

Regarding making it case insensitive, I considered that and did a quick research about it, but all the pieces of information I found said that the performance would be considerably lower than adding both a-z and A-Z. Just let me know if you'd rather taking the chance and making it case insensitive.

It's only used in association with jQuery.parseHTML; any gains would be unnoticeable in the cost of creating elements.

Got it. I'll update the code and amend the PR.

markelog · 2015-10-12T14:08:44Z

Friendly ping

LeonardoBraga · 2015-10-13T00:39:50Z

@gibson042 Please ignore my previous question. I think I got what you meant and updated the PR.

timmywil · 2015-10-13T14:37:09Z

test/unit/manipulation.js

Thank you for this! I think my only note is to be more specific about how we're respecting the spec here.

My pleasure to help!

Would ...

html(String) respect the spec on how to handle special and name-terminating chars for tag names (gh-2005)

... better describe the changes?

Works for me!

@timmywil Done.

@LeonardoBraga Are you sure? It doesn't look like the change got committed.

@timmywil My apologies. It's updated now.

LeonardoBraga · 2015-10-19T02:53:51Z

Just pinging to know if there's anything else I can do to help the team land this PR. Thanks!

dmethvin · 2015-10-19T20:55:33Z

src/manipulation.js

Does this regex need the two negated > in a row? There's one in the modified part and one that was there originally.

Without the two negated >, some tests fail such as this one. It seems that before, the \w was taking care of the job of the first negation, but now we're finding the name-terminating characters explicitly, we'll need to handle this ourselves.

OK that makes sense, thanks!

gibson042 · 2015-10-20T16:16:11Z

test/unit/manipulation.js

This should also include / and >.

Or rather, those cases should also be tested. It looks like > is covered implicitly, so maybe just add buildChild( method, "<" + nodeName + "/>" ) into the helpers.

gibson042 · 2015-10-20T16:24:16Z

Sorry for the delay, @LeonardoBraga. Did you check the size impact of moving all the \/\0> sequence to the front of their character sets?

Other than these minor comments, it looks good to me.

LeonardoBraga · 2015-12-05T02:10:31Z

@gibson042 Moving \/\0> to the front of the exclusion sequences produces the following differences:

raw     gz Compared to last run                                             
  =     +2 dist/jquery.js                                                   
  =     -1 dist/jquery.min.js

Based on my (limited) knowledge about the libraries usage in general, I lean towards benefiting the minified version (by having \/\0> in the beginning of their exclusions lists). The PR was updated to address the comments above and move \/\0> to the beginning. Please let me know if any additional changes are required.

Closes jquerygh-2005

LeonardoBraga · 2016-01-07T20:09:28Z

Just touching base to see if there's anything else I can do to help you guys have this PR landed. Thanks!

mgol · 2016-01-07T20:14:14Z

@LeonardoBraga Sorry for the delay, we're preparing for 1.12/2.2 simultaneous release, your PR is scheduled for 3.0.0 so we'll get to it once we're done with 1.12/2.2. :)

gibson042 · 2016-01-07T21:58:14Z

Thanks for your patience, @LeonardoBraga.

LeonardoBraga · 2016-01-07T22:01:40Z

Thank you all! I'm just glad I was able to help. :-)

jquerybot added the CLA: Valid label Oct 7, 2015

LeonardoBraga force-pushed the manipulation-gh2005 branch from 983af84 to 381d4b2 Compare October 7, 2015 22:44

LeonardoBraga force-pushed the manipulation-gh2005 branch from 381d4b2 to 3f4e917 Compare October 7, 2015 22:52

gibson042 reviewed Oct 8, 2015
View reviewed changes

LeonardoBraga force-pushed the manipulation-gh2005 branch 2 times, most recently from 75f679e to ec5acd9 Compare October 13, 2015 00:38

LeonardoBraga force-pushed the manipulation-gh2005 branch 3 times, most recently from 17ff880 to de84b98 Compare October 13, 2015 02:48

timmywil reviewed Oct 13, 2015
View reviewed changes

LeonardoBraga force-pushed the manipulation-gh2005 branch 2 times, most recently from 828e2ca to d674651 Compare October 14, 2015 16:13

dmethvin reviewed Oct 19, 2015
View reviewed changes

LeonardoBraga force-pushed the manipulation-gh2005 branch from d674651 to 4abef66 Compare October 20, 2015 02:40

gibson042 reviewed Oct 20, 2015
View reviewed changes

LeonardoBraga force-pushed the manipulation-gh2005 branch from 4abef66 to 9ed20d0 Compare December 5, 2015 02:08

Manipulation: Bring tagname regexes up to spec

b1f678a

Closes jquerygh-2005

LeonardoBraga force-pushed the manipulation-gh2005 branch from 9ed20d0 to b1f678a Compare December 5, 2015 02:15

gibson042 closed this in fb9472c Jan 7, 2016

LeonardoBraga deleted the manipulation-gh2005 branch January 7, 2016 22:05

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Manipulation: Bring tagname regexes up to spec #2634

Manipulation: Bring tagname regexes up to spec #2634

Uh oh!

Conversation

LeonardoBraga commented Oct 7, 2015

Uh oh!

mgol commented Oct 7, 2015

Uh oh!

LeonardoBraga commented Oct 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog commented Oct 12, 2015

Uh oh!

LeonardoBraga commented Oct 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonardoBraga commented Oct 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 commented Oct 20, 2015

Uh oh!

LeonardoBraga commented Dec 5, 2015

Uh oh!

LeonardoBraga commented Jan 7, 2016

Uh oh!

mgol commented Jan 7, 2016

Uh oh!

gibson042 commented Jan 7, 2016

Uh oh!

LeonardoBraga commented Jan 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

8 participants