[$250] mWeb - Chat - Before emoji picker box opens, a seperate compose box with cursor is shown briefly

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.63
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4499507
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Tap on a report
3. Tap emoji picker in compose box
4. Select an emoji
5. Note size of emoji in compose box is bigger compare to production
6. Select one more emoji
7. Send the message
8. Long press the sent message amd tap edit comment
9. Tap emoji picker from edit compose box

Expected Result:

Before emoji picker box opens, a separate compose box with cursor must not be shown briefly while edit comment

Actual Result:

Before emoji picker box opens, a separate compose box with cursor is shown briefly while edit comment

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6453358_1713428958074.edit.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01030e7a0cab136f03
• Upwork Job ID: 1782524877291671552
• Last Price Increase: 2024-04-22
• Automatic offers:
• s77rt | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The main composer is shown when opening the edit composer emoji picker.

What is the root cause of that problem?

When we focus on an edit composer, the main composer will be hidden. But when the edit composer is blurred, the main composer will shown again. In this case, when we press the emoji picker button, the edit composer will be blurred.

But actually, we already prevent the main composer from showing when we press the emoji picker button as shown here.

App/src/pages/home/report/ReportActionItemMessageEdit.tsx

Lines 449 to 457 in 305f12c

	onBlur={(event: NativeSyntheticEvent<TextInputFocusEventData>) => {
	setIsFocused(false);
	// @ts-expect-error TODO: TextInputFocusEventData doesn't contain relatedTarget.
	const relatedTargetId = event.nativeEvent?.relatedTarget?.id;
	if (relatedTargetId && [messageEditInput, emojiButtonID].includes(relatedTargetId)) {
	return;
	}
	setShouldShowComposeInputKeyboardAware(true);
	}}

The way it works is, that if the pressed element is an emoji picker button ID, then don't show the main composer yet. However, the emoji picker button element doesn't have the ID set.

We already pass the ID here,

App/src/pages/home/report/ReportActionItemMessageEdit.tsx

Line 469 in 305f12c

id={emojiButtonID}

However, the pressable component accepts nativeID prop instead of id.

App/src/components/Pressable/GenericPressable/index.tsx

Line 19 in 305f12c

id={props.nativeID}

App/src/components/Pressable/GenericPressable/index.tsx

Line 26 in 305f12c

nativeID={props.nativeID}

What changes do you think we should make in order to solve the problem?

Rename the props from id to nativeID specifically in EmojiPickerButton (or we can rename the pressable props to id, but we need to rename it for many components too).

we may need to check for messageEditInput ID too

Note: in native, the main composer will still appear (even though covered by the modal) because it's blurred when the modal shows. To further improve it, we can ignore the blur when the emoji picker modal is active.

if (relatedTargetId && [messageEditInput, emojiButtonID].includes(relatedTargetId) || EmojiPickerAction.isActive(action.reportActionID)) {
    return;
}

But there are currently 2 issues with EmojiPickerAction.isActive. EmojiPickerAction.isActive compares the active ID and the passed ID.

App/src/components/EmojiPicker/EmojiPicker.tsx

Line 150 in 305f12c

const isActive = (id: string) => !!id && id === activeID;

The active ID is set after calculating the anchor position, but we need to set it immediately when calling showEmojiPicker.

App/src/components/EmojiPicker/EmojiPicker.tsx

Lines 88 to 100 in 305f12c

	calculateAnchorPosition(emojiPopoverAnchor?.current, anchorOriginValue).then((value) => {
	onWillShow?.();
	setIsEmojiPickerVisible(true);
	setEmojiPopoverAnchorPosition({
	horizontal: value.horizontal,
	vertical: value.vertical,
	});
	emojiAnchorDimension.current = {
	width: value.width,
	height: value.height,
	};
	setEmojiPopoverAnchorOrigin(anchorOriginValue);
	setActiveID(id);

Second, we never clear the active ID when closing the emoji picker, so we need to do that.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01030e7a0cab136f03

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[puneetlath]

@s77rt FYI there is already a proposal above.

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct. This looks to be a regression from 822efb5. Regarding the web solution, we should use id since nativeID is deprecated. Regarding native, did you try clearing the active action? I remember we had some tricky bugs on that area and a lengthy issue #23908

[bernhardoj]

Yes, I tried it. The isActive issue of emoji picker is also mentioned in the proposal there

but instead of clearing it when other edit message is focused, we should clear it when the emoji picker hides.

btw, the 3rd case in the linked issue happens on main

[s77rt]

@bernhardoj

but instead of clearing it when other edit message is focused, we should clear it when the emoji picker hides

That was the first attempt but didn't work well when a message is deleted from another device while the emoji picker for that message is still open. Let's test that scenario as well as other cases where EmojiPickerAction.isActive is used.

🎀 👀 🎀 C+ reviewed
Link to proposal

[s77rt]

Not overdue. @puneetlath ^

[melvin-bot]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @s77rt

[melvin-bot]

This issue has not been updated in over 15 days. @puneetlath, @s77rt, @bernhardoj eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[s77rt]

PR was deployed to production 2 weeks ago

[puneetlath]

Whoops, right you are. Can you complete the checklist @s77rt?

[puneetlath]

@bernhardoj has been paid.

@s77rt payment just pending checklist above.

[s77rt]

• The PR that introduced the bug has been identified: Upgrade to react-native-web v0.19.9 #24482
• The offending PR has been commented on: 822efb5#r142442593
• A discussion in #expensify-bugs has been started: I don't feel we have much to discuss, the bug was caused by conflicts
• Determine if we should create a regression test for this bug: No

[s77rt]

@puneetlath Done!

[puneetlath]

kk all paid. Thanks everyone and sorry for the delay!