Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Typeahead popup position #3874

Closed
wants to merge 10 commits into from
24 changes: 24 additions & 0 deletions src/typeahead/test/typeahead.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,30 @@ describe('typeahead tests', function () {
changeInputValueTo(element, 'ba');
expect(findDropDown($document.find('body')).length).toEqual(0);
});

it('should have right position after scroll', function() {
var element = prepareInputEl('<div><input ng-model="result" typeahead="item for item in source | filter:$viewValue" typeahead-append-to-body="true"></div>');
var dropdown = findDropDown($document.find('body'));
var body = angular.element(document.body);

// Set body height to allow scrolling
body.css({height:'10000px'});

// Scroll top
window.scroll(0, 1000);

// Set input value to show dropdown
changeInputValueTo(element, 'ba');

// Init position of dropdown must be 1000px
expect(dropdown.css('top') ).toEqual('1000px');

// After scroll, must have new position
window.scroll(0, 500);
body.triggerHandler('scroll');
$timeout.flush();
expect(dropdown.css('top') ).toEqual('500px');
});
});

describe('focus first', function () {
Expand Down
56 changes: 52 additions & 4 deletions src/typeahead/typeahead.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
};
}])

.directive('typeahead', ['$compile', '$parse', '$q', '$timeout', '$document', '$rootScope', '$position', 'typeaheadParser',
function ($compile, $parse, $q, $timeout, $document, $rootScope, $position, typeaheadParser) {
.directive('typeahead', ['$compile', '$parse', '$q', '$timeout', '$document', '$window', '$rootScope', '$position', 'typeaheadParser',
function ($compile, $parse, $q, $timeout, $document, $window, $rootScope, $position, typeaheadParser) {

var HOT_KEYS = [9, 13, 27, 38, 40];
var eventDebounceTime = 200;

return {
require:'ngModel',
Expand Down Expand Up @@ -96,6 +97,7 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
matches: 'matches',
active: 'activeIdx',
select: 'select(activeIdx)',
'move-in-progress': 'moveInProgress',
query: 'query',
position: 'position'
});
Expand Down Expand Up @@ -153,8 +155,7 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
//position pop-up with matches - we need to re-calculate its position each time we are opening a window
//with matches as a pop-up might be absolute-positioned and position of an input might have changed on a page
//due to other elements being rendered
scope.position = appendToBody ? $position.offset(element) : $position.position(element);
scope.position.top = scope.position.top + element.prop('offsetHeight');
recalculatePosition();

element.attr('aria-expanded', true);
} else {
Expand All @@ -170,6 +171,52 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
});
};

// bind events only if appendToBody params exist - performance feature
if ( appendToBody ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces in between ( and )

angular.element( $window ).bind( 'resize', fireRecalculating );
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces just after ( and just before )

$document.find( 'body' ).bind( 'scroll', fireRecalculating );
Copy link
Contributor

Choose a reason for hiding this comment

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

Both these event listeners need to be throttled - the resize and scroll events both fire often.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, a debounce on each of the event listeners would be better, since we only care about the trailing trigger of the event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces just after ( and just before )

}

// Declare the timeout promise var outside the function scope so that stacked calls can be cancelled later
var timeoutEventPromise;

// Default progress type
scope.moveInProgress = false;

function fireRecalculating() {

if( !scope.moveInProgress ){
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to if (!scope.moveInProgress) {

scope.moveInProgress = true;
scope.$digest();
}

// Cancel previous timeout
if ( timeoutEventPromise ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces just after ( and just before )

$timeout.cancel(timeoutEventPromise);
}

// Debounced executing recalculate after events fired
timeoutEventPromise = $timeout(function () {

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

// if popup is visible
if ( scope.matches.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

recalculatePosition();
}

scope.moveInProgress = false;
scope.$digest();

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

}, eventDebounceTime);

}

// recalculate actual position and set new values to scope
// after digest loop is popup in right position
function recalculatePosition() {
scope.position = appendToBody ? $position.offset(element) : $position.position(element);
scope.position.top = scope.position.top + element.prop('offsetHeight');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a separate top calculation?

Also this should be scope.position.top += element.prop('offsetHeight')

Copy link
Author

Choose a reason for hiding this comment

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

This is function copied from original code and top, must be calculate with offsetHeight of input, because dropdown must be showed after bottom edge of input.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense to move the functionality into the $position service perhaps?

Copy link
Author

Choose a reason for hiding this comment

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

Do you think something like $position.bottomEdge(element)?

}

resetMatches();

//we need to propagate user's query so we can higlight matches
Expand Down Expand Up @@ -358,6 +405,7 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
query:'=',
active:'=',
position:'&',
moveInProgress:'=',
select:'&'
},
replace:true,
Expand Down
2 changes: 1 addition & 1 deletion template/typeahead/typeahead-popup.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<ul class="dropdown-menu" ng-show="isOpen()" ng-style="{top: position().top+'px', left: position().left+'px'}" style="display: block;" role="listbox" aria-hidden="{{!isOpen()}}">
<ul class="dropdown-menu" ng-show="isOpen() && !moveInProgress" ng-style="{top: position().top+'px', left: position().left+'px'}" style="display: block;" role="listbox" aria-hidden="{{!isOpen()}}">
<li ng-repeat="match in matches track by $index" ng-class="{active: isActive($index) }" ng-mouseenter="selectActive($index)" ng-click="selectMatch($index)" role="option" id="{{::match.id}}">
<div typeahead-match index="$index" match="match" query="query" template-url="templateUrl"></div>
</li>
Expand Down