Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($compile): fix ng-prop-* with undefined values #16798

Merged
merged 1 commit into from Jan 11, 2019

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Dec 22, 2018

Fixes #16797

src/ng/compile.js Outdated Show resolved Hide resolved
test/ng/ngPropSpec.js Outdated Show resolved Hide resolved
var element = $compile('<span ng-prop-text="myText" />')($rootScope);
// Initialze to non-falsey
$rootScope.myText = 'abc';
$rootScope.$digest();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I like to say, you can make these shorter with: $rootScope.$apply('text = ...') 😁

src/ng/compile.js Outdated Show resolved Hide resolved
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Might be worth adding a test to ensure that values do not go through .prop() or that special property names (such as class) are not handled specially.

@jbedard
Copy link
Contributor Author

jbedard commented Dec 30, 2018

I've added 2 tests: 8b9cb92

@@ -3837,7 +3837,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
pre: function ngPropPreLinkFn(scope, $element) {
function applyPropValue() {
var propValue = ngPropGetter(scope);
$element.prop(propName, sanitizer(propValue));
$element[0][propName] = sanitizer(propValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$element can only ever have 1 element at this point, right? 😟

@jbedard jbedard merged commit 8b973e0 into angular:master Jan 11, 2019
jbedard added a commit that referenced this pull request Jan 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants