fix(jqLite): Adding more support for custom element tagnames

[richardaday]

Before this change, jqLite would allow you to create elements such as
"my-foo" and "div-foo" but would not allow you to create certain
elements such as "tr-foo" and "td-foo". This is incorrect as well as
undocumented and unexpected.

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[richardaday]

This bug also exists in jQuery and I hope to find some time soon to send a patch their way as well.

[gkalpak]

Same fix as #10618.
(More explicit testing 😃).

[richardaday]

Wow, both pull requests were created at almost the same time!

What would you like me to do with this pull request? I can close it if you like.

[gkalpak]

I like the fact that there are tests for more elements (it shouldn't make any actual difference, but feels better 😃).
Ideally, I would change the tests like this (using nodeName_ and having also some HTML content just to be sure):

nodes = jqLite('<x>y<x/>');
expect(nodes.length).toBe(1);
expect(nodeName_(nodes)).toBe('x');
expect(nodes.html()).toBe('y');

It would also be nice to have similar tests for :-delimited tag names (for future-proofing) and some nested elements (e.g. jqLite('<div><tr-foo>Test</tr-foo></div>')).

_{If you can add these test, I would close my PR in favor of this one 😛}

[richardaday]

Perfect. Give me a day or two.
On Jan 3, 2015 3:46 AM, "Georgios Kalpakas" notifications@github.com
wrote:

I like the fact that there are tests for more elements (it shouldn't make
any actual difference, but feels better [image: 😃]).
Ideally, I would change the tests like this (using nodeName_ and having
also some HTML content just to be sure):

nodes = jqLite('y');
expect(nodes.length).toBe(1);
expect(nodeName_(nodes)).toBe('x');
expect(nodes.html()).toBe('y');

It would also be nice to have similar tests for :-delimited tag names
(for future-proofing) and some nested elements (e.g.
jqLite('

Test

')).

If you can add these test, I would close my PR in favor of this one [image:
😛]

—
Reply to this email directly or view it on GitHub
#10619 (comment).

[gkalpak]

The bug has been fixed on the jQuery side (thx @LeonardoBraga), but it seems that the fix will be released in version 3.0 (which according to the roadmap is planned for Q1 2015).

Not sure if/when Angular 1.x will make the switch to jQuery 3.x.
@petebacondarwin: What should we do about this ?

1. Fix it in jqLite, but don't have tests for the fix (since it will break on jQuery) ?
2. Fix it in jqLite and have different tests for jqLite/jQuery (until jQuery gets to 3.0) ?
3. Wait for Angular to upgrade to jQuery 3.x, before fixing jqLite ?

[lgalfaso]

We should fix this at jqLite (using the same pattern that was used in jQuery) and have tests that only run with jqLite. Do please add a comment in the test pointing to jQuery issue and the fact that this will work as expected in jQuery 3.0

[gkalpak]

What needs to be done:

• Port the changes from the jQuery patch:
  • Update SINGLE_TAG_REGEXP. (This is not functionaly necessary; just to be in sync with jQuery.)
  • Update TAG_NAME_REGEXP.
  • Update XHTML_TAG_REGEXP. (This is not functionaly necessary; just to be in sync with jQuery.)
• Address my comment above.
• Make sure the test are only run with jqLite. The _jqLiteMode global can be used for that:
  if (_jqLiteMode) { /* run the test */ }
• Add an appropriate comment in the test as per @lgalfaso's comment.

@richardaday: Would you like to take care of these in this PR ?

[caitp]

this has been on hold for a while --- do we want this?

Since it won't do anything to help people using jQuery, I'm not sure. Really don't want to monkey patch jQuery again

[LeonardoBraga]

I can provide the PR to address @gkalpak's and @lgalfaso's comments and action items. I think we should add these changes. Do you see an downside on this, @caitp ?

[caitp]

@LeonardoBraga the downside I see is that it will mean an inconsistency between jqLite and the jQuery version used by most applications, I'm not super keen on that. But I'm undecided, I think @petebacondarwin should make the call on this.

[caitp]

that said, by all means send a PR, since this one has not been updated since January

[petebacondarwin]

Bumping this to 1.5 as it is not a regression and we need to think about it a little. @mzgol do you have a view on this?

[mgol]

IMO it's not bad that it creates inconsistency with jQuery as it's just fixing a bug. jQuery has fixed this bug as well and it will be included in 3.0.0 when it's released. I don't see it blocking that if Angular is used with jQuery 2.1.1 this bug will appear; we can say it's a jQuery bug and you have to either use jqLite or upgrade to jQuery 3.0.0 or later to have it working.

Review comments still need to be addressed, though, like the @gkalpak's one: #10619 (comment). I'd just omit the relevant tests in jQuery <3.0.0 and not all jQueries. If the PR is ready before the final 1.4 release then we can land it IMO.

[gkalpak]

@LeonardoBraga, do you plan to submit a PR for this ?
(If not, I will; just asking to avoid duplication of effort 😄)

[gkalpak]

@mzgol, what is the best option for verifying the jQuery version used in tests ?

[mgol]

@gkalpak

@mzgol, what is the best option for verifying the jQuery version used in tests?

If you just want to check if it's 3.0.0 or newer, the easiest would be to:

if (window.jQuery && Number(window.jQuery.fn.jquery.split('.')[0]) >= 3) { /* code */ }

as jQuery.fn.jquery contains the version. Some files have the jQuery global set to undefined in jqLite mode (no idea where's the code causing that) so some tests check for jQuery instead of window.jQuery, you may try it.

If we wanted more fine-grained version comparisons (the above code is ugly and doesn't scale behind major checks), it'd be best to use the semver module:

if (window.jQuery && semver.satisfies(window.jQuery.fn.jquery, '>=3.0.0')) { /* code */ }

I know @IgorMinar has been recently trying to reduce used dependencies instead of adding new ones, though. :-)

EDIT: we already have semver in dependencies so no need to add anything to package.json, you may just use it. You'd have to load the node_modules/semver/semver.browser.js file in Karma.

[gkalpak]

Ckecking for 3.0.0 or newer would suffice, so the former approach is fine. Thx !

[gkalpak]

Closing in favor of #12759 (due to inactivity).