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 Safari IME support #11016

Merged

Conversation

SuperKenVery
Copy link
Contributor

@SuperKenVery SuperKenVery commented May 30, 2023

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Currently in Safari, when using IME, use Enter to input the typed characters as text (rather than, say, selected Chinese characters), the whole message will be sent.

After the fix, using Enter to input characters in IME doesn't cause the message to be sent.

Before:
before

After:
after

Tested on Safari, Firefox and Chromium. The problem is fixed on Safari and didn't cause issues on Firefox or Chromium.

See also: https://www.stum.de/2016/06/24/handling-ime-events-in-javascript/

Fuck safari. This is my 2nd pr for it...

Signed-off-by: 许煜恒 xyhken@icloud.com
Type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

@SuperKenVery SuperKenVery requested a review from a team as a code owner May 30, 2023 15:17
@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems labels May 30, 2023
@SuperKenVery
Copy link
Contributor Author

I tried to manually do the test on Safari, Firefox and Chromium. I was able start direct chat and send the Hello, not sure what happened in cypress...

@germain-gg germain-gg requested review from alunturner and removed request for germain-gg June 26, 2023 15:29
@alunturner
Copy link
Contributor

Hello. Firstly thanks for the contribution and completely agree with the sentiment towards Safari. Super useful linked article too.

Playing around with this code locally in safari, it works as you described it. However I'm not convinced we need to add the hasIMECompositionJustEnded. In the example they use this to swallow keyup events but we don't listen for those, so we don't need that code. I think we'd be fine simply swallowing the extra safari event on key down.

When I remove hasIMEComposingJustEnded from the code you've added (plus all references to it) it still seems to work for me.

Would you mind trying this out and seeing if you agree with what I've said?

@SuperKenVery
Copy link
Contributor Author

Oh yeah. You're right 🤣

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane, thanks

@t3chguy t3chguy added this pull request to the merge queue Jan 15, 2024
Merged via the queue into matrix-org:develop with commit 97339ee Jan 15, 2024
21 checks passed
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 3, 2024
Weirdly, there is no 1.11.56, so it apparently was skipped?

Changes in [1.11.57](https://github.com/element-hq/element-web/releases/tag/v1.11.57) (2024-01-31)
==================================================================================================
## 🦖 Deprecations

* Deprecate welcome bot `welcome_user_id` support ([#26885](element-hq/element-web#26885)). Contributed by @t3chguy.

## ✨ Features

* Expose apps/widgets ([#12071](matrix-org/matrix-react-sdk#12071)). Contributed by @charlynguyen.
* Enable the rust-crypto labs button ([#12114](matrix-org/matrix-react-sdk#12114)). Contributed by @richvdh.
* Show a progress bar while migrating from legacy crypto ([#12104](matrix-org/matrix-react-sdk#12104)). Contributed by @richvdh.
* Update Twemoji to Jdecked v15.0.3 ([#12147](matrix-org/matrix-react-sdk#12147)). Contributed by @t3chguy.
* Change Quick Settings icon ([#12141](matrix-org/matrix-react-sdk#12141)). Contributed by @florianduros.
* Use Compound tooltips more widely ([#12128](matrix-org/matrix-react-sdk#12128)). Contributed by @t3chguy.

## 🐛 Bug Fixes

* Fix OIDC bugs due to amnesiac stores forgetting OIDC issuer \& other data ([#12166](matrix-org/matrix-react-sdk#12166)). Contributed by @t3chguy.
* Fix account management link for delegated auth OIDC setups ([#12144](matrix-org/matrix-react-sdk#12144)). Contributed by @t3chguy.
* Fix Safari IME support ([#11016](matrix-org/matrix-react-sdk#11016)). Contributed by @SuperKenVery.
* Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12127](matrix-org/matrix-react-sdk#12127)).
* Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12126](matrix-org/matrix-react-sdk#12126)). Contributed by @t3chguy.
* Fix 1F97A and 1F979 in Twemoji COLR font ([#12177](matrix-org/matrix-react-sdk#12177)).
## ✨ Features

* Expose apps/widgets ([#12071](matrix-org/matrix-react-sdk#12071)). Contributed by @charlynguyen.
* Enable the rust-crypto labs button ([#12114](matrix-org/matrix-react-sdk#12114)). Contributed by @richvdh.
* Show a progress bar while migrating from legacy crypto ([#12104](matrix-org/matrix-react-sdk#12104)). Contributed by @richvdh.
* Update Twemoji to Jdecked v15.0.3 ([#12147](matrix-org/matrix-react-sdk#12147)). Contributed by @t3chguy.
* Change Quick Settings icon ([#12141](matrix-org/matrix-react-sdk#12141)). Contributed by @florianduros.
* Use Compound tooltips more widely ([#12128](matrix-org/matrix-react-sdk#12128)). Contributed by @t3chguy.

## 🐛 Bug Fixes

* Fix OIDC bugs due to amnesiac stores forgetting OIDC issuer \& other data ([#12166](matrix-org/matrix-react-sdk#12166)). Contributed by @t3chguy.
* Fix account management link for delegated auth OIDC setups ([#12144](matrix-org/matrix-react-sdk#12144)). Contributed by @t3chguy.
* Fix Safari IME support ([#11016](matrix-org/matrix-react-sdk#11016)). Contributed by @SuperKenVery.
* Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12127](matrix-org/matrix-react-sdk#12127)).
* Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12126](matrix-org/matrix-react-sdk#12126)). Contributed by @t3chguy.
* Fix 1F97A and 1F979 in Twemoji COLR font ([#12177](matrix-org/matrix-react-sdk#12177)).
## ✨ Features

* Use jitsi-lobby in video channel (video rooms) ([#26879](element-hq/element-web#26879)). Contributed by @toger5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants