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

Extend the JQueryAutoCompleteControl to allow setting a option to blur after click #1464

Conversation

martingabzdil
Copy link

We want to extend the JQueryAutoCompleteControl to allow us to set the option to blur the input field when the selection is made with a click event.

…r the input when selection is made by mouse click
@andy-berry-dev
Copy link
Member

@dchambers:

Hi @martingabzdil, this patch doesn't seem complete since there is no other reference to m_bBlurAfterClick within the class. If you look at the m_bOpenOnFocus variable, which provides a similar option, it is both set to a default value of false within the constructor, and conditionally set to true within setOptions(). Is this what you're intending for m_bBlurAfterClick too?

@dchambers
Copy link
Contributor

Sorry, more updates that occured on the issue:

@martingabzdil

Hi Dominic Chambers Yes. It is set to true with setOptions(). I can add it to the constructor too if you want.

@dchambers

No, I'm sure we can do that. I was just confirming that that's how it was going to work.

@dchambers
Copy link
Contributor

Actually however, I didn't even realize that there was a pull-request for this that included the code in setOptions(). I'd just assumed the 'patch' was what had been given in the original issue description.

@briandipalma
Copy link
Contributor

The default browser action for a click should blur away the input.
This means this issue is probably something we cause ourself.
You can see that in the Control code we are preventing the default action.

// don't propagate this to the keydown (if it's triggered by an enter)
event.stopImmediatePropagation();
event.preventDefault();
return false;

This code might be causing the issue. We could see if deleting it will fix it.
I took a look at the caplin git history. The earliest commit (from 2012) had
this in it.

// don't propagate this to the keydown (if it's triggered by an enter)
event.stopPropagation();
event.preventDefault();

This code was copied from a CT2 repo I think. The commit message is no help.

Merging using cutlass-main-libs->slimshady-libs

It's changed to match what's there currently in another copy of the CT2
codebase so it's difficult to tell exactly why someone decided they wanted to
prevent default (and propagation) but I wouldn't be surprised if it was some
workaround.

I'd remove the code and see if it fixes the issue. If it does I'd change the
PR to be the removal of that code.

It's possible that the workaround was down to a bug in IE where buttons with
no type ended up submitting to the server or trading because they'd pick up
events they weren't meant to. To fix it you just need to set a type on the
button of button if I recall correctly.

@dchambers
Copy link
Contributor

Thanks @briandipalma, that's really helpful.

@dchambers
Copy link
Contributor

There was lots of helpful discussion on this issue that occurred elsewhere. Here's a copy of those messages:

@martingabzdil:

@briandipalma That was the first thing that I tried and indeed removing stopPropagation, preventDefault and return false works.
The problem is this is also connected to another patch PCTLIBRARY-1014 which clears the text after a valid input. So when I removed those, that patch wasn't working, because the events were being propagated and the field set with the text again.
I then decided to leave it there and trigger the blur manually.

@briandipalma:

because the events were being propagated and the field set with the text again.

I don't understand this, a click event on the dropdown is propogating up the DOM and causing the input box to be filled with text?
At first sight this sounds like very weird behaviour to me. Can we fix the behaviour?

@martingabzdil:

Brian Di Palma That's the jQueryAutocomplete behaviour. You start typing, get the autocomplete sugestion, select and it fills the text in the input.
We have a patch for that to prevent this behaviour if you choose to.

@briandipalma:

It looks to me that jQuery sets the input value to the selected value after a select event. If you cancel the event it doesn't do that.
What we are doing is cancelling the event and preventing default. Those being separate side effects we should be able to cancel the event but allow default to still go ahead.
I'd try that but keep in mind that jQuery conflates the two actions because...well I'm not sure there is a good reason for doing that but that's how jQuery behaves (I think if you stop propagation it prevents default or vice versa). It's worth experiementing with the removal of the prevent default logic while maintaining the stop propagation logic.

@martingabzdil:

@briandipalma This widget creates a custom event after the select event is triggered and then handles it differently than you are expecting. The default behavior for the autocompleteselect is as I posted above, setting the input value with the value of the selected item and focus back on the input. The widget also has conditions to skip setting the input value if you use event.preventDefault() or return false; in the select method but you still gain the focus on the input.
I tried removing event.preventDefault() and/or event.stopImmediatePropagation() and/or return false; however this doesn't work with the desired outcome.

@briandipalma:

If the control is that poorly designed then I guess we have no choice but to work around its flaws like this.
I consider my CR done, back to the BRJS team.

thecapdan added a commit that referenced this pull request Jul 29, 2015
…-allow-blur-after-click

Extend the JQueryAutoCompleteControl to allow setting a option to blur after click
@thecapdan thecapdan merged commit 2e07edd into BladeRunnerJS:master Jul 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants