From 161ecb0d7ed7971bcb3ff84e46e8c25698cab6bf Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 20 Jul 2017 21:50:22 +0300 Subject: [PATCH 1/8] refactor(uiSelectCtrl): extract ctrl.activeIndex reset logic --- src/uiSelectController.js | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index d6023d0b9..d2ce9df74 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -92,30 +92,35 @@ uis.controller('uiSelectCtrl', function _resetSearchInput() { if (ctrl.resetSearchInput) { ctrl.search = EMPTY_SEARCH; - //reset activeIndex - if (!ctrl.multiple) { - if (ctrl.selected && ctrl.items.length) { - ctrl.activeIndex = _findIndex(ctrl.items, function(item){ - return angular.equals(this, item); - }, ctrl.selected); - } else { - ctrl.activeIndex = 0; - } + + _resetActiveIndex(); + } + } + + function _resetActiveIndex() { + + if (!ctrl.multiple) { + if (ctrl.selected && ctrl.items.length) { + ctrl.activeIndex = _findIndex(ctrl.items, function(item){ + return angular.equals(this, item); + }, ctrl.selected); + } else { + ctrl.activeIndex = 0; } } } - function _groupsFilter(groups, groupNames) { - var i, j, result = []; - for(i = 0; i < groupNames.length ;i++){ - for(j = 0; j < groups.length ;j++){ - if(groups[j].name == [groupNames[i]]){ - result.push(groups[j]); - } + function _groupsFilter(groups, groupNames) { + var i, j, result = []; + for(i = 0; i < groupNames.length ;i++){ + for(j = 0; j < groups.length ;j++){ + if(groups[j].name == [groupNames[i]]){ + result.push(groups[j]); } } - return result; } + return result; + } // When the user clicks on ui-select, displays the dropdown list ctrl.activate = function(initSearchValue, avoidReset) { From 534085d810acd97e77ddfce466c5f39960eebf6a Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 20 Jul 2017 22:11:55 +0300 Subject: [PATCH 2/8] refactor(uiSelectCtrl): extract ctrl.activate extract dropdown open logic to private function Extracted: - method: _displayDropdown(initSearchValue, avoidReset) -> handle opening - method: _canAnimate(element) - method: _animateDropdown(searchInput, initSearchValue, container) --- src/uiSelectController.js | 101 ++++++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 42 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index d2ce9df74..c945f42b5 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -125,49 +125,8 @@ uis.controller('uiSelectCtrl', // When the user clicks on ui-select, displays the dropdown list ctrl.activate = function(initSearchValue, avoidReset) { if (!ctrl.disabled && !ctrl.open) { - if(!avoidReset) _resetSearchInput(); - - $scope.$broadcast('uis:activate'); - ctrl.open = true; - ctrl.activeIndex = ctrl.activeIndex >= ctrl.items.length ? 0 : ctrl.activeIndex; - // ensure that the index is set to zero for tagging variants - // that where first option is auto-selected - if ( ctrl.activeIndex === -1 && ctrl.taggingLabel !== false ) { - ctrl.activeIndex = 0; - } - var container = $element.querySelectorAll('.ui-select-choices-content'); - var searchInput = $element.querySelectorAll('.ui-select-search'); - if (ctrl.$animate && ctrl.$animate.on && ctrl.$animate.enabled(container[0])) { - var animateHandler = function(elem, phase) { - if (phase === 'start' && ctrl.items.length === 0) { - // Only focus input after the animation has finished - ctrl.$animate.off('removeClass', searchInput[0], animateHandler); - $timeout(function () { - ctrl.focusSearchInput(initSearchValue); - }); - } else if (phase === 'close') { - // Only focus input after the animation has finished - ctrl.$animate.off('enter', container[0], animateHandler); - $timeout(function () { - ctrl.focusSearchInput(initSearchValue); - }); - } - }; - - if (ctrl.items.length > 0) { - ctrl.$animate.on('enter', container[0], animateHandler); - } else { - ctrl.$animate.on('removeClass', searchInput[0], animateHandler); - } - } else { - $timeout(function () { - ctrl.focusSearchInput(initSearchValue); - if(!ctrl.tagging.isActivated && ctrl.items.length > 1 && ctrl.open) { - _ensureHighlightVisible(); - } - }); - } + _displayDropdown(initSearchValue, avoidReset); } else if (ctrl.open && !ctrl.searchEnabled) { // Close the selection if we don't have search enabled, and we click on the select again @@ -175,6 +134,64 @@ uis.controller('uiSelectCtrl', } }; + function _displayDropdown(initSearchValue, avoidReset) { + if(!avoidReset) _resetSearchInput(); + + $scope.$broadcast('uis:activate'); + ctrl.open = true; + ctrl.activeIndex = ctrl.activeIndex >= ctrl.items.length ? 0 : ctrl.activeIndex; + // ensure that the index is set to zero for tagging variants + // that where first option is auto-selected + if ( ctrl.activeIndex === -1 && ctrl.taggingLabel !== false ) { + ctrl.activeIndex = 0; + } + + var container = $element.querySelectorAll('.ui-select-choices-content'); + var searchInput = $element.querySelectorAll('.ui-select-search'); + + if (_canAnimate(container)) { + _animateDropdown(searchInput, initSearchValue, container); + } else { + $timeout(function () { + ctrl.focusSearchInput(initSearchValue); + if(!ctrl.tagging.isActivated && ctrl.items.length > 1 && ctrl.open) { + _ensureHighlightVisible(); + } + }); + } + } + + function _canAnimate(element) { + + return ctrl.$animate && ctrl.$animate.on && ctrl.$animate.enabled(element[0]); + } + + function _animateDropdown(searchInput, initSearchValue, container) { + var animateHandler = function (elem, phase) { + if (phase === 'start' && ctrl.items.length === 0) { + // Only focus input after the animation has finished + ctrl.$animate.off('removeClass', searchInput[0], animateHandler); + $timeout(function () { + ctrl.focusSearchInput(initSearchValue); + }); + } + else if (phase === 'close') { + // Only focus input after the animation has finished + ctrl.$animate.off('enter', container[0], animateHandler); + $timeout(function () { + ctrl.focusSearchInput(initSearchValue); + }); + } + }; + + if (ctrl.items.length > 0) { + ctrl.$animate.on('enter', container[0], animateHandler); + } + else { + ctrl.$animate.on('removeClass', searchInput[0], animateHandler); + } + } + ctrl.focusSearchInput = function (initSearchValue) { ctrl.search = initSearchValue || ctrl.search; ctrl.searchInput[0].focus(); From 00aaa0f128c1400defa55a89a3ca412c03499125 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 20 Jul 2017 22:32:49 +0300 Subject: [PATCH 3/8] fix(uiSelectCtrl): scroll to active position on dropdown open When ui-select is configured with `resetSearchInput: false` `ctrl.activeIndex` is not calculated when `ctrl.activate()` is invoked Opening an ui-select control with preselected model doesn't trigger active index update/reset and so the container is scroll the the element with 0 `ctrl.activeIndex` initial value (check `_ensureHighlightVisible()`) --- src/uiSelectController.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index c945f42b5..27b53cdcc 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -92,9 +92,9 @@ uis.controller('uiSelectCtrl', function _resetSearchInput() { if (ctrl.resetSearchInput) { ctrl.search = EMPTY_SEARCH; - - _resetActiveIndex(); } + + _resetActiveIndex(); } function _resetActiveIndex() { @@ -134,11 +134,17 @@ uis.controller('uiSelectCtrl', } }; - function _displayDropdown(initSearchValue, avoidReset) { - if(!avoidReset) _resetSearchInput(); + function _displayDropdown(initSearchValue, avoidSearchReset) { + if(avoidSearchReset) { + _resetActiveIndex(); + } + else { + _resetSearchInput(); + } $scope.$broadcast('uis:activate'); ctrl.open = true; + ctrl.activeIndex = ctrl.activeIndex >= ctrl.items.length ? 0 : ctrl.activeIndex; // ensure that the index is set to zero for tagging variants // that where first option is auto-selected From 3c9a3c6357d0f709574d29ced5dac320e9a6c8a0 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 20 Jul 2017 22:39:02 +0300 Subject: [PATCH 4/8] refactor(uiSelectCtrl): extract focus handling on dropdown open Extracted repeating logic that focuses the search input after the dropdown is clicked Performed with $timeout so any digest or animation logic is waited for --- src/uiSelectController.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index 27b53cdcc..7bc7d0cec 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -158,12 +158,7 @@ uis.controller('uiSelectCtrl', if (_canAnimate(container)) { _animateDropdown(searchInput, initSearchValue, container); } else { - $timeout(function () { - ctrl.focusSearchInput(initSearchValue); - if(!ctrl.tagging.isActivated && ctrl.items.length > 1 && ctrl.open) { - _ensureHighlightVisible(); - } - }); + _focusWhenReady(initSearchValue); } } @@ -198,6 +193,15 @@ uis.controller('uiSelectCtrl', } } + function _focusWhenReady(initSearchValue) { + $timeout(function () { + ctrl.focusSearchInput(initSearchValue); + if(!ctrl.tagging.isActivated && ctrl.items.length > 1 && ctrl.open) { + _ensureHighlightVisible(); + } + }); + } + ctrl.focusSearchInput = function (initSearchValue) { ctrl.search = initSearchValue || ctrl.search; ctrl.searchInput[0].focus(); From 27fb45bb8c1b095ad18854a694f5d3d251659531 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 20 Jul 2017 22:52:14 +0300 Subject: [PATCH 5/8] fix(uiSelectCtrl): scroll to active position on dropdown open in conjunction with $animate The function `_ensureHighlightVisible()` that scrolls to the active (highlighted) element is never invoked at the end of the animation, when animation is used The logic that handles non animation opening is a bit different in that it scrolls to the highlighted element Updated the animation handler to perform similarly --- src/uiSelectController.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index 7bc7d0cec..08508cc28 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -172,16 +172,12 @@ uis.controller('uiSelectCtrl', if (phase === 'start' && ctrl.items.length === 0) { // Only focus input after the animation has finished ctrl.$animate.off('removeClass', searchInput[0], animateHandler); - $timeout(function () { - ctrl.focusSearchInput(initSearchValue); - }); + _focusWhenReady(initSearchValue); } else if (phase === 'close') { // Only focus input after the animation has finished ctrl.$animate.off('enter', container[0], animateHandler); - $timeout(function () { - ctrl.focusSearchInput(initSearchValue); - }); + _focusWhenReady(initSearchValue); } }; From e5e93f68105553180fc84468df25bcc5c3ad706f Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 20 Jul 2017 23:55:28 +0300 Subject: [PATCH 6/8] test(uiSelectCtrl): test active element is scrolled into view on open --- test/select.spec.js | 102 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/test/select.spec.js b/test/select.spec.js index 7add0ade5..97f09e57d 100644 --- a/test/select.spec.js +++ b/test/select.spec.js @@ -3518,6 +3518,7 @@ describe('ui-select tests', function () { expect(scope.fetchFromServer.calls.any()).toEqual(true); }); }); + describe('Test key down key up and activeIndex should skip disabled choice', function () { it('should ignore disabled items, going down', function () { var el = createUiSelect({ uiDisableChoice: "person.age == 12" }); @@ -3629,4 +3630,105 @@ describe('ui-select tests', function () { }); }); }); + + describe('Test scrolling to highlighted item after opening', function () { + + beforeEach(function () { + + scope.people = scope.people.concat([ + { name: 'Elvis', email: 'elvis@email.com', group: 'Foo', age: 23 }, + { name: 'Ace', email: 'ace@email.com', group: 'Foo', age: 30 }, + { name: 'Mitchell', email: 'mitchell@email.com', group: 'Foo', age: 41 } + ]); + }); + + it('Should set ctrl.active index to the selected person index', function () { + + var lastIndex = scope.people.length - 1; + scope.selection.selected = scope.people[lastIndex]; + + var el = createUiSelect(); + clickMatch(el); + + expect(el.scope().$select.activeIndex).toEqual(lastIndex); + }); + + it('Should set ctrl.active index to the selected with `resetSearchInput = false`', function () { + + var lastIndex = scope.people.length - 1; + scope.selection.selected = scope.people[lastIndex]; + + var el = createUiSelect({ resetSearchInput: false}); + clickMatch(el); + + expect(el.scope().$select.activeIndex).toEqual(lastIndex); + }); + + it('Should scroll the last item into view with animation enabled', inject(function ($animate) { + + // This test should be updated with proper animation triggering and testing + // tried with `ngAnimateMock` but NO animation is triggered on digest or flush + + scope.selection.selected = scope.people.slice().pop(); + var el = createUiSelect(); + var choicesContentEl = $(el).find('.ui-select-choices-content').get(0); + + spyOn($animate, 'enabled').and.returnValue(true); + spyOn($animate, 'on'); + + clickMatch(el); + + var animationHandler = $animate.on.calls.mostRecent().args[2]; + animationHandler(choicesContentEl, 'close'); + $timeout.flush(); + + var optionEl = $(el).find('.ui-select-choices-row div:contains("Mitchell")').get(0); + + expect(choicesContentEl.scrollTop).toBeGreaterThan(0); + expect(isScrolledIntoContainer(choicesContentEl, optionEl)).toEqual(true); + })); + + it('Should scroll the last item into view with animation disabled', inject(function ($animate) { + + spyOn($animate, 'enabled').and.returnValue(false); + + scope.selection.selected = scope.people.slice().pop(); + + var el = createUiSelect(); + clickMatch(el); + $timeout.flush(); + + var choicesContentEl = $(el).find('.ui-select-choices-content').get(0); + var optionEl = $(el).find('.ui-select-choices-row div:contains("Mitchell")').get(0); + + expect(choicesContentEl.scrollTop).toBeGreaterThan(0); + expect(isScrolledIntoContainer(choicesContentEl, optionEl)).toEqual(true); + })); + + it('Should scroll the last item into view with `resetSearchInput = false`', function () { + + scope.selection.selected = scope.people.slice().pop(); + + var el = createUiSelect({ resetSearchInput: false}); + clickMatch(el); + $timeout.flush(); + + var choicesContentEl = $(el).find('.ui-select-choices-content').get(0); + var optionEl = $(el).find('.ui-select-choices-row div:contains("Mitchell")').get(0); + + expect(choicesContentEl.scrollTop).toBeGreaterThan(0); + expect(isScrolledIntoContainer(choicesContentEl, optionEl)).toEqual(true); + }); + + function isScrolledIntoContainer(container, item) + { + var scrollTop = container.scrollTop; + var scrollBottom = scrollTop + container.clientHeight; + + var itemTop = item.offsetTop; + var itemBottom = itemTop + item.clientHeight; + + return (scrollTop <= itemTop) && (itemBottom <= scrollBottom); + } + }); }); From a5eb57c95acd92d4c295a6894adc67583e937d39 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 21 Jul 2017 13:29:38 +0300 Subject: [PATCH 7/8] chore(karma.conf): include `dist/select.css` This sets proper element size during testing And doesn't break the rest of the test Tests for scroll position depend on these styles --- karma.conf.js | 1 + 1 file changed, 1 insertion(+) diff --git a/karma.conf.js b/karma.conf.js index 89c684ca3..78920b538 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -15,6 +15,7 @@ module.exports = function(config) { 'node_modules/angular-mocks/angular-mocks.js', 'dist/select.js', + 'dist/select.css', 'test/helpers.js', 'test/**/*.spec.js' ], From e6ba734e36e584aa7b5574a5aeabe72db835e641 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 21 Jul 2017 14:14:15 +0300 Subject: [PATCH 8/8] refactor(uiSelectCtrl): update _animateDropdown() to be async and return promise The promise is resolved when the animation is switched off (unsubscribed) This extracts the additional responsibility to call focus/scroll the active element at the end of the animation --- src/uiSelectController.js | 52 ++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/uiSelectController.js b/src/uiSelectController.js index 08508cc28..8831be235 100644 --- a/src/uiSelectController.js +++ b/src/uiSelectController.js @@ -2,11 +2,12 @@ * Contains ui-select "intelligence". * * The goal is to limit dependency on the DOM whenever possible and - * put as much logic in the controller (instead of the link functions) as possible so it can be easily tested. + * put as much logic in the controller (instead of the link functions) as possible so it can be + * easily tested. */ uis.controller('uiSelectCtrl', - ['$scope', '$element', '$timeout', '$filter', '$$uisDebounce', 'uisRepeatParser', 'uiSelectMinErr', 'uiSelectConfig', '$parse', '$injector', '$window', - function($scope, $element, $timeout, $filter, $$uisDebounce, RepeatParser, uiSelectMinErr, uiSelectConfig, $parse, $injector, $window) { + ['$scope', '$element', '$timeout', '$filter', '$$uisDebounce', 'uisRepeatParser', 'uiSelectMinErr', 'uiSelectConfig', '$parse', '$injector', '$window', '$q', + function($scope, $element, $timeout, $filter, $$uisDebounce, RepeatParser, uiSelectMinErr, uiSelectConfig, $parse, $injector, $window, $q) { var ctrl = this; @@ -156,7 +157,9 @@ uis.controller('uiSelectCtrl', var searchInput = $element.querySelectorAll('.ui-select-search'); if (_canAnimate(container)) { - _animateDropdown(searchInput, initSearchValue, container); + // Only focus input after the animation has finished + _animateDropdown(searchInput, container) + .then(_focusWhenReady.bind(null, initSearchValue)); } else { _focusWhenReady(initSearchValue); } @@ -167,26 +170,28 @@ uis.controller('uiSelectCtrl', return ctrl.$animate && ctrl.$animate.on && ctrl.$animate.enabled(element[0]); } - function _animateDropdown(searchInput, initSearchValue, container) { - var animateHandler = function (elem, phase) { - if (phase === 'start' && ctrl.items.length === 0) { - // Only focus input after the animation has finished - ctrl.$animate.off('removeClass', searchInput[0], animateHandler); - _focusWhenReady(initSearchValue); + function _animateDropdown(searchInput, container) { + + return $q(function (resolve, reject) { + + var animateHandler = function (elem, phase) { + if (phase === 'start' && ctrl.items.length === 0) { + ctrl.$animate.off('removeClass', searchInput[0], animateHandler); + resolve(); + } + else if (phase === 'close') { + ctrl.$animate.off('enter', container[0], animateHandler); + resolve(); + } + }; + + if (ctrl.items.length > 0) { + ctrl.$animate.on('enter', container[0], animateHandler); } - else if (phase === 'close') { - // Only focus input after the animation has finished - ctrl.$animate.off('enter', container[0], animateHandler); - _focusWhenReady(initSearchValue); + else { + ctrl.$animate.on('removeClass', searchInput[0], animateHandler); } - }; - - if (ctrl.items.length > 0) { - ctrl.$animate.on('enter', container[0], animateHandler); - } - else { - ctrl.$animate.on('removeClass', searchInput[0], animateHandler); - } + }); } function _focusWhenReady(initSearchValue) { @@ -326,7 +331,8 @@ uis.controller('uiSelectCtrl', /** * Typeahead mode: lets the user refresh the collection using his own function. * - * See Expose $select.search for external / remote filtering https://github.com/angular-ui/ui-select/pull/31 + * See Expose $select.search for external / remote filtering + * https://github.com/angular-ui/ui-select/pull/31 */ ctrl.refresh = function(refreshAttr) { if (refreshAttr !== undefined) {