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

typeahead-focus-first=false option to prevent first match from being focused #2916

Closed
wants to merge 1 commit into from

Conversation

wkonkel
Copy link
Contributor

@wkonkel wkonkel commented Oct 31, 2014

Fixes #908.

Should be 100% backwards compatible with existing markup and functionality.

Current functionality (first result is automatically focused as you type):
<input typeahead="..." />

New functionality (first result is NOT automatically focused as you type):
<input typeahead="..." typeahead-focus-first="false" />

@wkonkel wkonkel changed the title typeahead-focus-first option to prevent first match from being focused typeahead-focus-first=false option to prevent first match from being focused Oct 31, 2014
@@ -59,6 +59,8 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap

var appendToBody = attrs.typeaheadAppendToBody ? originalScope.$eval(attrs.typeaheadAppendToBody) : false;

var focusFirst = originalScope.$eval(attrs.typeaheadFocusFirst) !== false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's safer and more consistent to do same check as above (on line 60, to check whether the attrs expression is truthy/not empty) as that would work with any falsey value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from isEditable on line 50 which is also default-true. Line 60 for appendToBody is default-false. If you prefer different style for default-true attributes, I would suggest updating both isEditable and focusFirst, but I think that's outside of the scope of this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, in that case this is fine.

@chrisirhc
Copy link
Contributor

Just reviewed. Looks good except for a minor change before we can merge it. Thank you!

@chrisirhc chrisirhc added this to the 0.12 milestone Nov 2, 2014
@chrisirhc
Copy link
Contributor

Just found one more thing, when the user presses the up arrow key, it should select the last element on the typeahead list. This PR is also missing tests.

@wkonkel
Copy link
Contributor Author

wkonkel commented Nov 3, 2014

Fixed problem with up arrow key and added test coverage.

@@ -80,7 +80,8 @@ describe('typeahead tests', function () {
this.message = function () {
return 'Expected "' + this.actual + '" to be opened.';
};
return typeaheadEl.length === 1 && typeaheadEl.hasClass('ng-hide') === false && liEls.length === noOfMatches && $(liEls[activeIdx]).hasClass('active');

return typeaheadEl.length === 1 && typeaheadEl.hasClass('ng-hide') === false && liEls.length === noOfMatches && (activeIdx === -1 ? !$(liEls).hasClass('active') : $(liEls[activeIdx]).hasClass('active'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're changing this line, could you break this into multiple lines for readability?

@chrisirhc
Copy link
Contributor

Looks great. Just missing one more test.

@wkonkel
Copy link
Contributor Author

wkonkel commented Nov 4, 2014

I'm stuck on this test...

  it('should submit the form when hitting enter', function () {
    $scope.form_was_submitted = false;
    var element = prepareInputEl('<form ng-submit="form_was_submitted=true"><input ng-model="result" typeahead="item for item in source | filter:$viewValue" typeahead-focus-first="false"><button type="submit">submit</button></form>');

    changeInputValueTo(element, 'b');
    triggerKeyDown(element, 13);
    expect($scope.form_was_submitted).toBeTruthy();
  });

It should be triggering the form's ng-submit, but it's not. Any suggestions?

@chrisirhc
Copy link
Contributor

The submit event is not triggering because you need a submit button in the form (for Chrome). I think testing the submission of the form shouldn't be required, since that's browser specific and may/may not require a submit button in the form. This article contains the references to the specification: http://tjvantoll.com/2013/01/01/enter-should-submit-forms-stop-messing-with-that/

Perhaps you can test using ng-keydown event whether the $event.isDefaultPrevented() on the input field itself?

@wkonkel
Copy link
Contributor Author

wkonkel commented Nov 4, 2014

Yes, the expected behavior is that the outer form is submitted when hitting enter from the input and nothing is focused. I've tested it in a browser and it works as expected.

In the test I pasted above, the form already has a submit button <button type="submit">submit</button> and I also tried <input type="submit" /> but that didn't work either. I started down the path of checking for isDefaultPrevented but couldn't figure out how to access/mock the event objects from the tests.

@chrisirhc
Copy link
Contributor

Actually, that could be a quirk in PhantomJS (Edit: It's using Chrome by default, so this shouldn't be the case.) , where the enter key doesn't work as expected.

With regards to finding the isDefaultPrevented, you should be able to do something like:

var element = prepareInputEl('<input ng-model="result" ng-keydown="keyDownEvent = $event; keyDownDefaultPrevented = $event.isDefaultPrevented()" typeahead="item for item in source | filter:$viewValue" typeahead-focus-first="false"></form>');

And then check that keyDownEvent is set and keyDownEventDefaultPrevented is false?
Testing this is indeed tricky.

@wkonkel
Copy link
Contributor Author

wkonkel commented Nov 5, 2014

That approach worked. Thanks!

@chrisirhc
Copy link
Contributor

Excellent work. LGTM. This will be merged in a bit.

@chrisirhc
Copy link
Contributor

I'll be rewording the commit message some of your commits later when I merge, to conform with our git commit message conventions. If you would like to do it yourself, please go ahead.
We're following the conventions at https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y/edit .

@wkonkel
Copy link
Contributor Author

wkonkel commented Nov 7, 2014

Squash merged and reformatted commit message.

@chrisirhc chrisirhc closed this in 35d0cc1 Nov 9, 2014
OronNadiv pushed a commit to lanetix/bootstrap that referenced this pull request Nov 18, 2014
Add typeahead-focus-first option to prevent first match from being
focused.

Currently, the first result is automatically focused as you type. Now, set
`typeahead-focus-first="false"` and the first result is *not*
automatically focused as you type.

Closes angular-ui#908
Closes angular-ui#2916
@mkoperator
Copy link

This is a very sweet feature, however, I think it is missing the action for tab. When nothing is selected pressing enter closes the typeahead, When pressing tab it goes to the next field but does not close the typeahead list. Not sure if this is the right place to note it.

@acgentry
Copy link

+1 to closing the typeahead onblur of the input, with templates the form could be covered in a large drop down and the user has to tab back after tabbing off and escaping the screen if he doesn't want to select anything.

@pnowy
Copy link

pnowy commented May 25, 2015

+1 to closing the typeahead onblur from me too! I have a form when the user can select something from dictionary or enter new value. When user does not select from popup and use tab the popup still appears. Does anyone know how can I prepare byway for this?

lvostinar pushed a commit to Servoy/bootstrap that referenced this pull request Oct 22, 2015
just for navigation, do not select from list
@lvostinar
Copy link

This commit is not really enough for us, not closing the list at tab out is quite a big deal. For now I just disabled the select by tab key, I think tab should be used for navigation only. Our scenario is that the user wants to quickly navigate using tab between input fields but doesn't want to change the value of the typeahead. So tab in/ tab out action should return the control to its initial state.

@icfantv
Copy link
Contributor

icfantv commented Oct 22, 2015

@lvostinar, commenting on old and closed issues is a really good way for us to miss stuff.

As yours is a support related request, please follow the instructions here.

@lvostinar
Copy link

I commented here because I think this issue needs to be reopen. The fix is merged in your codebase but is not complete.

@icfantv
Copy link
Contributor

icfantv commented Oct 23, 2015

@lvostinar, you want it to be reopened because this solution is not enough for you (your exact words). I interpret this to mean that you are wanting an added feature above and beyond what this issue and fix was designed to address.

If you would like us to consider your request, I ask that you please take the time to open a new issue to track this request we will see about addressing it. You can even reference this issue from the new one. Help us help you.

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

Successfully merging this pull request may close these issues.

typeahead list - don't auto select / highlight first item
7 participants