feat($compile): add support for arbitrary property and event bindings #16614

jbedard · 2018-06-26T07:55:50Z

Here's the initial work for arbitrary property and event bindings. I think ng-attr-* and ng-on-* are essentially ready. The main thing to do is the $sce logic which I'm very unfamiliar with (I've never used it!), so any help/feedback in that area would be great. Also docs...

Could also think about doing the ng-bindon-* as @gkalpak has been calling it (something equivalent to [(*)] in Angular).

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

TODO:

gkalpak

I haven't looked at the tests, but the rest looks reasonable 🎉

gkalpak · 2018-06-26T09:52:07Z

src/ng/compile.js

+   *
+   * Defines the security context for HTML properties bound by ng-prop-*
+   * 
+   * @param {string} the context type


{string} the context type --> {string} ctx the context type
(Same below.)

gkalpak · 2018-06-26T09:55:20Z

src/ng/compile.js

+   */
+  (function setupPropertyContexts() {
+    function registerContext(ctx, values) {
+      forEach(values, function(v) { PROP_CONTEXTS[v.toLowerCase()] = ctx; });


Since this is an internal function, we can avoid the .toLowerCase() (and ensure values are lowercased).

I'd prefer keeping it so the properties below can have the standard property casing. This also makes the list of properties almost the exact same as https://github.com/angular/angular/blob/6.0.6/packages/compiler/src/schema/dom_security_schema.ts#L31-L58

gkalpak · 2018-06-26T09:56:24Z

src/ng/compile.js

+   * @param {string} the element name or '*' to match any element
+   * @param {string} the property name
+   */
+  this.addPropertyContext = function(ctx, elementName, propertyName) {


If you really want this to be an add operation, you need to ensure that the key doesn not already exists 😁

I added the ctxoverride error for this

gkalpak · 2018-06-26T09:58:02Z

src/ng/compile.js

+      'object|codebase',  'object|data',
+      'script|src',
+    ]);
+  })();


I wonder if it is worth avoiding the size increase in core and move these to a separated module.

I'd say since we already have attribute sanitizing built in we should probably do the same for properties?

I wonder how much this affects the overall size. Since these are strings, they can't be minified, but there are less of them than I thought there would be and they probably compress fine. So, I don't think this is a problem, but am still curious about the size increase.

As a quick (and inaccurate) test, this setup method on it's own is 1.2kb and 0.7kb minfied. Should minify a bit better when not isolated and then gzip would help a bit more...

Size is a bit of a concern, especially because there might be very limited take up of this feature.
What if we put the core changes into angular.js but always throw an exception if the ngProp module is not loaded. Then we could move as much of this sanitization (and anything else auxiliary) into that module and keep the core changes to a minimum?

Does that mean ngProp can not be used at all without that extra module though?

Yes. That would be the case. I think that might be reasonable since this is a specialised scenario that many AngularJS applications would not use...?

I'd really prefer if it was usable without adding another file/module, especially since that file will just be this property list... all the actual functionality is baked into $compile and will already be there. Although I guess the ng-on-* is the one that will be used more, so that one is more important to me to be baked in.

Is it mainly just the size?

I could probably make this a bit smaller (concatenate all the strings instead of arrays of strings...).

I mentioned I'd try to make this minify better... looks like the closure compiler already does what I was thinking of (convert ['a', 'b', 'c'] to 'a b c'.split(' ')).

When minifying only this IIFE it is 371 bytes gzipped. When minifying the full library, then measuring just this IIFE it is 348 bytes gzipped (however it might have been minified to gzip better along with the rest of the app?).

So basically ~350 bytes is a good estimate.

gkalpak · 2018-06-26T10:20:03Z

src/ng/compile.js

+                'ng- versions (such as ng-click or ng-on-click instead of ng-prop-onclick) instead.');
+      }
+
+      var nodeName = nodeName_(node);


Nit: Since we are computing the name here, we could pass it to getTrustedPropContext() directly.

I wanted to keep the signatures of getTrustedPropContext and getTrustedAttrContext to be the same. WDYT?

However it looks like getTrustedAttrContext is also called right beside a nodeName_(node) call like this, so I could change both...

Done (jbedard@bd63689)

gkalpak · 2018-06-26T10:23:10Z

src/ng/compile.js

+              scope.$watch(fn, function propertyWatchActionFn(value) {
+                if (value) {
+                  // Sanitize img[srcset] + source[srcset] values.
+                  if ((nodeName === 'img' || nodeName === 'source') && propName === 'srcset') {


Since these values are available beforehand, we could create different watchActionsFns and avoid checking them over and over.

I'll try this, but will also do it for attributes since the code is the exact same. Should have a fixup commit doing this shortly...

EDIT: nevermind attributes don't do the exact same (this condition is in $attr.set instead), so I'll make this change only here 👍

Done, but instead of different watchActionFns I created different sanitizer methods that a common watchActionFn invokes...

gkalpak · 2018-06-26T10:24:34Z

src/ng/directive/ngEventDirs.js

+function createEventDirective($parse, $rootScope, attrName, eventName, forceAsync) {
+  return {
+    restrict: 'A',
+    priority: 0,


Why the explicit 0?

jbedard · 2018-06-28T06:11:48Z

test/ng/compileSpec.js

+        $rootScope.name = 'angular';
+        $rootScope.$apply();
+        log('digest=' + element.prop('myName'));
+        expect(log).toEqual('compile=undefined; preLinkP101=undefined; preLinkP0=undefined; postLink=undefined; postCompile=undefined; digest=angular');


Here's one I wasn't sure about...

With ng-attr-* the attribute is evaluated on pre-link so it is available to other link functions before the first $watch applies. I have not done that for ng-prop-*. Should we?

I don't think that's very important for properties. Listening to attributes is / was quite important in AngularJS, but with props that's different.

Just make sure it is documented, so that people don't assume it works the same as ngAttr.

But why not make it the same as ngAttr?

I think for $attr it is important because properties are put into $attrs which is part of the directive compile/link API. But there is no equivalent API for properties. Unless you consider $element.prop/attr the API...

I think I'd vote to make it the same as ngAttr just for consistency. Unless someone can think of a case where assigning to a DOM property one extra time (init + $watch) would be an issue?

jbedard · 2018-06-28T06:20:59Z

test/ng/compileSpec.js

+        element = $compile('<img ng-prop-src="testUrl"></img>')($rootScope);
+        // Some browsers complain if you try to write `javascript:` into an `img[src]`
+        // So for the test use something different
+        $rootScope.testUrl = $sce.trustAsMediaUrl('someuntrustedthing:foo();');


Note that for the ng-attr- version of this test this string was someUnstrustedThing:foo(). But it seems like for properties the browser (at least some) change the protocol to lowercase when assigned to the property, so the expect below failed (which also has the mixed case for the attribute version). I changed both this line and the expect to be all lower case to avoid this since I don't think it matters here...

jbedard · 2018-06-28T06:23:08Z

test/ng/compileSpec.js

+        expect(element.prop('src')).toEqual('someuntrustedthing:foo();');
+      }));
+
+      it('should sanitize concatenated values even if they are trusted', inject(function($rootScope, $compile, $sce) {


Is this test and the next correct for expressions like this? I'm not sure if expressions vs interpolation are or should be handled different...?

I think so. Was the consensus yesterday that ngProp can / should only consider the result of all expressions?

jbedard · 2018-06-28T19:15:45Z

src/ng/compile.js

-                }
+
+            if (isNgProp) {
+              attrs[ngAttrName] = value;


Is this correct? Which attribute should have the `value? If any?

Is attrs the attrs object that gets passed to the link fn / controller? I don't think ngProp should set the attribute here.

Seems right to me.

Yes this attrs is what gets passed to the compile / link / controller.

Note that here we are assigning attrs.ngPropFoo, not attrs.foo like ng-attr-foo would. Also the value is the expression that will later get $parsed. While I agree it's weird putting anything prop related on attrs, I did this so we can...
a) use the existing createEventDirective from ngEventDirs.js
b) follow the same pattern as ngEventDirs.js and ng-attr-* where the value that gets $parse-ed (or $interpolate-ed for ng-attr) is read of the attribute in the compile step of the directive

I'm not sure how to achieve both of those without putting the expression onto attrs...

Ideas? Opinions?

In my comment above, I meant it seems fine to me to have attrs[ngAttrName] = value.
Even if ngProp* assigns a property, it is still an attribute on the DOM element, so I think it should be reflected on $attrs.

jbedard · 2018-06-29T03:50:52Z

test/ng/compileSpec.js

+        });
+      });
+
+      it('should use $$sanitizeUri on concatenated trusted values', function() {


Narretz · 2018-06-29T08:06:26Z

test/ng/compileSpec.js

+        expect(element.prop('src')).toEqual('someuntrustedthing:foo();');
+      }));
+
+      it('should sanitize concatenated values even if they are trusted', inject(function($rootScope, $compile, $sce) {


I think so. Was the consensus yesterday that ngProp can / should only consider the result of all expressions?

Narretz · 2018-06-29T08:08:06Z

test/ng/compileSpec.js

+        expect(element.prop('src')).toEqual('unsafe:untrusted:foo();untrusted:foo();');
+      }));
+
+      it('should not sanitize other properties', inject(function($compile, $rootScope) {


I feel like this is obsolete, too. It doesn't make sense in ngProp context at least.

Narretz · 2018-06-29T08:09:40Z

test/ng/compileSpec.js

+      });
+
+      it('should sanitize all uris in srcset', inject(function($rootScope, $compile) {
+        element = $compile('<img srcset="{{testUrl}}"></img>')($rootScope);


Shouldn't this be ng-prop-srcset="testUrl"?

Narretz · 2018-06-29T08:14:36Z

src/ng/compile.js

-                }
+
+            if (isNgProp) {
+              attrs[ngAttrName] = value;


Is attrs the attrs object that gets passed to the link fn / controller? I don't think ngProp should set the attribute here.

gkalpak

A couple of thoughts:

We also need to document how people can bind to case-sensitive properties using case-insensitive attributes (e.g. ng-prop-inner_h_t_m_l) 😁
It would be interesting to see how the compiler performance is affected by the changes.

gkalpak · 2018-06-29T08:27:38Z

docs/content/error/$compile/ctxoverride.ngdoc

+@fullName Context Override
+@description
+
+This error occurs when the security context for an attribute is defined multiple times under different security contexts.


attribute --> property (?)

gkalpak · 2018-06-29T08:28:11Z

docs/content/error/$compile/ctxoverride.ngdoc

+```
+$compileProvider.addPropertyContext("my-element", "src", "mediaUrl");
+$compileProvider.addPropertyContext("my-element", "src", "resourceUrl");    //throws
+```


I think it would be useful to have a link to the docs somewhere in this error message.

Which docs specifically?

The addPropertyContext docs 😁

We could and probably should create directive doc entries for ngProp and ngOn even though they are not real directives. NgAttr is mentioned in the attribute guide but that isn't so great either.

gkalpak · 2018-06-29T08:29:05Z

src/ng/compile.js

+   * @name $compileProvider#addPropertyContext
+   * @description
+   *
+   * Defines the security context for HTML properties bound by ng-prop-*


What about SVG properties 😁

Looks like we normally say "HTML/SVG"?

I don't know. Maybe 😁
But ngProp should also work on SVG elements (which are first-class citizen in AngularJS 🤓).

I don't think this is HTML at all, it should be "DOM properties" no?

gkalpak · 2018-06-29T08:31:37Z

src/ng/compile.js

+   * 
+   * @param {string} elementName the element name or '*' to match any element
+   * @param {string} propertyName the property name
+   * @param {string} ctx the context type


It might be useful to link to $sce here (or else people would have no idea what contexts are supported).

The description of this parameter could be a bit more descriptive. This is what is on the $sce.trustAs method for a similar parameter:

The context in which this value is safe for use, e.g. $sce.URL,
$sce.RESOURCE_URL, $sce.HTML, $sce.JS or $sce.CSS.

gkalpak · 2018-06-29T08:38:16Z

src/ng/compile.js

+      'object|codebase',  'object|data',
+      'script|src',
+    ]);
+  })();


Why the IIFE?

It hides the registerContext helper method, and I guess the main reason is to isolate the block of code (mostly) copied from Angular.

Hide from who? We don't this IIRC.

Hide from you!

I guess I just like keeping the stuff copied from Angular in one place. Prevents people from using it or moving the helper method elsewhere etc.

I can drop the IIFE if people don't like it though...

You keep talking about this registerContext helper method, but I don't see it 😛

Perfect, the IIFE is hiding it well then 😛

Does the IIFE help the closure compiler to reduce the code size?

Does the IIFE help the closure compiler to reduce the code size?

I think since the full library is already in an IIFE this one doesn't help, or might even make it larger since the closure compiler probably doesn't remove this one... That is a valid concern that I agree might be worth removing the IIFE for.

As mentioned in a different comment... the IIFE probably adds 5-10 bytes ("function(){}()".length + gzipping). The closure compiler does not remove it like I was hoping, I guess because it would cause some vars/functions to be exposed a level higher.

gkalpak · 2018-06-29T09:09:11Z

src/ng/compile.js

            ngAttrName = directiveNormalize(name);
-            isNgAttr = NG_ATTR_BINDING.test(ngAttrName);
-            if (isNgAttr) {
+            if (ngPrefixMatch = ngAttrName.match(NG_PREFIX_BINDING)) {


Shouldn't there be an extra pair of parenthesis here, to indicate the assignment is intentional?

gkalpak · 2018-06-29T09:11:58Z

src/ng/compile.js

-                }
+
+            if (isNgProp) {
+              attrs[ngAttrName] = value;


Seems right to me.

gkalpak · 2018-06-29T09:16:53Z

src/ng/compile.js


    function addAttrInterpolateDirective(node, directives, value, name, isNgAttr) {
-      var trustedContext = getTrustedContext(node, name);
+      var tag = nodeName_(node);


Why not nodeName for consistency with addPropertyDirective 😁

I think because getTrustedAttrContext used tag, but I agree and will update this one to nodeName...

gkalpak · 2018-06-29T09:23:24Z

src/ng/compile.js

+          return {
+            pre: function ngPropPreLinkFn(scope, $element) {
+              scope.$watch(fn, function propertyWatchActionFn(value) {
+                $element.prop(propName, value && sanitizer(value));


The value && check is redundant. .prop(propName, sanitizer(value)) should be enough (and gives sanitizers a chance to decide whether a falsy value is safe or not).

gkalpak · 2018-06-29T09:34:47Z

test/ng/compileSpec.js

+      expect($rootScope.name).toBe('Misko3');
+    }));
+
+    it("should use angular.element(x).on() API to add listener", inject(function($compile, $rootScope) {


...to add listener --> ...to add listener, so that Jason can hook his ngUpgrade hacks into it 😛

Narretz · 2018-06-29T11:47:42Z

Wrt compiler performance - I think we should put ngProp and ngOn behind a flag, similar to css class / comment directives. This way, there's no hit if you don't need it.

jbedard · 2018-06-29T16:27:14Z

WRT performance I am not too concerned. The only changes effecting existing apps should be

the ng-attr-* regex is a bit more complicated (this runs on every attribute!)
if (isNgProp) { ... } else if (isNgOn) { ... } else { ... old code ... } was added, so basically adds 2 if (variable) statements

I don't think this is worth a flag IMO, at least not just for performance reasons (based on my assumptions that is).

jbedard · 2018-06-29T16:29:09Z

src/ng/compile.js

-            var multiElementMatch = ngAttrName.match(MULTI_ELEMENT_DIR_RE);
-            if (multiElementMatch && directiveIsMultiElement(multiElementMatch[1])) {
+            // support *-start / *-end multi element directives
+            } else if ((multiElementMatch = ngAttrName.match(MULTI_ELEMENT_DIR_RE)) && directiveIsMultiElement(multiElementMatch[1])) {
              attrStartName = name;
              attrEndName = name.substr(0, name.length - 5) + 'end';
              name = name.substr(0, name.length - 6);
            }

            nName = directiveNormalize(name.toLowerCase());
            attrsMap[nName] = name;


Is this right for ng-on/prop? I don't think we'd want to "map" ng-on-foo to the foo attribute... Would we want to map it at all? To ngAttrName (still containing ng-on/prop prefix) instead of name?

It is fine imo. We should indeed not reflect the property/event as attribute, but keeping track of the ngProp/On prefixed attribute sounds reasonable and consistent (since the attribute is there).

I think your comment is true regarding the attrs[ngAttrName] = value; a few lines down. But what about the "mapping" on this line?

This line creates the ngPropFoo => foo mapping for https://docs.angularjs.org/api/ng/type/$compile.directive.Attributes#$attr ...

Oh, you are right. I don't think we want to map foo to ngPropFoo.
(Mapping ng-prop-foo to ngPropFoo sounds right (for consistency), although it would probably have no effect.)

petebacondarwin · 2018-07-02T09:26:45Z

src/ng/compile.js

+
+  /**
+   * @ngdoc method
+   * @name $compileProvider#addPropertyContext


I think this method needs a more descriptive name. Context is a pretty ambiguous term. Perhaps it could be addPropertyTrustContext or addPropertySecurityContext or something?

petebacondarwin · 2018-07-02T09:43:35Z

src/ng/compile.js

+   * - STYLE => CSS
+   * - various URL => MEDIA_URL
+   */
+  (function setupPropertyContexts() {


Tempting to name this function registerNativePropertyContexts or something similar?

petebacondarwin · 2018-07-02T10:10:31Z

src/ng/compile.js

+
+            if (isNgProp) {
+              attrs[ngAttrName] = value;
+              addPropertyDirective(node, directives, ngAttrName, name);


Why not pass value in here since we know it and it must be a static template string? That way you don't need to add the value to the attrs map?

petebacondarwin · 2018-07-02T10:11:53Z

src/ng/compile.js

+      directives.push({
+        priority: 100,
+        compile: function ngPropCompileFn(_, attr) {
+          var fn = $parse(attr[attrName]);


We could receive this value as a parameter to the addPropertyDirective and avoid adding it to the attrs map.

Note that I wanted to read the attribute here at compile time so it is equivalent to attribute interpolation and $parseing of ng-event and ng-on-* which all read the attribute at compile time (although interpolation then reads it again at link time :|).

petebacondarwin · 2018-07-02T10:14:33Z

src/ng/directive/ngEventDirs.js

    }];
  }
 );

+function createEventDirective($parse, $rootScope, attrName, eventName, forceAsync) {


We could have a value param on this function which is used rather than attr[attrName] if it is defined.
That way you can avoid adding the value to the attrs map in the case of ng-on-, no?

I wanted to avoid making changes to the existing event directives, and I thought making ng-on-* the same would also be good. In all those cases the attr[attrName] is not read until compile time, and those attributes do exist in the attrs map...

petebacondarwin · 2018-07-02T10:16:05Z

src/ng/compile.js

+              addPropertyDirective(node, directives, ngAttrName, name);
+            } else if (isNgEvent) {
+              attrs[ngAttrName] = value;
+              addEventDirective(directives, ngAttrName, name);


We could pass the value here as an extra param, and this function would know to use that rather than reading off the attrs map.

petebacondarwin

Maybe I am missing something but I feel really uncomfortable about re-using the $attr stuff for these property directives. I believe that a bit of refactoring would allow us to avoid putting these values on that map and give us tighter control over how these special directives are handled.

gkalpak · 2018-07-02T11:01:57Z

Why? What is the problem with having attributes reflected on $attrs? Why is it OK for $attrs.ngClick to be present, but not for $attrs.ngOnFooEvent?

jbedard · 2018-07-03T05:38:46Z

Similar to what @gkalpak said, I think ngClick and ngOnClick should be equivalent other then the extra "on" in the attribute name. Similar for properties... ngPropDisabled and ngDisabled should be equivalent, except for interpolation vs expression (and the fact that ngDisabled does some to-boolean casting).

I think this means that $attrs will contain ngOnClick and ngPropDisabled, but will not contain click or disabled or anything like that...

petebacondarwin · 2018-07-03T07:01:44Z

Well I never! ngClick does get added to the attrs. That is interesting...

OK so I relent on my campaign to remove it. I'll take a look at the security stuff and tests later today.

jbedard · 2018-07-04T03:38:05Z

Added a few more fixups including a fix for the attr.$attr mappings (4995d0e) along with some tests that failed before it (the ng-attr test never failed, but seemed good to add at the same time).

gkalpak · 2018-07-04T09:51:30Z

src/ng/compile.js

-            if (isNgAttr || !attrs.hasOwnProperty(nName)) {
+            if (isNgProp || isNgEvent) {
+              attrs[nName] = value;
+              attrsMap[nName] = attr.name;


For reference, this is the difference between how ngAttr and ngProp attributes get mapped (and it looks reasonable to me):

// HTML: x-ng-attr-my-form_action attrsMap['myFormaction'] = 'my-formAction' // HTML: x-ng-prop-my-form_action attrsMap['ngPropMyForm_action'] = 'x-ng-prop-my-form_action'

Should the _s not behave the same?

I'll try to add some tests with _s for both ngAttr and ngProp

I've added a commit c032255 (or two 7835fc3) to test this.

The result is a bit different then you assumed for the ngProp case:

// HTML: x-ng-attr-my-form_action attrsMap['myFormaction'] = 'my-formAction' // HTML: x-ng-prop-my-form_action attrsMap['ngPropMyFormAction'] = 'x-ng-prop-my-form_action'

Notice the capital A for the ng-prop one... I think that is actually more correct then the ng-attr one. This is because we invoke directiveNormalize in two different ways:

https://github.com/angular/angular.js/blob/v1.7.2/src/ng/compile.js#L2258 - on the raw attribute name

https://github.com/angular/angular.js/blob/v1.7.2/src/ng/compile.js#L2274 - on the no-prefix attribute name, but in all lower case. This lower casing causes the mapping to be myFormaction instead of myFormAction

Is this a bug? Or a feature and we should be copying for ng-prop? 🤔

I think that behaviour for ngAttr looks like a bit of a bug to me... especially given this test:

it('should work if they are prefixed with x- or data-', inject(function($compile, $rootScope) { $rootScope.dimensions = '0 0 0 0'; $rootScope.number = 0.42; $rootScope.scale = 1; element = $compile('<svg data-ng-attr-view_box="{{dimensions}}">' + '<filter x-ng-attr-filter_units="{{number}}">' + '<feDiffuseLighting data-ng:attr_surface_scale="{{scale}}">' + '</feDiffuseLighting>' + '<feSpecularLighting x-ng:attr_surface_scale="{{scale}}">' + '</feSpecularLighting></filter></svg>')($rootScope); expect(element.attr('viewBox')).toBeUndefined(); $rootScope.$digest(); expect(element.attr('viewBox')).toBe('0 0 0 0'); expect(element.find('filter').attr('filterUnits')).toBe('0.42'); expect(element.find('feDiffuseLighting').attr('surfaceScale')).toBe('1'); expect(element.find('feSpecularLighting').attr('surfaceScale')).toBe('1'); }));

suggests that element.attr('...') expects the attribute to be capitalised in the way we expect.

Yeah... the element.attr('...') and the real attribute name are correct, it's only the nName variable which is what is used on $attrs that is wrong.

Should we fix it though? If anyone is depending on these bad names it would be hard to find+fix if we fixed the bug. However it would be nice to fix the bug...

Created #16624

Narretz · 2018-07-26T09:08:33Z

test/ng/ngPropSpec.js

+  describe('*[style]', function() {
+    // Support: ??
+    // Some browsers throw when assignging to HTMLElement.style
+    function canAssingStyleProp() {


typo: canAssingStyleProp

Narretz · 2018-07-26T09:10:18Z

test/ng/ngPropSpec.js

+  });
+
+  describe('*[style]', function() {
+    // Support: ??


Don't we know which browser is this? IE9?

Where I was looking all the browsers test output as randomly output mixed together so I couldn't tell. Are there per-browser logs somewhere?

See https://travis-ci.org/angular/angular.js/jobs/406066641#L750

Although I would guess it is IE9 because 2 tests failed in IE9 and that error ("Attempted to assign to readonly property") occurred 2 times...

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo=”x”` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar=”barOccured($event)”` will add a listener to the “bar” event and invoke the `barOccured($event)` expression. For properties or events using mixed case underscores can be used before capital letters. For example `ng-prop-my_prop="x"` will bind `x` to the `myProp` property, and `ng-on-my_custom_event="x()"` will invoke `x()` on the `myCustomEvent` event. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

jbedard · 2018-07-30T20:23:17Z

I've addressed the typo + support-?? issues, updated the commit message, rebased and squashed.

gkalpak

💯 x 🎉

gkalpak · 2018-07-30T21:05:16Z

There is a typo in the commit message:

[...] events could be bound using such as ng-click [...]

The following also sounds a little confusing (since there are no capital letters in the attribute to put the underscore before 😁):

For properties or events using mixed case underscores can be used before capital letters.

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo=”x”` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar=”barOccured($event)”` will add a listener to the “bar” event and invoke the `barOccured($event)` expression. For properties or events using mixed case underscores can be used before capital letters. For example `ng-prop-my_prop="x"` will bind `x` to the `myProp` property, and `ng-on-my_custom_event="x()"` will invoke `x()` on the `myCustomEvent` event. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo=”x”` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar=”barOccured($event)”` will add a listener to the “bar” event and invoke the `barOccured($event)` expression. For properties or events using mixed case, underscores can be used before capital letters. For example `ng-prop-my_prop="x"` will bind `x` to the `myProp` property, and `ng-on-my_custom_event="x()"` will invoke `x()` on the `myCustomEvent` event. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo=”x”` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using directives such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar=”barOccured($event)”` will add a listener to the “bar” event and invoke the `barOccured($event)` expression. For properties or events using mixed case, underscores can be used before capital letters. For example `ng-prop-my_prop="x"` will bind `x` to the `myProp` property, and `ng-on-my_custom_event="x()"` will invoke `x()` on the `myCustomEvent` event. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

jbedard · 2018-07-30T21:17:17Z

I've broken up the commit message into <100 char lines. Fixed the events sentance.

The following also sounds a little confusing (since there are no capital letters in the attribute to put the underscore before 😁):

I added a comma in there. Is that clearer?

Thanks for reviewing the commit message. Let me know if you find any other issues or things that could be clearer or added.

gkalpak · 2018-07-31T11:34:18Z

I also noticed that some double-quotes in the commit message are fancier than others (” vs "). It's good to have non-fancy quotes everywhere.

I would expand a bit on the "binding to camel case" thing. Maybe something like:

Since HTML attributes are case-insensitive, you need to convert property and event names to snake_case for ng-prop-* and ng-on-*. For example, to bind to the fooBar property use ng-prop-foo_bar...

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo="x"` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using directives such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar="barOccured($event)"` will add a listener to the “bar" event and invoke the `barOccured($event)` expression. Since HTML attributes are case-insensitive, property and event names are specified in snake_case for `ng-prop-*` and `ng-on-*`. For example, to bind property `fooBar` use `ng-prop-foo_bar`, to listen to event `fooBar` use `ng-on-foo_bar`. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

jbedard · 2018-07-31T21:11:29Z

I've updated the message again

gkalpak · 2018-07-31T21:44:46Z

I still see at least on fancy quote: “bar" 😁
Other than that looks great 💯

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo="x"` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using directives such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar="barOccured($event)"` will add a listener to the "bar" event and invoke the `barOccured($event)` expression. Since HTML attributes are case-insensitive, property and event names are specified in snake_case for `ng-prop-*` and `ng-on-*`. For example, to bind property `fooBar` use `ng-prop-foo_bar`, to listen to event `fooBar` use `ng-on-foo_bar`. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

jbedard · 2018-08-01T01:33:15Z

Fixed the quotes again

Narretz · 2018-08-01T07:20:38Z

Travis is green, looks like we're good to go!

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo="x"` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using directives such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar="barOccured($event)"` will add a listener to the "bar" event and invoke the `barOccured($event)` expression. Since HTML attributes are case-insensitive, property and event names are specified in snake_case for `ng-prop-*` and `ng-on-*`. For example, to bind property `fooBar` use `ng-prop-foo_bar`, to listen to event `fooBar` use `ng-on-foo_bar`. Fixes angular#16428 Fixes angular#16235 Closes angular#16614

…ings Properties: Previously only arbitrary DOM attribute bindings were supported via interpolation such as `my-attribute="{{expression}}"` or `ng-attr-my-attribute="{{expression}}"`, and only a set of distinct properties could be bound. `ng-prop-*` adds support for binding expressions to any DOM properties. For example `ng-prop-foo="x"` will assign the value of the expression `x` to the `foo` property, and re-assign whenever the expression `x` changes. Events: Previously only a distinct set of DOM events could be bound using directives such as `ng-click`, `ng-blur` etc. `ng-on-*` adds support for binding expressions to any DOM event. For example `ng-on-bar="barOccured($event)"` will add a listener to the "bar" event and invoke the `barOccured($event)` expression. Since HTML attributes are case-insensitive, property and event names are specified in snake_case for `ng-prop-*` and `ng-on-*`. For example, to bind property `fooBar` use `ng-prop-foo_bar`, to listen to event `fooBar` use `ng-on-foo_bar`. Fixes #16428 Fixes #16235 Closes #16614

googlebot added the cla: yes label Jun 26, 2018

gkalpak reviewed Jun 26, 2018

View reviewed changes

Narretz added this to the 1.7.x milestone Jun 27, 2018

Narretz added type: feature component: $compile frequency: moderate severity: inconvenient labels Jun 27, 2018

jbedard force-pushed the 16428-web-components branch from 81d1529 to bd63689 Compare June 28, 2018 02:52

jbedard commented Jun 28, 2018

View reviewed changes

jbedard commented Jun 29, 2018

View reviewed changes

Narretz suggested changes Jun 29, 2018

View reviewed changes

gkalpak reviewed Jun 29, 2018

View reviewed changes

jbedard commented Jun 29, 2018

View reviewed changes

petebacondarwin reviewed Jul 2, 2018

View reviewed changes

petebacondarwin suggested changes Jul 2, 2018

View reviewed changes

gkalpak reviewed Jul 4, 2018

View reviewed changes

Narretz reviewed Jul 26, 2018

View reviewed changes

jbedard force-pushed the 16428-web-components branch from 0a5c935 to 9c461b3 Compare July 30, 2018 20:22

gkalpak approved these changes Jul 30, 2018

View reviewed changes

jbedard force-pushed the 16428-web-components branch from 9c461b3 to 6795251 Compare July 30, 2018 21:09

jbedard force-pushed the 16428-web-components branch from 6795251 to 84eb9f8 Compare July 30, 2018 21:13

jbedard force-pushed the 16428-web-components branch from 84eb9f8 to 7f7ab85 Compare July 30, 2018 21:16

jbedard force-pushed the 16428-web-components branch from 7f7ab85 to 06ba791 Compare July 31, 2018 18:09

jbedard force-pushed the 16428-web-components branch from 06ba791 to 129d5dc Compare July 31, 2018 22:38

Narretz approved these changes Aug 1, 2018

View reviewed changes

jbedard added 2 commits August 1, 2018 17:50

refactor($compile): move img[srcset] sanitizing to helper method

88a12f8

jbedard force-pushed the 16428-web-components branch from 129d5dc to dedb10c Compare August 2, 2018 00:51

jbedard merged commit dedb10c into angular:master Aug 2, 2018

dependencies bot mentioned this pull request Aug 9, 2018

Update 6 dependencies from npm JetBrains/ring-ui#349

Merged

feat($compile): add support for arbitrary property and event bindings #16614

feat($compile): add support for arbitrary property and event bindings #16614

Conversation

jbedard commented Jun 26, 2018 • edited

gkalpak left a 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

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

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

jbedard Jun 28, 2018 • edited

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

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

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

Choose a reason for hiding this comment

gkalpak left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jun 29, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jun 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Jun 29, 2018

jbedard commented Jun 26, 2018 •

edited

jbedard Jun 28, 2018 •

edited

gkalpak Jun 29, 2018 •

edited

gkalpak Jun 29, 2018 •

edited