refactor(*): separate jqLite/compile/sce camelCasing logic #15249

mgol · 2016-10-12T12:26:21Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactor

What is the current behavior? (You can also link to an open issue here)
jqLite's camelCasing logic converts not only dashes but also underscores & colons; it also collapses multiple dashes etc.

What is the new behavior (if this is a feature change)?
jqLite's camelCasing logic is separated from the one used in compile/sce and more strict, making jqLite more similar to the jQuery behavior. This is also needed to make the new planned data key camelCasing work like in jQuery 3.

Does this PR introduce a breaking change?
Yes.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

jqLite needs camelCase for it's css method; it should only convert one dash
followed by a lowercase letter to an uppercase one; it shouldn't touch
underscores, colons or collapse multiple dashes into one. This is behavior
of jQuery 3 as well.

This commit separates jqLite camelCasing from the $compile one (and $sce but
that's an internal-only use). The $compile version should behave as before.

Also, jqLite's css camelCasing logic was put in a separate function and
refactored: now the properties starting from an uppercase letter are used by
default (i.e. Webkit, not webkit) and the only exception is for the -ms- prefix
that is converted to ms, not Ms. This makes the logic clearer as we're just
always changing a dash followed by a lowercase letter by an uppercase one; this
is also how it works in jQuery.

Ref #15126
Fix #7744

gkalpak · 2016-10-12T14:11:10Z

src/jqLite.js

@@ -136,22 +136,31 @@ JQLite._data = function(node) {
 function jqNextId() { return ++jqId; }


-var SPECIAL_CHARS_REGEXP = /([:\-_]+(.))/g;
-var MOZ_HACK_REGEXP = /^moz([A-Z])/;
+var DASH_LOWERCASE_REGEXP = /-([a-z])/g;


I wonder if we should match [A-Z] as well.

We shouldn't. That's the whole point of this change!

And I even wrote some tests to ensure that's not getting converted. ;)

gkalpak · 2016-10-12T14:16:42Z

src/ng/sce.js

@@ -27,6 +27,13 @@ var SCE_CONTEXTS = {

 // Helper functions follow.

+var UNDERSCORE_LOWERCASE_REGEXP = /_([a-z])/g;


Here too, I wonder what might break by only matching lowercase letters.

Nothing. ;) This is a purely internal function, only used to iterate once through a few constant strings during the initialization.

gkalpak · 2016-10-12T14:18:50Z

test/jqLiteSpec.js

    });

+    it('should not convert slashes followed by a non-letter', function() {


slashes --> dashes (?)

mgol · 2016-10-12T14:28:38Z

I updated the commit message, adding a breaking change info.

petebacondarwin · 2016-10-12T15:32:32Z

test/jqLiteSpec.js

+      expect(a.style.fooBar).toBe('bar');
+    });
+
+    it('should covert dash-separated strings to camelCase', function() {


covert -> convert

petebacondarwin · 2016-10-12T15:33:19Z

test/jqLiteSpec.js

+      expect(a.style['foo:bar_baz']).toBe('baz');
+    });
+
+    it('should covert leading dashes followed by a lowercase letter', function() {


covert -> convert

petebacondarwin · 2016-10-12T15:34:29Z

test/jqLiteSpec.js

+
+      jqA.css('foo-42- -a-B', 'foo');
+
+      expect(a.style['foo-42- A-B']).toBe('foo');


This is a crazy case. Do we really care that this is what it does?

I don't expect keys exactly like that to be used in practice but this string tests the fact that only -[a-z] gets converted pretty well. I can split it into separate cases if you prefer but I thought one string should suffice.

petebacondarwin · 2016-10-12T15:35:02Z

test/jqLiteSpec.js

+      expect(a.style['foo-42- A-B']).toBe('foo');
+    });
+
+    it('should covert the -ms- prefix to ms instead of Ms', function() {


covert -> convert

petebacondarwin · 2016-10-12T15:36:33Z

test/jqLiteSpec.js

+      expect(kebabToCamel('foo-42- -a-B')).toBe('foo-42- A-B');
+    });
+
+    it('should not covert browser specific css properties in a special way', function() {


covert -> convert

petebacondarwin · 2016-10-12T15:36:42Z

test/jqLiteSpec.js

+      expect(kebabToCamel('foo:bar_baz')).toBe('foo:bar_baz');
+    });
+
+    it('should covert leading dashes followed by a lowercase letter', function() {


covert -> convert

mgol · 2016-10-12T17:14:32Z

Typos fixed.

mgol · 2016-10-12T17:50:58Z

@petebacondarwin Should it be fix instead of refactor as well? This is aligning with jQuery amongst other things.

mgol · 2016-10-12T18:00:25Z

I tweaked the commit message (see the first commit).

@petebacondarwin does that look OK?

petebacondarwin · 2016-10-12T19:29:18Z

Better!

jqLite needs camelCase for it's css method; it should only convert one dash followed by a lowercase letter to an uppercase one; it shouldn't touch underscores, colons or collapse multiple dashes into one. This is behavior of jQuery 3 as well. This commit separates jqLite camelCasing from the $compile one (and $sce but that's an internal-only use). The $compile version should behave as before. Also, jqLite's css camelCasing logic was put in a separate function and refactored: now the properties starting from an uppercase letter are used by default (i.e. Webkit, not webkit) and the only exception is for the -ms- prefix that is converted to ms, not Ms. This makes the logic clearer as we're just always changing a dash followed by a lowercase letter by an uppercase one; this is also how it works in jQuery. Ref angular#15126 Fix angular#7744 BREAKING CHANGE: before, when Angular was used without jQuery, the key passed to the css method was more heavily camelCased; now only a single (!) hyphen followed by a lowercase letter is getting transformed. This also affects APIs that rely on the css method, like ngStyle. If you use Angular with jQuery, it already behaved in this way so no changes are needed on your part. To migrate the code follow the example below: Before: HTML: // All five versions used to be equivalent. <div ng-style={background_color: 'blue'}></div> <div ng-style={'background:color': 'blue'}></div> <div ng-style={'background-color': 'blue'}></div> <div ng-style={'background--color': 'blue'}></div> <div ng-style={backgroundColor: 'blue'}></div> JS: // All five versions used to be equivalent. elem.css('background_color', 'blue'); elem.css('background:color', 'blue'); elem.css('background-color', 'blue'); elem.css('background--color', 'blue'); elem.css('backgroundColor', 'blue'); // All five versions used to be equivalent. var bgColor = elem.css('background_color'); var bgColor = elem.css('background:color'); var bgColor = elem.css('background-color'); var bgColor = elem.css('background--color'); var bgColor = elem.css('backgroundColor'); After: HTML: // Previous five versions are no longer equivalent but these two still are. <div ng-style={'background-color': 'blue'}></div> <div ng-style={backgroundColor: 'blue'}></div> JS: // Previous five versions are no longer equivalent but these two still are. elem.css('background-color', 'blue'); elem.css('backgroundColor', 'blue'); // Previous five versions are no longer equivalent but these two still are. var bgColor = elem.css('background-color'); var bgColor = elem.css('backgroundColor');

petebacondarwin · 2016-10-13T06:54:49Z

Landed via #15238

mgol added component: $compile component: jqLite type: refactor needs: breaking change component: misc core component: ngSanitize labels Oct 12, 2016

mgol added this to the 1.6.0-rc.0 milestone Oct 12, 2016

mgol self-assigned this Oct 12, 2016

googlebot added the cla: yes label Oct 12, 2016

This was referenced Oct 12, 2016

ngStyle ignores properties with underscore only when jQuery is installed #7744

Closed

refactor(data): camelCase keys in jqLite#data #15238

Closed

mgol force-pushed the camelCase branch 2 times, most recently from 2c70cca to 705eba2 Compare October 12, 2016 13:09

gkalpak reviewed Oct 12, 2016

View reviewed changes

mgol force-pushed the camelCase branch from 705eba2 to 008baa9 Compare October 12, 2016 14:28

mgol force-pushed the camelCase branch from 008baa9 to 99b2dde Compare October 12, 2016 14:30

petebacondarwin reviewed Oct 12, 2016

View reviewed changes

mgol force-pushed the camelCase branch from 06aa615 to 145b4bb Compare October 12, 2016 17:59

mgol force-pushed the camelCase branch from 145b4bb to b9c338d Compare October 12, 2016 22:45

petebacondarwin closed this Oct 13, 2016

mgol deleted the camelCase branch October 13, 2016 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(*): separate jqLite/compile/sce camelCasing logic #15249

refactor(*): separate jqLite/compile/sce camelCasing logic #15249

mgol commented Oct 12, 2016

gkalpak Oct 12, 2016 •

edited

Loading

mgol Oct 12, 2016

mgol Oct 12, 2016

gkalpak Oct 12, 2016

mgol Oct 12, 2016

gkalpak Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin Oct 12, 2016

petebacondarwin Oct 12, 2016

petebacondarwin Oct 12, 2016

mgol Oct 12, 2016

petebacondarwin Oct 12, 2016

petebacondarwin Oct 12, 2016

petebacondarwin Oct 12, 2016

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

petebacondarwin commented Oct 13, 2016

		@@ -27,6 +27,13 @@ var SCE_CONTEXTS = {

		// Helper functions follow.

		var UNDERSCORE_LOWERCASE_REGEXP = /_([a-z])/g;

		});

		it('should not convert slashes followed by a non-letter', function() {


		jqA.css('foo-42- -a-B', 'foo');

		expect(a.style['foo-42- A-B']).toBe('foo');

refactor(*): separate jqLite/compile/sce camelCasing logic #15249

refactor(*): separate jqLite/compile/sce camelCasing logic #15249

Conversation

mgol commented Oct 12, 2016

gkalpak Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

petebacondarwin commented Oct 13, 2016

gkalpak Oct 12, 2016 •

edited

Loading