Skip to content
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.

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

AutoComplete: List doesn't close when it loses focus after scrolling #981

Closed
SherryH opened this issue Jul 9, 2013 · 12 comments
Closed

Comments

@SherryH
Copy link

SherryH commented Jul 9, 2013

The dropdown list of an autocomplate widget would normally close when it loses focus, but it does not close after you scroll the list by clicking the up/down arrows in the scrollbar.

To Reproduce:

  1. Go to http://jsfiddle.net/sherryh/zs8jh/1/
  2. Type in a char such as "a"
  3. Scroll down the autocomplete box a little bit by clicking on the Down Arrow Key
  4. Click somewhere else

Expected Outcome:
The autocomplete dropdown list should hide.

Actual Outcome:
It doesn't.

@gurumvg
Copy link
Contributor

gurumvg commented Jul 10, 2013

This issue should surface in browsers (IE, Chrome) other than Firefox, as Firefox doesn't fire blur event when focus is lost / moved from any focusable element to scroll-bar. In AutoComplete case, blur doesn't get fired when user clicks on the scroll-bar when the focus was previously on the input field.

The quick fix for this that I could think right now is to also check the target for html or body using target.test('html,body') in the below 'doc' handler method in autocomplete-list submodule.

    _afterDocClick: function (e) {
        var boundingBox = this._boundingBox,
            target      = e.target;

        if(target.test('html,body') || (
                target !== this._inputNode &&
                target !== boundingBox &&
                target.ancestor('#' + boundingBox.get('id'), true))){

            this.hide();
        }
    }

@lsmith. Are you good with this fix? If so, I will submit a pull request for this fix.

@rgrove
Copy link
Contributor

rgrove commented Jul 17, 2013

@gurumvg Actually, looking at this code again, I'm wondering why that target.ancestor() test is testing for truthiness. That seems to be the cause of the problem, since it would effectively prevent the list from being hidden unless the target is inside the bounding box, which is the opposite of the intent.

@saw, it looks like you made that change in commit ebb4caa. Do you remember why the ancestor test is truthy instead of falsy? Just a mistake, or was there some reason I'm missing?

@ghost ghost assigned rgrove Jul 17, 2013
@gurumvg
Copy link
Contributor

gurumvg commented Jul 17, 2013

@rgrove I believe the target.ancestor() check was added to account for the case when user (selects) clicks on any of the list item, in which case the widget should be hidden.

@rgrove
Copy link
Contributor

rgrove commented Jul 17, 2013

@gurumvg Nope, item clicks are handled by _onItemClick(), which selects the clicked item and fires a select event. The default handler for the select event then hides the list.

If you look at ebb4caa, the original behavior of _afterDocClick() was to hide the list when anything outside the list was clicked. After ebb4caa, that changed to anything inside the list, which doesn't appear to have been intentional, if I'm understanding the commit message.

@gurumvg
Copy link
Contributor

gurumvg commented Jul 19, 2013

agreed @rgrove, will try to convert the truthy check to falsy and see if it breaks anything.

@gurumvg
Copy link
Contributor

gurumvg commented Jul 22, 2013

Changing the truthy target.ancestor('#' + boundingBox.get('id'), true) check to falsy resolved the issue without any side-effects.

@saw
Copy link
Contributor

saw commented Jul 25, 2013

Hrm, thinking back into the past I think this must have been a mistake, I was just trying to fix a different issue.

@rgrove
Copy link
Contributor

rgrove commented Jul 25, 2013

Thanks guys! I'll get this fixed soon.

@jenny
Copy link
Member

jenny commented Sep 3, 2013

Moving out of Sprint 9 into Sprint 10. Feel free to unassign milestone if you wish.

@SherryH
Copy link
Author

SherryH commented Nov 14, 2013

Hello, just wondering if you are still planning to fix this issue?

@rgrove
Copy link
Contributor

rgrove commented Nov 14, 2013

@SherryH Thanks for the reminder! So sorry for forgetting about this; I got busy and it totally slipped my mind. This is fixed now in the dev-master and dev-3.x branches, and should be in the next release.

@rgrove rgrove closed this as completed Nov 14, 2013
@SherryH
Copy link
Author

SherryH commented Nov 15, 2013

@rgrove Thanks very much! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants