Issue #19: added support for scrollbar on dropdown menu #30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Originally, using CSS, you could force the menu to have a max-height and use a scroll (overflow:auto or overflow-y:auto) to reach the list items that where outside the visual area. However, clicking on the scrollbar that could appear would not work (you could only scroll using the mouse wheel, trackpad gesture or similar). The reason was that any click on it would fire a blur event that would close the menu. This commit solves this issue by making a difference between clicks on a list item (<li>) or clicks anywhere else inside the menu (including the scrollbar). The first case will produce the original behavior (item is selected and then the menu is closed). The second one will not select an item and will avoid the closing of the menu (also it will reposition the focus on the input element). The second part of this enhancement is related with navigation using up and down arrows: now that we have a "hidden" area, we must assure that any selected item is kept away from that area by forcing the menu to scroll (vertically) accordingly. In order to achieve this: * When going up: prev() method now checks if the top border of the newly selectes <li> element is inside the visible area or not. If not, the menu is scrolled (using jQuery's scrollTop method) to the exact position of that top border (leaving the mentioned element fully visible). * When going down: next() method does something similar to the previous case but taking in consideration the bottom border of the selected <li> element It was also necessary to take care of the mouseenter event that could be triggered when we scroll programmatically (the mouse is not moved, but the elements behind the mouse - the menu items - do move because of the scrolling movement). The solution for that was setting a variable that tells the next mouseenter method invocation to avoid changing the active element, after prev() or next() is called (if the user moves the mouse after prev() or next() the mouseenter can start changing the active element again... this is done in order to avoid messing the expected behavior of mouseenter in cases where prev() or next() makes the menu scroll but the mouse never leaves the element it is currently over, which could happen for small scrolling distances).
| @@ -363,14 +363,28 @@ function ($) { | |||
| // Selects the next result | |||
| // | |||
| next: function (event) { | |||
| var active = this.$menu.find('.active').removeClass('active'); | |||
| var that = this; | |||
| var $menu = that.$menu; | |||
There was a problem hiding this comment.
We are going to use this.$menu very often, so I believe using a variable gives shorter file, specially after "minification"
|
I forgot to make one final adjustment. It seems that the blur method has an error in the verification of the focus owner: Where it says if (!that.$menu.is(':focus')) {It should be: if (!that.$element.is(':focus')) {I'll add a commit to this pull request to change this. I don't know if in branch 2.0 this has been solved or if there is any reported issue about this that could be closed after this solution. |
Blur method fires a timeout which will check where the focus is after 150 ms in order to close the menu or keep it open. However, that verification was checking if the $menu was the owner of the focus. $menu is an "ul", so it can't be the owner of any focus. Probably the real intetion was to check $element, so I made the corresponding change. Note: in issue tcrosen#24 @smucode suggest checking for $menu.is(':hover'). That could be a better solution to the problem discussed in that issue... or probably there's even a better one out there... It would be a good idea to give it some thoughts in order to fix that issue too.
|
I made an online demo for testing in JSFiddle: http://jsfiddle.net/r44dt/ |
I was considering clicking "inside an item" as equivalent to clicking inside an anchor inside the menu. But there was a case that I forgot: when someone clicks in the "highlighted area" of the text inside the anchor. That area is achieved by surrounding the corresponding text with a <strong> tag. So, when clicking on that area, the target for the event fired will not be the anchor, but the strong tag. This made it necessary to check not only if the tagName for the target is equal to 'A', but also if the tagName of the parent of the target is (this is the case when we click inside the highlighted area).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements enhancements needed to give support for features mentioned on issue #19: scrollbar support on menu.
Originally, using CSS, you could force the menu to have a max-height and use a scroll (overflow:auto or overflow-y:auto) to reach the list items that where outside the visual area. However, clicking on the scrollbar that could appear would not work (you could only scroll using the mouse wheel, trackpad gesture or similar). The reason was that any click on it would fire a blur event that would close the menu.
This commit solves this issue by making a difference between clicks on a list item or clicks anywhere else inside the menu (including the scrollbar). The first case will produce the original behavior (item is selected and then the menu is closed). The second one will not select an item and will avoid the closing of the menu (also it will reposition the focus on the input element).
The second part of this enhancement is related with navigation using up and down arrows: now that we have a "hidden" area, we must ensure that any selected item is kept away from that area by forcing the menu to scroll (vertically) accordingly.
In order to achieve this:
It was also necessary to take care of the mouseenter event that could be triggered when we scroll programmatically (the mouse is not moved, but the elements behind the mouse - the menu items - do move because of the scrolling movement). The solution for that was setting a variable that tells the next mouseenter method invocation to avoid changing the active element, after prev() or next() is called (if the user moves the mouse after prev() or next() the mouseenter can start changing the active element again... this is done in order to avoid messing the expected behavior of mouseenter in cases where prev() or next() makes the menu scroll but the mouse never leaves the element it is currently over, which could happen for small scrolling distances).