Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use two inputs for Chosen’s focus and search. #2962

Merged
merged 1 commit into from
Apr 9, 2018
Merged

Conversation

adunkman
Copy link
Contributor

@harvesthq/chosen-developers as a follow-up to #2939, fixing #2953.

As mentioned in #2939:

As an aside, I originally went with two inputs — a new one to catch the focus events when the results drop was closed, and the existing search input — however, it quickly became ridiculous because letters typed into the focus input cause the results drop to filter and show at the same time. This meant that I had to forward events that caused letters to be typed into the focus input over to the search input… and simulating keyboard events is gross.

Using a single input that moves ended up being significantly easier.

This isn’t my first choice, but IE11 has demonstrated interesting behavior when moving a focused input element in the DOM:

  • If an input is moved in the DOM during the processing of a focus event, it results in the input becoming blurred (this is the same cross-browser).
  • In IE11, the input never triggers a blur event, it just simply no longer is focused.
  • In IE11, the browser seems to still considers the element focused, and therefore calling focus() on it does not cause it to become focused.

Here’s a minimal example:

IE11 Chrome
screencast showing that blur events are not fired in IE11 screencast showing blur events being fired in Chrome

I dug into this quite a bit, but I wasn’t able to come up with a way to reliably move the input cross-browser.

The result: introducing a second input, registering the same event handlers on it, and then syncing the values of the inputs when jumping focus between the two inputs when the results drop is shown. It’s a little hacky, but I’m not confident that I can find a less-hacky solution. If you can think of one, I’m so interested in doing that. 😅

@@ -179,10 +187,8 @@ class Chosen extends AbstractChosen
@container.addClass "chosen-container-active"
@active_field = true

@search_field.val(@search_field.val())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually a difference between the jQuery and Prototype versions — the Prototype version seen below is also removed, but it was:

@search_field.value = this.get_search_field_value()

That’s almost the same, but not quite — resulting in the initial keypress being doubled in the Prototype version due to the change in that method (for example, keyboard focusing the element and pressing R resulted in RR in Prototype but R in jQuery).

I couldn’t see a reason for setting the value of the field to… itself… so I dropped it completely to make the behavior consistent.

Looking at the git history, it appears to originate in 465eb61, the initial commit by @pfiller, humorously mentioning “Plenty of bugs at this point!”

@@ -70,6 +70,7 @@ $chosen-sprite-retina: url('chosen-sprite@2x.png') !default;
cursor: pointer;
opacity: 0;
position: absolute;
width: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This small CSS change causes the carat to disappear in IE11. Why does the carat appear on-screen for a transparent input in the first place? I don’t have a good answer to that.

@focus_field.on 'focus.chosen', (evt) => this.input_focus(evt); return

transfer_value = () =>
@search_field.val(@search_field.val() + @focus_field.val())
Copy link
Contributor

Choose a reason for hiding this comment

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

I left this concatenation in because @adunkman said there's a way to leave a single select and come back to it with a value present. I haven't been able to reproduce that, but I trust @adunkman.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may be trusting me more than I trust me… I’ll do some extra testing and try to figure it out, otherwise I’ll add a commit to drop the concat. 🐈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn’t figure it out! Must have been mistaken. Removed the concat and squashed the commits to prep for final review.

Moving a single input to catch keyboard events isn’t reliable in IE11, so we must use two for adequate cross-browser support.

Note: the `setTimeout`s are needed for the cut/paste events to make sure the
value being read from the `focus_field` happens after the events finish
changing the input's value.
Copy link
Contributor

@braddunbar braddunbar left a comment

Choose a reason for hiding this comment

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

I don't love having to do this…but I also don't have a better solution so I support this change! 🤷‍♂️

Many, many, many thanks for wading through this Andrew. You are the best. ❤️

@adunkman
Copy link
Contributor Author

adunkman commented Apr 9, 2018

I don't love having to do this…

Samesies. Thanks for the reviews and assistance, all! ❤️

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

Successfully merging this pull request may close these issues.

3 participants