Fix broken global create emoji - #69411
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| * Calls the onChangeText callback with a modified room name | ||
| */ | ||
| const setModifiedRoomName = (event: NativeSyntheticEvent<TextInputChangeEventData>) => { | ||
| const roomName = event.nativeEvent.text; |
There was a problem hiding this comment.
@ishpaul777 The event from live markdown doesn't have text, so I need to use onChangeText instead. This makes me think that we might get some uncaught errors when replacing all text input with live markdown input, since there are a lot of text input in our app.
There was a problem hiding this comment.
@bernhardoj thanks for bringing this up, your concern is valid. I think the best step would be to carefully audit each usage, manually test any special cases (like amount input or magic code), and add automated UI tests wherever possible.
There was a problem hiding this comment.
I found another crash when trying to input the password for a PDF-protected form. This makes me think that we should probably find a more reliable approach. Currently, the global create emoji Unicode is only recognized by our custom font file (CustomEmojiWebFont.ttf).
I'm thinking that maybe it's best to somehow edit our current Expensify font (ExpensifyMono-, ExpensifyNeue-, ExpensifyNewKansas-*) to include the global create emoji unicode. This way, we don't need CustomEmojiWebFont.ttf anymore. But:
- I haven't checked how to do it yet
- The font is created/updated by a 3rd-party, so every time the font is updated, we will need to re-add the global create emoji unicode to the font file.
There was a problem hiding this comment.
@bernhardoj If we can get support for custom emojis added upstream by the 3rd-party font maintainer, that would be ideal. I’d lean against maintaining a separate patched version ourselves since it could become error-prone and difficult to keep in sync with updates.
There was a problem hiding this comment.
@bernhardoj if you'd like to pivot to this solution, lets first bring it to the slack and involve design team as well
There was a problem hiding this comment.
There was a problem hiding this comment.
@ishpaul777 based on the disc, should we just handle the non-text input case?
There was a problem hiding this comment.
that sounds good @bernhardoj, please let me know when its ready
|
gentle bump @bernhardoj 👋 |
|
@bernhardoj its still broken on android for menuitem, can you please check? Screen.Recording.2025-09-15.at.11.16.58.PM.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-09-15.at.11.30.08.PM.movAndroid: mWeb ChromeScreen.Recording.2025-09-15.at.11.16.58.PM.moviOS: HybridAppScreen.Recording.2025-09-15.at.10.41.10.PM.moviOS: mWeb SafariScreen.Recording.2025-09-15.at.10.27.32.PM.movMacOS: Chrome / SafariScreen.Recording.2025-09-15.at.10.13.58.PM.movMacOS: DesktopScreen.Recording.2025-09-15.at.11.23.00.PM.mov |
|
Checking |
| const isOnlyCustomEmoji = useMemo(() => { | ||
| if (typeof children === 'string') { | ||
| return containsOnlyCustomEmoji(children); | ||
| return containsOnlyCustomEmoji(children.replace(CONST.UNICODE.LTR, '')); |
There was a problem hiding this comment.
Fixed. MenuItem prepend LTR unicode on Android only, so we need to clean it here before checking. (we still render the whole text with the LTR unicode)
App/src/components/MenuItem.tsx
Line 617 in ae3b077
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.2.16-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.2.16-14 🚀
|
Explanation of Change
Fixed Issues
$ #67976
PROPOSAL: #67976 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Precondition: copy the global create emoji
3. Verify the global create emoji shows correctly on the merchant inputPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop