Skip to content

Issue #19: added support for scrollbar on dropdown menu #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Issue #19: added support for scrollbar on dropdown menu #30

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a07c6c5
Added support for scrollbar on dropdown menu
tgirardi Dec 3, 2012
edcb98d
Solved bad verification in blur method
tgirardi Dec 4, 2012
afa1bdd
Solution to BUG when clicking on highlighted area inside item
tgirardi Dec 6, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 63 additions & 20 deletions js/bootstrap-typeahead.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// ----------------------------------------------------------------------------
// ----------------------------------------------------------------------------
//
// bootstrap-typeahead.js
//
Expand Down Expand Up @@ -208,7 +208,7 @@ function ($) {
lookup: function (event) {
var that = this,
items;

if (that.ajax) {
that.ajaxer();
}
Expand Down Expand Up @@ -302,7 +302,7 @@ function ($) {
left: pos.left
});

this.$menu.show();
this.$menu.show();
this.shown = true;

return this;
Expand Down Expand Up @@ -341,8 +341,8 @@ function ($) {
i.find('a').html(that.highlighter(item[that.options.display], item));
return i[0];
});

items.first().addClass('active');
items.first().addClass('active');
this.$menu.html(items);
return this;
},
Expand All @@ -363,29 +363,57 @@ function ($) {
// Selects the next result
//
next: function (event) {
var active = this.$menu.find('.active').removeClass('active');
var that = this;
var $menu = that.$menu;
Copy link
Author

Choose a reason for hiding this comment

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

We are going to use this.$menu very often, so I believe using a variable gives shorter file, specially after "minification"

var active = $menu.find('.active');
Copy link
Author

Choose a reason for hiding this comment

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

I just noticed that this change (removing the removeClass invocation and doing it later) is not related with the issue being fixed. This has to do with keeping the last element as the active element when next() is called while at that situation (active == last).

After line 373 of this new version there should be a:

if(active.length) {
    return;
}

So we don't do anything if we are at the last element and calling next().

I should have removed this change as I did with the mentioned code... :-(

However, I'll take the chance to ask if something similar is done in any commit in the 2.0 branch or if there's an issue related with this (I believe that in the previous version of this file, pressing down arrow while in the last element of the list would leave the list with no item selected).

var next = active.next();

var menuHeight = $menu.innerHeight();
var bottomPosition;

if (!next.length) {
next = $(this.$menu.find('li')[0]);
}


active.removeClass('active');
next.addClass('active');
bottomPosition = next.position().top + next.height();
if(menuHeight < bottomPosition){
Copy link
Author

Choose a reason for hiding this comment

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

If the menu has no scrollbar this will always be false... I feel there would be no considerable advantages in checking previously if there is a scrollbar on the menu or not, compared with the disadvantage of a larger code if we do this kind of checking... but I'm not 100% sure. Any thoughts?

that.avoidMouseEnter = true;
Copy link
Author

Choose a reason for hiding this comment

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

Ment to avoid any mouseenter event, that could be fired as a consequence of the scrollTop invocation, from changing the active element

$menu.on('mousemove',function(){
that.avoidMouseEnter = false;
$menu.off('mousemove');
});
Copy link
Author

Choose a reason for hiding this comment

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

If the user moves the mouse after the execution of this method, we must cancel the restriction imposed in line 381

$menu.scrollTop($menu.scrollTop() + bottomPosition - menuHeight);
Copy link
Author

Choose a reason for hiding this comment

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

Here we do the scroll in order to keep the newly active item as fully visible. The bottom border of that element will be the lower limit of the menu's visual area.

}
},

//------------------------------------------------------------------
//
// Selects the previous result
//
prev: function (event) {
var active = this.$menu.find('.active').removeClass('active');
var that = this;
Copy link
Author

Choose a reason for hiding this comment

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

Very similar to what has been described in the comments for next() method

var $menu = that.$menu;
var active = $menu.find('.active');
var prev = active.prev();
var menuScroll = $menu.scrollTop();
var topPosition;

if (!prev.length) {
prev = this.$menu.find('li').last();
}

active.removeClass('active');
prev.addClass('active');
topPosition = menuScroll + prev.position().top;
if(menuScroll > topPosition){
that.avoidMouseEnter = true;
$menu.on('mousemove',function(){
that.avoidMouseEnter = false;
$menu.off('mousemove');
});
$menu.scrollTop(topPosition);
}
},

//=============================================================================================================
Expand All @@ -402,13 +430,13 @@ function ($) {
this.$element.on('blur', $.proxy(this.blur, this))
.on('keyup', $.proxy(this.keyup, this));

if (this.eventSupported('keydown')) {
this.$element.on('keydown', $.proxy(this.keypress, this));
} else {
this.$element.on('keypress', $.proxy(this.keypress, this));
}

this.$menu.on('click', $.proxy(this.click, this))
if (this.eventSupported('keydown')) {
this.$element.on('keydown', $.proxy(this.keypress, this));
} else {
this.$element.on('keypress', $.proxy(this.keypress, this));
}
this.$menu.on('mousedown', $.proxy(this.click, this))
.on('mouseenter', 'li', $.proxy(this.mouseenter, this));
},
Copy link
Author

Choose a reason for hiding this comment

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

click is not triggered when clicking the scrollbar. We'll need to use mousedown


Expand Down Expand Up @@ -484,6 +512,10 @@ function ($) {
var that = this;
e.stopPropagation();
e.preventDefault();
if(this.keepFocus) {
that.$element.focus();
this.keepFocus = false;
}
Copy link
Author

Choose a reason for hiding this comment

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

keepFocus is set to true by click() method when someones clicks inside the menu, but not inside any menu item (so we need to keep the menu opened)

setTimeout(function () {
if (!that.$menu.is(':focus')) {
that.hide();
Expand All @@ -496,16 +528,27 @@ function ($) {
// Handles clicking on the results list
//
click: function (e) {
e.stopPropagation();
e.preventDefault();
this.select();
if(e.which != 1) {
return;
}
if(e.target.tagName == 'A'){
e.stopPropagation();
e.preventDefault();
this.select();
} else {
this.keepFocus = true;
}
Copy link
Author

Choose a reason for hiding this comment

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

If we are clinking inside the menu, only close it if we clicked inside an item of the list

},

//------------------------------------------------------------------
//
// Handles the mouse entering the results list
//
mouseenter: function (e) {
if(this.avoidMouseEnter) {
this.avoidMouseEnter = false;
return;
}
Copy link
Author

Choose a reason for hiding this comment

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

avoidMouseEnter is set to true by prev() and next(). It can be reset to false also by moving the mouse after prev() or next()

this.$menu.find('.active').removeClass('active');
$(e.currentTarget).addClass('active');
}
Expand Down Expand Up @@ -556,7 +599,7 @@ function ($) {
}

$.fn.typeahead.Constructor = Typeahead;

//------------------------------------------------------------------
//
// DOM-ready call for the Data API (no-JS implementation)
Expand Down