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

Update input text on autocomplete option highlight #16897

Closed
jimf opened this issue Jun 15, 2016 · 21 comments · Fixed by #16899
Closed

Update input text on autocomplete option highlight #16897

jimf opened this issue Jun 15, 2016 · 21 comments · Fixed by #16899
Labels

Comments

@jimf
Copy link
Contributor

jimf commented Jun 15, 2016

Just want to start out by saying that I'd be happy to implement any work that might come out of this issue. I'm more looking for a general +1 on the idea, as well as hash out what the api might look like. With that out of the way...

Goal: I'm looking to replace a jQuery-UI based autocomplete implementation with Awesomplete. However, for feature parity, I need the input text to update as I navigate through autocomplete choices. More concretely:

  1. A list of autocomplete options are shown (e.g., I've typed "ja" and get the choices "java" and "javascript")
  2. I navigate through the options
  3. As I navigate, the input text updates to reflect the highlighted choice (pressing down once will update the field text to "java" and so on)
    • hitting escape closes the autocomplete dialog and reverts the input text back to the original value ("ja")
    • "selecting" the option works as is already implemented

I'm fairly confident that this can all be achieved with the existing API. Live-updating the input text is fairly trivial with the awesomplete-highlight event, and the original value can be cached on awesomplete-open. The hacky part comes with the escape functionality, as the code currently makes no distinction between pressing escape and any of the other methods of closing. I might be able to do something with setTimeout / clearTimeout, but it's definitely not ideal, hence this issue.

So open questions:

  • Is there general value in this behavior? Does it fall inline with the goals of this library?
  • If yes, would this be best wrapped up in a single constructor option (and if so, what's a good name), or would it be preferred to leave much of this in userland and a) expose an update to the escape/blur functionality to either close in a "close and do nothing" state, or b) alternatively fire off a second, more meaningful event? Or something else entirely?
@vlazar
Copy link
Collaborator

vlazar commented Jun 15, 2016

@jimf Thanks for very detailed explanation.

So how does jQuery UI autocomplete widget handles this case? Does it have option, event or something else?

@jimf
Copy link
Contributor Author

jimf commented Jun 15, 2016

@vlazar this is the default behavior. Afaict, there is actually no direct way to disable it.

@vlazar
Copy link
Collaborator

vlazar commented Jun 15, 2016

Personally I'm leaning towards having additional event and implementing your use case in userland.

b) alternatively fire off a second, more meaningful event

It will be trivial to add new event here: https://github.com/LeaVerou/awesomplete/blob/gh-pages/awesomplete.js#L70

It looks like hitting escape is the only case when the currently highlighted choice should be reverted on close(). In all other cases where close() is called and already triggers awesomplete-close event should keep currently highlighted.

But then, of course, it's an API and obvious question would be wether we should add one more event or not.

Also, I'm a bit worried about meaningful event name. If we'll add, say awesomplete-cancel it will have no meaning for Awesomplete itself, since it doesn't change input currently, so there is nothing to cancel.

@jimf
Copy link
Contributor Author

jimf commented Jun 15, 2016

Okay, I've hacked together a proof-of-concept that does what I want using the existing API (only tested in Chrome): https://jsfiddle.net/jimfitz/wgk9a32c/. I'm not in love with the timeout stuff, but I don't think there's an alternative without updating the library. The cancel-event approach would allow me to consolidate onClose and onComplete into a single, more purposeful callback. A dedicated option would get rid of all of the callbacks and state variables, but at the expense of more API surface and library code.

PS: Possibly worth a secondary PR, but it seems the close event gets fired, even when the dialog isn't open. Is this known/intentional? I had to work around it in the fiddle. I'd imagine close could just be updated with an if (opened) check to early-return.

@jimf
Copy link
Contributor Author

jimf commented Jun 16, 2016

Also, I'm a bit worried about meaningful event name. If we'll add, say awesomplete-cancel it will have no meaning for Awesomplete itself, since it doesn't change input currently, so there is nothing to cancel.

Another option worth considering is adding an argument to the cancel event that would expose the cancel method. The following example would suffice for my needs:

$.fire(this.input, "awesomplete-close", { escape: true });

However, it may be beneficial to encode the "close reason" for all the places where the dialog closes, so one could differentiate between escape/blur/confirm etc. directly in the event callback.

@vlazar
Copy link
Collaborator

vlazar commented Jun 17, 2016

I've played a bit with your example at https://jsfiddle.net/jimfitz/wgk9a32c/. I don't know if this is something you care about, but jQueryUI example at http://jqueryui.com/autocomplete/ does not revert selected text on blur event in addition to keydown esc, while yours does.

Here is how I would probably implement this https://jsfiddle.net/yaufeLwn/1/. It also handles blur/esc similarly to jQueryUI.

Is it simple enough for not to add new features to Awesomplete? Or maybe there are other good reasons to support add new event or "close reason" so that library users can simplify their code?

EDIT: Updated URL to fiddle.

@jimf
Copy link
Contributor Author

jimf commented Jun 17, 2016

I don't know if this is something you care about, but jQueryUI example at http://jqueryui.com/autocomplete/ does not revert selected text on blur event in addition to keydown esc, while yours does.

Yes, sorry. I fixed that behavior in my own code but didn't bother to update the fiddle. The behavior you describe/implemented is what I'm after.

Here is how I would probably implement this [snip]

Nice! I got so caught up trying to do this with the given Awesomplete events that it didn't even occur to me to just use a keydown. This is much nicer.

Is it simple enough for not to add new features to Awesomplete? Or maybe there are other good reasons to support add new event or "close reason" so that library users can simplify their code?

At this point I am satisfied. Thank you for your time! 🍻

@jimf jimf closed this as completed Jun 17, 2016
@vlazar
Copy link
Collaborator

vlazar commented Jun 18, 2016

I got so caught up trying to do this with the given Awesomplete events that it didn't even occur to me to just use a keydown.

Yep, exactly. I was trying to do the same thing initially. It's like trying to do something X-way immediately shuts off the brain for other options :)

@LeaVerou
Copy link
Owner

I like the proposed behavior.
The fact that it needs a keydown instead of one of the existing events sounds like a failing of our API to me. It sounds like awesomplete-close does need a parameter about the reason for closing. Thoughts?

@vlazar
Copy link
Collaborator

vlazar commented Jun 20, 2016

Ok, here is the list where awesomplete-close is fired currently:

  • on blur event
  • on keydown with Esc key
  • on form submit event
  • right before awesomplete-selectcomplete event
  • when user typed less chars than minChars in input
  • and in case no matches found

What would be the good names for parameter value in those cases?

We probably don't want to attach semantic meaning to this parameter. For example, one may want similar behavior on blur and on Esc, or a different behavior, like in this question with jQueryUI replacement where on Esc it reverts selected text, but on blur it keeps it.

@LeaVerou
Copy link
Owner

reason? With values like blur, esc, submit, select, minchars, nomatches?

@jimf jimf reopened this Jun 20, 2016
@jimf
Copy link
Contributor Author

jimf commented Jun 20, 2016

reason? With values like blur, esc, submit, select, minchars, nomatches?

👍 from me. I'd be happy to implement if we're in agreement.

@LeaVerou
Copy link
Owner

Sure, as long as you update the docs too!

@vlazar
Copy link
Collaborator

vlazar commented Jun 21, 2016

Can you do the tests too, please?

@vlazar
Copy link
Collaborator

vlazar commented Jun 21, 2016

Just reviewed the list of cases when awesomplete-close event is fired. It's probably a good time to do some cleanup as well?

For example, I agree with this concern:

PS: Possibly worth a secondary PR, but it seems the close event gets fired, even when the dialog isn't open. Is this known/intentional? I had to work around it in the fiddle. I'd imagine close could just be updated with an if (opened) check to early-return.

Also, does minchars case is valuable enough to be implemented as a separate reason or at all? What if we combine it with nomatches?

@jimf
Copy link
Contributor Author

jimf commented Jun 21, 2016

Sure, as long as you update the docs too!

Will do.

Can you do the tests too, please?

Of course!

It's probably a good time to do some cleanup as well?

Sounds good. I'll make that change in a separate PR.

Also, does minchars case is valuable enough to be implemented as a separate reason or at all? What if we combine it with nomatches?

Combining makes sense to me, but I don't have strong feelings.

jimf added a commit to jimf/awesomplete that referenced this issue Jun 22, 2016
Emit a reason when firing `awesomplete-close` events to provide
transparency to attached event listeners regarding the circumstance in
which the close was triggered. Reasons include `blur`, `esc`, `submit`,
`select`, and `nomatches`.

Closes LeaVerou#16897
jimf added a commit to jimf/awesomplete that referenced this issue Jun 22, 2016
Emit a reason when firing `awesomplete-close` events to provide
transparency to attached event listeners regarding the circumstance in
which the close was triggered. Reasons include `blur`, `esc`, `submit`,
`select`, and `nomatches`.

Closes LeaVerou#16897
jimf added a commit to jimf/awesomplete that referenced this issue Jun 22, 2016
Emit a reason when firing `awesomplete-close` events to provide
transparency to attached event listeners regarding the circumstance in
which the close was triggered. Reasons include `blur`, `esc`, `submit`,
`select`, and `nomatches`.

Closes LeaVerou#16897
@El4a
Copy link

El4a commented Dec 19, 2016

Is this new behavior released yet? I can't tell but I don't think it is?

@El4a
Copy link

El4a commented Dec 21, 2016

Also: I used the enhancement from @vlazar fiddle (https://jsfiddle.net/yaufeLwn/1/). But this behavior does conflict with 'autoFirst:true'. I kinda need both features to work. But if you use them together, you lose the ability to backspace and type something else.

@vlazar
Copy link
Collaborator

vlazar commented Dec 25, 2016

Is this new behavior released yet? I can't tell but I don't think it is?

@El4a What behavior are you referring to?

#16898 and #16899 are released in v1.1.1

@El4a
Copy link

El4a commented Dec 26, 2016

@vlazar I meant the proposed behavior described by jimf in the original post concerning :
"A list of autocomplete options are shown (e.g., I've typed "ja" and get the choices "java" and "javascript")
I navigate through the options
As I navigate, the input text updates to reflect the highlighted choice (pressing down once will update the field text to "java" and so on)"

I just got the latest zip and this feature didn't seem to be included, so I used the fiddle you made to enhance it.

About my other issue that it conflicts with the setting autoFirst:true -> I realised that's to be expected.

@vlazar
Copy link
Collaborator

vlazar commented Dec 27, 2016

No, the proposed behavior is not supported out of the box, but is easy to implement.

This issue however led to #16899 with additional info available on events, so it should be even easier o implement than in original fiddle.

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

Successfully merging a pull request may close this issue.

4 participants