fix(select): handle corner case of adding options via a custom directive #13878
Conversation
gkalpak
force-pushed
the
fix-select-chromeHack-on-comment
branch
from
39f21f3
to
ba177c1
Compare
Under specific circumstances (e.g. adding options via a directive with `replace: true` and a structural directive in its template), an error occurred when trying to call `hasAttribute()` on a comment node (which doesn't support that method). This commit fixes it by first checking if `hasAttribute()` is available. Fixes angular#13874
gkalpak
force-pushed
the
fix-select-chromeHack-on-comment
branch
from
ba177c1
to
f0e1eb0
Compare
| scope: {myOptions: '='}, | ||
| replace: true, | ||
| template: | ||
| '<option value="{{ ::option.value }}" ng-repeat="option in ::myOptions">' + |
There was a problem hiding this comment.
Is there a reasonw why this uses one-time binding?
There was a problem hiding this comment.
No particular reason - force of habbit :)
|
@Narretz, I made the suggested changes. PTAL |
| @@ -80,6 +80,9 @@ var SelectController = | |||
|
|
|||
| // Tell the select control that an option, with the given value, has been added | |||
| self.addOption = function(value, element) { | |||
| // Skip comment nodes, as they only pollute the `optionsMap` | |||
| if (element.prop('nodeType') === NODE_TYPE_COMMENT) return; | |||
There was a problem hiding this comment.
That was my first "reaction", but then I thought: "What if element[0] is undefined ?"
So, instead of element[0] && element[0].nodeType, I went for element.prop(...) (I'm not even sure if the element passed to the linking function can be empty...)
There was a problem hiding this comment.
AddOption is a private Api so we guarantee that element will always be jq
wrapped. I'm a bit worried about the overhead of calling prop on every
option.
Am 29.01.2016 13:28 schrieb "Georgios Kalpakas" notifications@github.com:
In src/ng/directive/select.js
#13878 (comment):@@ -80,6 +80,9 @@ var SelectController =
// Tell the select control that an option, with the given value, has been added
self.addOption = function(value, element) {
- // Skip comment nodes, as they only pollute the
optionsMap- if (element.prop('nodeType') === NODE_TYPE_COMMENT) return;
That was my first "reaction", but then I thought: "What if element[0] is
undefined ?"
So, instead of element[0] && element[0].nodeType, I went for
element.prop(...) (I'm not even sure if the element passed to the linking
function can be empty...)—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/13878/files#r51255240.
|
small suggestion, otherwise LGTM |
Under specific circumstances (e.g. adding options via a directive with `replace: true` and a structural directive in its template), an error occurred when trying to call `hasAttribute()` on a comment node (which doesn't support that method). This commit fixes it by filtering out comment nodes in the `addOption()` method. Fixes #13874 Closes #13878
Under specific circumstances (e.g. adding options via a directive with
replace: trueand a structural directive in its template), an error occurred when trying to callhasAttribute()on a comment node (which doesn't support that method).This commit fixes it by first checking if
hasAttribute()is available.Fixes #13874