From 11d2b9342e54258a67840ecd573d9212ff347c0a Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Wed, 30 Nov 2016 20:30:54 +0100 Subject: [PATCH 1/3] fix(ngOptions): ignore comment nodes when removing 'selected' attribute Closes #15454 --- src/ng/directive/ngOptions.js | 2 +- test/ng/directive/ngOptionsSpec.js | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index a8adfb0b2e1b..bc5d5a80fca6 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -452,7 +452,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, var removeEmptyOption = function() { if (!providedEmptyOption) { emptyOption.remove(); - } else { + } else if (emptyOption[0].nodeType === NODE_TYPE_ELEMENT) { emptyOption.removeAttr('selected'); } }; diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index edfc0f98d108..bdcef1ef68d6 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -2557,6 +2557,22 @@ describe('ngOptions', function() { expect(linkLog).toEqual(['linkCompileContents', 'linkNgOptions']); }); + it('should select the correct option after linking when the ngIf expression is initially falsy', function() { + scope.values = [ + {name:'black'}, + {name:'white'}, + {name:'red'} + ]; + scope.selected = scope.values[2]; + + expect(function() { + createSingleSelect(''); + scope.$apply(); + }).not.toThrow(); + + expect(element.find('option')[2]).toBeMarkedAsSelected(); + expect(linkLog).toEqual(['linkNgOptions']); + }); it('should not throw with a directive that replaces', inject(function($templateCache, $httpBackend) { $templateCache.put('select_template.html', ''); From 860cb61b945f2ffcb013a0c241f21a4f4529587f Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Thu, 1 Dec 2016 23:24:19 +0100 Subject: [PATCH 2/3] actual fix --- src/ng/directive/ngOptions.js | 41 ++++++++++++++--- test/ng/directive/ngOptionsSpec.js | 72 ++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index bc5d5a80fca6..bc437795dc24 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -429,6 +429,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, } var providedEmptyOption = !!emptyOption; + var emptyOptionRendered = false; var unknownOption = jqLite(optionTemplate.cloneNode(false)); unknownOption.val('?'); @@ -445,14 +446,16 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, selectElement.prepend(emptyOption); } selectElement.val(''); - emptyOption.prop('selected', true); // needed for IE - emptyOption.attr('selected', true); + if (emptyOptionRendered) { + emptyOption.prop('selected', true); // needed for IE + emptyOption.attr('selected', true); + } }; var removeEmptyOption = function() { if (!providedEmptyOption) { emptyOption.remove(); - } else if (emptyOption[0].nodeType === NODE_TYPE_ELEMENT) { + } else if (emptyOptionRendered) { emptyOption.removeAttr('selected'); } }; @@ -587,9 +590,35 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, // compile the element since there might be bindings in it $compile(emptyOption)(scope); - // remove the class, which is added automatically because we recompile the element and it - // becomes the compilation root - emptyOption.removeClass('ng-scope'); + if (emptyOption[0].nodeType === NODE_TYPE_COMMENT) { + // This means the empty option has currently no actual DOM node, probably because + // it has been modified by a transclusion directive. + + emptyOptionRendered = false; + + // Redefine the registerOption function, which will catch + // options that are added by ngIf etc. (rendering of the node is async because of + // lazy transclusion) + selectCtrl.registerOption = function(optionScope, optionEl) { + console.log('register'); + if (optionEl.val() === '') { + emptyOptionRendered = true; + emptyOption = optionEl; + emptyOption.removeClass('ng-scope'); + // This ensures the new empty option is selected if previously no option was selected + ngModelCtrl.$render(); + + optionEl.on('$destroy', function() { + emptyOption = undefined; + emptyOptionRendered = false; + }); + } + } + } else { + emptyOption.removeClass('ng-scope'); + emptyOptionRendered = true; + } + } else { emptyOption = jqLite(optionTemplate.cloneNode(false)); } diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index bdcef1ef68d6..60ad21b0b99e 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -2574,6 +2574,78 @@ describe('ngOptions', function() { expect(linkLog).toEqual(['linkNgOptions']); }); + + it('should add / remove the "selected" attribute on empty option which has an initially falsy ngIf expression', function() { + scope.values = [ + {name:'black'}, + {name:'white'}, + {name:'red'} + ]; + scope.selected = scope.values[2]; + + createSingleSelect(''); + scope.$apply(); + + expect(element.find('option')[2]).toBeMarkedAsSelected(); + + scope.$apply('isBlank = true'); + expect(element.find('option')[0].value).toBe(''); + expect(element.find('option')[0]).not.toBeMarkedAsSelected(); + + scope.$apply('selected = null'); + expect(element.find('option')[0].value).toBe(''); + expect(element.find('option')[0]).toBeMarkedAsSelected(); + + scope.selected = scope.values[1]; + scope.$apply(); + expect(element.find('option')[0].value).toBe(''); + expect(element.find('option')[0]).not.toBeMarkedAsSelected(); + expect(element.find('option')[2]).toBeMarkedAsSelected(); + }); + + + it('should add / remove the "selected" attribute on empty option which has an initially truthy ngIf expression when no option is selected', function() { + scope.values = [ + {name:'black'}, + {name:'white'}, + {name:'red'} + ]; + scope.isBlank = true; + + createSingleSelect(''); + scope.$apply(); + + expect(element.find('option')[0].value).toBe(''); + expect(element.find('option')[0]).toBeMarkedAsSelected(); + + scope.selected = scope.values[2]; + scope.$apply(); + expect(element.find('option')[0]).not.toBeMarkedAsSelected(); + expect(element.find('option')[3]).toBeMarkedAsSelected(); + }); + + + it('should add the "selected" attribute on empty option which has an initially falsy ngIf expression when no option is selected', function() { + scope.values = [ + {name:'black'}, + {name:'white'}, + {name:'red'} + ]; + + createSingleSelect(''); + scope.$apply(); + + expect(element.find('option')[0]).not.toBeMarkedAsSelected(); + + scope.isBlank = true; + scope.$apply(); + + expect(element.find('option')[0].value).toBe(''); + expect(element.find('option')[0]).toBeMarkedAsSelected(); + expect(element.find('option')[1]).not.toBeMarkedAsSelected(); + }); + + it('should not throw with a directive that replaces', inject(function($templateCache, $httpBackend) { $templateCache.put('select_template.html', ''); From dcf2b52c6d25c215ae4a72b22828cf635caf71de Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Fri, 2 Dec 2016 00:39:32 +0100 Subject: [PATCH 3/3] linter fixes --- src/ng/directive/ngOptions.js | 4 ++-- test/ng/directive/ngOptionsSpec.js | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index bc437795dc24..ad199406d694 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -600,7 +600,6 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, // options that are added by ngIf etc. (rendering of the node is async because of // lazy transclusion) selectCtrl.registerOption = function(optionScope, optionEl) { - console.log('register'); if (optionEl.val() === '') { emptyOptionRendered = true; emptyOption = optionEl; @@ -613,7 +612,8 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, emptyOptionRendered = false; }); } - } + }; + } else { emptyOption.removeClass('ng-scope'); emptyOptionRendered = true; diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index 60ad21b0b99e..f1f752bf352d 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -2557,6 +2557,7 @@ describe('ngOptions', function() { expect(linkLog).toEqual(['linkCompileContents', 'linkNgOptions']); }); + it('should select the correct option after linking when the ngIf expression is initially falsy', function() { scope.values = [ {name:'black'},