Add handlers for composition on Inputs #1082

Tanney-102 · 2022-12-21T15:49:31Z

Self Checklist

I wrote a PR title in English.
I added an appropriate label to the PR.
I wrote a commit message in English.
I wrote a commit message according to the Conventional Commits specification.
I added the appropriate changeset for the changes.
[Component] I wrote a unit test about the implementation.
[Component] I wrote a storybook document about the implementation.
[Component] I tested the implementation in various browsers.
- Windows: Chrome, Edge, (Optional) Firefox
- macOS: Chrome, Edge, Safari, (Optional) Firefox
[New Component] I added my username to the correct directory in the CODEOWNERS file.

Related Issue

Fixes #977

Summary

일본어 등 언어에서 text composition 시 IME 상 control key 입력이 dom의 이벤트로 핸들링 되지 않도록 합니다.
3f89218 에서 추가한 훅은 다른 커밋에서 제거되니 참고 부탁드립니다.
위 케이스처럼 뒤 커밋에서 다시 변경된 로직이 몇몇 있습니다. 새로운 훅을 작성하고 적용한 것이기 때문에 file changed를 바로 보시는 게 편하실 수도 있겠습니다.

Details

useKeyboardActionLockerWhileComposing 훅을 작성합니다.

keyboard event handlers를 인자로 받아 wrapping 합니다.
composition 중 일때 발생한 키보드 이벤트에 대해서는 핸들러를 실행하지 않도록 합니다.
어떤 키 입력에 대해 이벤트 핸들링을 막아줄 것인지는 keysToLock 인자를 보고 결정합니다.
keysToLock 인자를 제공하지 않으면 모든 키 입력에 대해 이벤트 헨들링을 막습니다.
예를 들어 해당 훅을 적용한 상태에서 일본어 키보드를 사용하면 다음과 같은 플로우를 경험하게 됩니다.
- 일본어 문자 입력
- KeyDown 이벤트 발생 => composition 전이므로 핸들러 실행
- CompositionStart 이벤트 발생 => 지금부터는 composing 상태
- KeyUp 이벤트 발생 => composition 중이므로 핸들러 실행X
  
  ...
- IME control key 입력(예를 들어 tab)
- KeyDown 이벤트 발생 => composition 중이므로 핸들러 실행X
- KeyUp 이벤트 발생 => composition 중이므로 핸들러 실행X
  
  ...
- Composition을 종료시키는 IME control key 입력(예를 들어 enter, escape)
- KeyDown 이벤트 발생 => composition 중이므로 핸들러 실행X
- CompositionEnd 이벤트 발생 => composition 종료
- KeyUp 이벤트 발생 => composition이 끝났으므로 핸들러 실행
Safari에 경우 enter나 escape을 입력해도 CompositionEnd 이후에 KeyDown 이벤트가 발생하는 버그가 있습니다.
- 이에 대한 해결 방식은 desk PR을 참고해주세요.
처음에는 isComposing 상태를 ref로 관리했으니 nativeEvent.isComposing을 참조하는 방식으로 변경합니다.

TextField와 TextArea에 useKeyboardActionLockerWhileComposing 훅을 적용합니다.

모든 IME control key에 대해 대응하지는 않습니다.
예를 들어 일본어 키도드 IME에서 히라가나로 변환하는 control + J 키처럼 특정 언어에서만 사용되는 키는 굳이 대응하지 않습니다.
input에 어떤 언어소스든 들어올 수 있고, 특정언어에서는 IME control key로 사용되지만 다른 언어에서는 아닌 경우도 있기 때문입니다.
따라서 대부분의 IME에서 공통적으로 사용될 것으로 기대되는 key들에 대해서만 대응합니다.
- composition end와 관련있는 enter, escape
- IME 상 이동과 관련있는 spacebar, tab, arrows

Breaking change or not (Yes/No)

Desk에 이미 비슷한 방식으로 대응이 되어있습니다.
해당 로직을 bezier로 대체하는 작업은 필요합니다.

References

keys event during composition

changeset-bot · 2022-12-21T15:49:33Z

🦋 Changeset detected

Latest commit: 461a50a

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

This PR includes changesets to release 2 packages

Name	Type
@channel.io/bezier-react	Minor
bezier-figma-plugin	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

…vent handler manually

- this hook wrap keyboard event handlers to lock while composing

…tLocker hook

…ion from safari event check - add comments about safari event check

…ef with nativeEvent.isComposing So we don't have to put composition event handlers into our inputs

…gument

codecov · 2022-12-22T10:56:11Z

Codecov Report

Base: 72.07% // Head: 72.14% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (461a50a) compared to base (85ba690).
Patch coverage: 86.95% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           next-v1    #1082      +/-   ##
===========================================
+ Coverage    72.07%   72.14%   +0.07%     
===========================================
  Files          223      225       +2     
  Lines         3051     3070      +19     
  Branches       840      845       +5     
===========================================
+ Hits          2199     2215      +16     
  Misses         728      728              
- Partials       124      127       +3

Impacted Files	Coverage Δ
...nts/Forms/useKeyboardActionLockerWhileComposing.ts	`81.25% <81.25%> (ø)`
.../src/components/Forms/Inputs/TextArea/TextArea.tsx	`100.00% <100.00%> (ø)`
...rc/components/Forms/Inputs/TextField/TextField.tsx	`54.12% <100.00%> (+0.42%)`	⬆️
...nts/Forms/Inputs/constants/CommonImeControlKeys.ts	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2022-12-22T10:58:10Z

Chromatic Report

🚀 Congratulations! Your build was successful!

sungik-choi · 2022-12-26T08:53:09Z

.changeset/lovely-planets-yawn.md

+"@channel.io/bezier-react": minor
+---
+
+keyboard event locker added to inputs


체인지로그를 더 자세히 적어주셔도 좋을 거 같아요. Composing 중 common ime control keys에 대해 keyboard event handler가 호출되지 않아야한다. 라는 내용이 포함되면 좋겠습니다. (영문)

sungik-choi · 2022-12-26T08:53:33Z

packages/bezier-react/src/components/Forms/Inputs/TextArea/TextArea.test.tsx

@@ -184,6 +185,34 @@ describe('TextArea 테스트 >', () => {
      expect(textareaElement.selectionEnd).toEqual(TEST_INITIAL_VALUE.length)
    })
  })
+
+  describe('Composing 중 common ime control keys에 대해 keyboard event handler가 호출되지 않아야한다. >', () => {


영문으로 작성해주세요!

sungik-choi · 2022-12-26T09:03:06Z

packages/bezier-react/src/components/Forms/Inputs/TextField/TextField.test.tsx

+      const input = rendered.getElementsByTagName('input')[0]
+
+      COMMON_IME_CONTROL_KEYS.forEach(async (key) => {
+        const isCompositionStartFired = fireEvent.compositionStart(input)


fireEvent 대신 userEvent 를 통해 테스트 가능할까요? (관련 링크)

userEvent에서는 composition을 mocking할 수 있는 방법을 따로 지원하지 않습니다 😢

CHEWCHEWW

저는 여전히 해당 훅을 베지어에서 지원해줘야 하는 지에 대한 의문이 있긴 합니다. 또한 크로스브라우징을 지원해주는 것과는 별개로, 특정 브라우저 여부를 디자인 시스템에서 알고 어떤 로직을 실행하는것에 컨선이 있기도 합니다.

베지어 컴포넌트와 깊은 연관성이 있지는 않아보이기도 해서, 베지어 컴포넌트는 이를 받을 수 있는 인터페이스만 열어주고, 어플리케이션 레벨에서 처리되어야 하는 일이 아닐까..? 하는 의문이 들어용! 다른 디자인 시스템은 어떻게 처리하고 있는지 궁금하네요.

Tanney-102 · 2023-01-04T00:46:35Z

@CHEWCHEWW
primer와 prose-mirror에서 enter 입력에 의한 keydown 이벤트에 한해 비슷한 처리를 해줍니다. 특히 primer는 제가 PR description에서 언급한 safari 버그에 대해 직접적으로(브라우저를 직접 확인하는 방식) 대응하고 있으며 prose-mirror에서는 compositionEnd 이벤트로부터 시간을 domain으로 하는 근방을 정의하고(500ms) 그 안에서의 keydown 이벤트를 핸들링하는 것으로 보아 같은 이슈에 대응할 목적으로 코드를 작성했다는 것을 추측해볼 수 있습니다.

이 기능을 제공하느냐는 편의 vs 유연함 사이에서의 선택이라고 생각합니다.
그리고 저는 필요한 편의라고 판단했습니다. 근거는 다음과 같습니다.

IME 상 control key 입력이 IME 바깥에서 처리될 여지가 있는 것은 분명 좋은 경험이 아닙니다. 이리한 경험을 유저에게 의도적으로 줄 이유는 없다고 생각하며, 따라서 이를 가능하게 하는 것은 불필요한 유연함이라고 생각합니다.
베지어는 오프소스이지만 그 전에 채널팀에서 사용하기 위한 디자인 시스템입니다. 채널의 서비스, 그 중에서도 데스크에서는 IME를 사용하는 언어(실제 고객들은 일본어 뿐만 아니라 베트남어 등 다양한 언어를 사용)를 지원하고, keydown or keyup 이벤트를 핸들링하는 input을 다수 가지고 있습니다. 이러한 사실로 인해 버그성 동작이 확인되었고 이를 수정한 바 있습니다. 앞으로 데스크에 추가될 컴포넌트들, 뿐만아니라 데스크 외 다른 앱에서 추가될 컴포넌트들에서 비슷한 이슈가 없을 것이라고 보장하기 어렵고 그때마다 대응하기 보다 디자인 시스템 차원에서 처리를 해주는게 맞다는 판단입니다.
레퍼런스가 적긴하지만 위에서 언급한 몇몇 라이브러리에서 비슷한 대응을 이미 해주고 있다는 것도 좋은 근거가 됩니다.

Tanney-102 · 2023-01-04T02:32:24Z

다른 라이브러리에서는 composition을 어떻게 처리하나에 대한 레퍼런스입니다.
230105 싱크 전까지 틈틈히 채워볼게요

antd
- antd에서 관리하는 react-comoponents/input에서는 따로 처리를 하지 않음 그러나 antd 레벨의 컴포넌트에서 처리
- typography > Editable
- input > Search
primer/react
- markdown editor에 관련로직 PR
prose-mirror
- input

This reverts commit 89f1883.

Tanney-102 added the enhancement Issues or PR related to making existing features better label Dec 21, 2022

Tanney-102 self-assigned this Dec 21, 2022

Tanney-102 force-pushed the features/handle-composition branch from 77201d6 to ce7498c Compare December 22, 2022 05:25

Tanney-102 added 13 commits December 22, 2022 19:51

feat(useEventLocker): implement hook that provide functions to lock e…

e54ef67

…vent handler manually

feat(useKeyboardActionLockerWhileComposing): implement hook

318595f

- this hook wrap keyboard event handlers to lock while composing

feat(TextField): wrap keyboard event handlers with composing locker

b9dc4ff

fix(TestField.test): correct fired event for lock keyboard event test

f52de23

fix(TextField.test): move tests for composing into new decribe context

cea3508

feat(TextArea): wrap keyboard event handlers with composing locker

b33855f

docs: add change set

d513c25

fix(useKeyboardActionLockerWhileComposing): merge logics from useEven…

0351092

…tLocker hook

fix(useKeyboardActionLockerWhileComposing): remove key===enter condit…

22fa9f5

…ion from safari event check - add comments about safari event check

refactor(useKeyboardActionLockerWhileComposing): replace isComposingR…

2523d76

…ef with nativeEvent.isComposing So we don't have to put composition event handlers into our inputs

fix(TextField.test): force fired key events to notice isComposing state

2886919

fix(TextArea.test): force fired key events to notice isComposing state

333b0be

fix(useKeyboardActionLockerWhileComposing): remove onKeyPress from ar…

63073db

…gument

Tanney-102 force-pushed the features/handle-composition branch from ce7498c to 63073db Compare December 22, 2022 10:51

fix: case sensitive import

593a988

Tanney-102 marked this pull request as ready for review December 22, 2022 11:02

Tanney-102 requested a review from sungik-choi as a code owner December 22, 2022 11:02

Tanney-102 requested review from quino0627 and CHEWCHEWW December 22, 2022 11:02

sungik-choi added the component label Dec 26, 2022

sungik-choi reviewed Dec 26, 2022

View reviewed changes

Tanney-102 added 3 commits January 2, 2023 11:38

docs(changeset): add details to changeset document

abc7927

test(TextArea): rewrite test case for keyboard locker in English

f3aac26

test(TextField): rewrite test case for keyboard locker

461a50a

sungik-choi approved these changes Jan 3, 2023

View reviewed changes

CHEWCHEWW reviewed Jan 4, 2023

View reviewed changes

Dogdriip self-requested a review January 5, 2023 06:39

Dogdriip approved these changes Jan 9, 2023

View reviewed changes

Tanney-102 requested a review from CHEWCHEWW January 16, 2023 06:28

sungik-choi removed the component label Jan 17, 2023

CHEWCHEWW approved these changes Jan 25, 2023

View reviewed changes

Tanney-102 merged commit 89f1883 into channel-io:next-v1 Feb 27, 2023

Tanney-102 added a commit that referenced this pull request Feb 28, 2023

Revert "Add handlers for composition on Inputs (#1082)"

9e40035

This reverts commit 89f1883.

github-actions bot mentioned this pull request Feb 28, 2023

ci(changesets): version packages (next-v1) #1142

Merged

github-actions bot mentioned this pull request May 30, 2023

ci(changesets): version packages #1369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handlers for composition on Inputs #1082

Add handlers for composition on Inputs #1082

Tanney-102 commented Dec 21, 2022 •

edited

Loading

changeset-bot bot commented Dec 21, 2022 •

edited

Loading

codecov bot commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

sungik-choi Dec 26, 2022

sungik-choi Dec 26, 2022

sungik-choi Dec 26, 2022

Tanney-102 Jan 2, 2023

CHEWCHEWW left a comment •

edited

Loading

Tanney-102 commented Jan 4, 2023

Tanney-102 commented Jan 4, 2023

Add handlers for composition on Inputs #1082

Add handlers for composition on Inputs #1082

Conversation

Tanney-102 commented Dec 21, 2022 • edited Loading

Self Checklist

Related Issue

Summary

Details

useKeyboardActionLockerWhileComposing 훅을 작성합니다.

TextField와 TextArea에 useKeyboardActionLockerWhileComposing 훅을 적용합니다.

Breaking change or not (Yes/No)

References

changeset-bot bot commented Dec 21, 2022 • edited Loading

🦋 Changeset detected

codecov bot commented Dec 22, 2022 • edited Loading

Codecov Report

github-actions bot commented Dec 22, 2022 • edited Loading

Chromatic Report

sungik-choi Dec 26, 2022

Choose a reason for hiding this comment

sungik-choi Dec 26, 2022

Choose a reason for hiding this comment

sungik-choi Dec 26, 2022

Choose a reason for hiding this comment

Tanney-102 Jan 2, 2023

Choose a reason for hiding this comment

CHEWCHEWW left a comment • edited Loading

Choose a reason for hiding this comment

Tanney-102 commented Jan 4, 2023

Tanney-102 commented Jan 4, 2023

Tanney-102 commented Dec 21, 2022 •

edited

Loading

changeset-bot bot commented Dec 21, 2022 •

edited

Loading

codecov bot commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

CHEWCHEWW left a comment •

edited

Loading