-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Conversation
src/ng/compile.js
Outdated
* @element ANY | ||
* | ||
* @description | ||
* The `ngOn` directive adds an event listener to a DOM element, and evaluates an expression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should mention that it uses jqLite.on
and not the native addEventListener
. I'm not sure how we refer to the internal jqLite
var in docs though. angular.element
?
src/ng/compile.js
Outdated
* malicious code. | ||
* For this reason, `ngProp` requires trusted expression content via | ||
* Strict Contextual Escaping with the {@link ng.$sce $sce service}. Practically this means | ||
* that different properties require different trusted content types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this reason,
ngProp
makes use of the Strict Contextual Escaping mechanisms in AngularJS
to ensure that, for security sensitive properties, the developer either marks the value as "trusted"
or applies an appropriate sanitization strategy to the value before it is assigned to the property.
src/ng/compile.js
Outdated
* | ||
* ### Sanitization | ||
* | ||
* By default, `$sce` will throw an error if it detects potentially dangerous content. However, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is accurate. It will, by default, throw an error if you try to write a non-trusted value to a security sensitive property, such as innerHTML
.
src/ng/compile.js
Outdated
* malicious code. | ||
* For this reason, `ngProp` applies Strict Contextual Escaping with the {@link ng.$sce $sce service}. | ||
* This means vulnerable properties require their content to be "trusted", based on the | ||
* context of the property. For example, the `iframe[src]` property is in the `RESOURCE_URL` context and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this directive is about properties and not attributes, we should probably not put iframe[src]
, instead I think we should put iframe.src
.
src/ng/compile.js
Outdated
* For this reason, `ngProp` applies Strict Contextual Escaping with the {@link ng.$sce $sce service}. | ||
* This means vulnerable properties require their content to be "trusted", based on the | ||
* context of the property. For example, the `iframe[src]` property is in the `RESOURCE_URL` context and | ||
* `innerHTML` is in the `HTML` context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, which requires that values written to this property are trusted as a
RESOURCE_URL
. This can be
set explicitly by calling$sce.trustAsResourceUrl(value)
on the value that is trusted before passing
it to theng-prop-src
directive. In some cases you can rely upon automatic sanitization of untrusted
values - see below.
src/ng/compile.js
Outdated
* | ||
* By default, `$sce` will throw an error if it detects untrusted content, and will not bind the | ||
* content. | ||
* However, if you include the {@link ngSanitize ngSanitize module}, it will try to sanitize the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ngSanitize is only related to HTML values. The URL sanitizers are built into the core ng
module.
src/ng/compile.js
Outdated
* However, if you include the {@link ngSanitize ngSanitize module}, it will try to sanitize the | ||
* potentially dangerous content, e.g. strip non-whitelisted tags and attributes when binding to | ||
* `innerHTML`. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The autogenerated "usage" block for this directive is not helpful or accurate.
If you add a @usa ge
tag then this will be used instead, which allows you to provide an appropriate usage for this "special" directive.
src/ng/compile.js
Outdated
* | ||
* Since HTML attributes are case-insensitive, camelCase properties like `innerHTML` must be escaped. | ||
* AngularJS uses the underscore (_) in front of a character to indicate that it is uppercase, so | ||
* `innerHTML` must be written as `ng-prop-inner_h_t_m_l="expression"`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth noting here that usually you would use ngBindHtml
for this.
* | ||
* <div class="prop-unit"> | ||
* ... so that actually dangerous content cannot be executed: | ||
* <div class="prop-binding" ng-prop-inner_h_t_m_l="$ctrl.unsafeContent"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this one cause an error and break the example app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it logs an error to the console, but doesn't stop execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it kills the current digest?
src/ng/compile.js
Outdated
* often use | ||
* [custom events](https://developer.mozilla.org/docs/Web/Guide/Events/Creating_and_triggering_events) | ||
* that are fired like normal DOM events. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth mentioning camelCasing here as well for custom events that have uppercase characters in them.
src/ng/compile.js
Outdated
* The `ngOn` directive adds an event listener to a DOM element, and evaluates an expression | ||
* when the event is fired. | ||
* `ngOn` allows adding listeners for arbitrary events by including | ||
* the event name in the attribute, e.g. `ng-on-drop="onDrop()"` executes the 'onDrop' expression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strictly the expression is onDrop()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of random suggestions...
Do we want to make more of the potential for use with Custom Elements?
Also do we want to draw a comparison with Angular's [prop]
and (event)
bindings?
The docs are written as if ngProp and ngOn were regular directives, which makes them easier to find.
127300f
to
65eb480
Compare
The docs are written as if ngProp and ngOn were regular directives, which makes them easier to find. Closes #16627
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: