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

Remove clip property by moving chosen-single’s search input. #2939

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

adunkman
Copy link
Contributor

@harvesthq/chosen-developers to fix #2816, replaces #2780.

This PR removes the deprecated clip property by replacing it with display: none and display: block, just as @bingxie did in #2780 — however, this PR also moves the search input when using a single-select chosen to fix focus issues.

The core issue is that we were relying on the search input to catch keyboard events (including tabbing to the field), and that for single-select chosen instances, the search input was inside of the results drop that is hidden when you’re tabbing to the field.

When using left: -9999px, the element was still technically visible (although it wasn’t) so it could catch keyboard events. When using clip: rect(0,0,0,0), it was also still technically visible (although it wasn’t).

For single-select chosen instances:

  • On initial render, the search input is rendered outside the results drop, over the selected option area. It’s set to opacity: 0 via CSS so it can receive keyboard events, but not adjust the design.
  • On results show, the search input is inserted into the results drop. Since elements can only exist in one place in the DOM at a time, it’s automatically removed from the previous location. The opacity: 0 CSS rule no longer applies.
  • On results hide, the search input is inserted back into the selected option area, which automatically removes it from the results drop. The opacity: 0 CSS rule applies once more.

/cc @Mateuspv as the original issue reporter


It looks like CI is failing to run tests because it can’t load jQuery — I checked for a quick fix but we may need to stop relying on a CDN for build stability. I’ll confirm that this is not just an issue with this PR, and then open up a new PR to fix the build. Until then, you can run the tests locally to confirm, if you’d like.

@adunkman
Copy link
Contributor Author

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.

@adunkman
Copy link
Contributor Author

Confirmed that the broken build is the same on a fresh branch from master: https://travis-ci.org/harvesthq/chosen/builds/330842693

We need not rely on clip: rect() if we move the input — we can instead use display: block/none.
@adunkman
Copy link
Contributor Author

Rebased to master after #2940 to fix build.

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.

Seems like a good approach to me! I'm in! 👍

@adunkman adunkman merged commit 290ba2c into master Jan 22, 2018
@adunkman adunkman deleted the replace-clip-prop branch January 22, 2018 16:31
@taichunmin
Copy link

There are still one clip:
https://github.com/harvesthq/chosen/blob/master/sass/chosen.scss#L146

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

Successfully merging this pull request may close these issues.

Remove CSS clip property
3 participants