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 pasting in composition mode #599

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

QichenZhu
Copy link
Contributor

@QichenZhu QichenZhu commented Jan 16, 2025

Details

Issues

In the web example app, pasting doesn’t work after typing with Samsung keyboard.

In Expensify app, pasting works, but the caret position is wrong in the case of issue Expensify/App#40025.

Cause

When pasting, mobile browsers don’t fire the compositionend event, so the component handles the pasting event in composition mode,

if (compositionRef.current) {
updateTextColor(divRef.current, parsedText);
updateSelection(e, {
start: newCursorPosition,
end: newCursorPosition,
});
divRef.current.value = parsedText;
if (onChangeText) {
onChangeText(parsedText);
}
return;
}

and the normal process doesn’t execute.

newInputUpdate = parseText(parser, divRef.current, parsedText, processedMarkdownStyle, newCursorPosition, true, !inputType, inputType === 'pasteText');

Solution

This PR fixes the issue by detecting composition mode with the isEventComposing() function.

Related Issues

Expensify/App#40025

PROPOSAL: Expensify/App#40025 (comment)

Manual Tests

Precondition: Use a soft keyboard in composition mode, such as Samsung keyboard with autocorrection enabled.

  1. Launch the web example app in Android Chrome.
  2. Copy some text.
  3. Clear the textbox.
  4. Type "aaa".
  5. Long press the caret until the context menu pops up.
  6. Tap Paste.
  7. Verify that the copied text pastes into the textbox.
Before After
before.mp4
after.mp4

Linked PRs

@tomekzaw
Copy link
Collaborator

tomekzaw commented Jan 16, 2025

Hi @QichenZhu, thanks for the PR. I don't have much expertise in the web part of Live Markdown so I'll just pass it to someone else from the team who will review your PR shortly.

Looks like web E2E test is failing, could you please fix it as well?

@QichenZhu QichenZhu marked this pull request as ready for review January 16, 2025 11:47
@QichenZhu
Copy link
Contributor Author

Thanks for your help! I’m open to any suggestions or alternative approaches.

Web E2E testing has passed.

Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

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

Overall looks good, I've tested it against most of the similar issues we've had like voice recognition, diacritics, GBoard and such - on safari, firefox and chromium 👍🏻

I still need to test it with a Samsung phone though, ideally a physical device, so I'll be doing that tomorrow when I get a hold of a phone with Samsung keyboard.

Changing anything related to composition is tricky, since we've had some regressions related to that in the past, hence the throughout testing 😓

@rlinoz rlinoz requested a review from jjcoffee January 22, 2025 12:48
@rlinoz
Copy link

rlinoz commented Jan 22, 2025

@QichenZhu looks like there are some conflicts, can you resolve them please?

@BartoszGrajdek were you able to test on a Samsung keyboard?

Copy link

@jjcoffee jjcoffee left a comment

Choose a reason for hiding this comment

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

LGTM! Tested on a virtual Samsung device and all looked okay to me.

android-chrome-2025-01-22_16.05.24.mp4

@jjcoffee
Copy link

Changing anything related to composition is tricky, since we've had some regressions related to that in the past, hence the throughout testing

Given this comment, should we ask Applause to test once we get to the App PR? @rlinoz

BartoszGrajdek
BartoszGrajdek previously approved these changes Jan 22, 2025
Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot about this issue due to some higher priority tasks 😓

Since @jjcoffee tested it on Samsung keyboard LGTM!

cc @tomekzaw can we merge this, or is there anything blocking us?

tomekzaw
tomekzaw previously approved these changes Jan 22, 2025
@BartoszGrajdek
Copy link
Collaborator

Given this comment, should we ask Applause to test once we get to the App PR? @rlinoz

You could actually create a build file with npm run prepare and npm pack, add it into E/App directory and update package.json to get react-native-live-markdown from that build tgz. That way you can test it before we push this to the main branch. WDYT? @tomekzaw @jjcoffee

@tomekzaw tomekzaw dismissed stale reviews from BartoszGrajdek and themself via 7870627 January 22, 2025 15:52
@tomekzaw tomekzaw merged commit ba3c4f8 into Expensify:main Jan 22, 2025
5 checks passed
@os-botify
Copy link
Contributor

os-botify bot commented Jan 22, 2025

🚀 Published to npm in 0.1.223 🎉

@tomekzaw
Copy link
Collaborator

I've just noticed @BartoszGrajdek's comment after I merged this PR. I think it should be safe to update in E/App. If anything goes wrong, we can use patch-package or just prepare another follow-up PR.

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.

5 participants