-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Revert previous clip-deprecation fixes in favor of hybrid clip and clip-path. #2989
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with chosen's code, but reverting some changes seams straightforward enough. Also, clip-path
sounds like a simpler approach! 👍
There was a problem hiding this 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 plan to me!
Where is the code dealing with focusin and focusout ? I don't see any added code relying on it. |
It's not in here, according to the last sentence:
|
Correct! I’ll be opening a follow-up PR today. Thanks for the review! |
@harvesthq/chosen-developers /cc @jisraelsen
This PR reverts #2939 and #2943, which attempted to avoid using the deprecated
clip
property and useddisplay: none
, at the cost of a second input to capture focus events.It didn’t work out too well, and has continued to be a headache as we try to use the latest version in Harvest. I think the best thing to do is admit it was the wrong choice and revert it. Juggling focus and key events isn’t standard across browsers, especially while making DOM modifications.
This PR instead leaves the deprecated
clip
property and additionally introduces the same fix usingclip-path
. The result is that browsers that do not support the new standardclip-path
will fallback to using the deprecatedclip
property, without requiring any additional changes.One nice side-effect: while trying to make this all work correctly, I had to dig way into the way we handle focus and blur events in Chosen. If you’re not aware, we’re currently doing a lot of juggling around blur events, since blurring an input technically happens while clicking on an item to select it. I discovered that we can avoid doing all of that if we rely on
focusin
andfocusout
events on the wrapping container, as demonstrated in this jsfiddle (watch forchosenfocus
andchosenblur
events). I’ll be pursuing this further in a separate PR, and expect to be able to delete a bunch of focus-specific properties as well as the extra 100ms the chosen drop is visible — but didn’t want to hold this up on an unrelated change.