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

Fix ios chrome ime double input issue. #4049

Merged

Conversation

ulion
Copy link
Contributor

@ulion ulion commented Jan 19, 2021

Is this adding or improving a feature or fixing a bug?

Fix a bug

What's the new behavior?

IME input won't duplicate.

How does this change work?

IOS chrome looks just fine, so no need special code on compositionEnd to insertData.

Have you checked that...?

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)

Does this fix any issues or need any specific reviewers?

Fixes: #
Reviewers: @

@ianstormtaylor
Copy link
Owner

Thanks @ulion!

@@ -613,7 +614,7 @@ export const Editable = (props: EditableProps) => {
// aren't correct and never fire the "insertFromComposition"
// type that we need. So instead, insert whenever a composition
// ends since it will already have been committed to the DOM.
if (!IS_SAFARI && !IS_FIREFOX && event.data) {
if (!IS_SAFARI && !IS_FIREFOX && !IS_IOS && event.data) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks fine, but it's stale as we recently updated it to IS_FIREFOX_LEGACY. Could you do a quick rebase and then we can land this? Thanks @ulion!

@ulion ulion force-pushed the hotfix/fix_ios_chrome_ime_double_input branch from b7e4ac7 to aba9997 Compare August 4, 2021 13:38
@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2021

🦋 Changeset detected

Latest commit: 29baeee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
slate-react Patch
slate Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ulion
Copy link
Contributor Author

ulion commented Aug 4, 2021 via email

@dylans
Copy link
Collaborator

dylans commented Aug 4, 2021

Thanks @ulion, I'm making a push to land obvious PRs asap, so I hope to get this merged very soon.

@dylans
Copy link
Collaborator

dylans commented Aug 5, 2021

@ulion This one was missing a changeset so I've added a PR on your branch. If you don't mind please accept and push into this PR and then I'll land it.

.changeset/metal-trees-rest.md Outdated Show resolved Hide resolved
.changeset/metal-trees-rest.md Outdated Show resolved Hide resolved
@dylans
Copy link
Collaborator

dylans commented Aug 6, 2021

@ulion sorry about the prettier issue, if you can please run yarn lint:prettier --write on the PR and push then I can merge. Or just give me edit permission on the PR and I'll fix and merge. Thanks.

@ulion
Copy link
Contributor Author

ulion commented Aug 6, 2021

@ulion sorry about the prettier issue, if you can please run yarn lint:prettier --write on the PR and push then I can merge. Or just give me edit permission on the PR and I'll fix and merge. Thanks.

done both.

@dylans dylans merged commit 6c84422 into ianstormtaylor:main Aug 6, 2021
dylans pushed a commit to dylans/slate that referenced this pull request Sep 13, 2021
* Fix ios chrome ime double input issue.

* add changeset

* pretty fix.

Co-authored-by: Dylan Schiemann <dylan@dojotoolkit.org>

Thanks @ulion!
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 this pull request may close these issues.

3 participants