[HOLD 2023-10-16] [$125] mweb/Safari - Composer - Cursor does not moved to a new line in the composer when tapped on the "return"

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue found when executing PR #27490

Action Performed:

1. Open https://staging.new.expensify.com/
2. Tap on any listed conversation in LHN
3. Tap on the composer box
4. Write any words
5. Tap on the "return" button on keayboard

Expected Result:

Cursor should be moved to a new line

Actual Result:

Nothing happened. The cursor remains on the same line

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.75-0

Reproducible in staging?: Yes

Reproducible in production?: No

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6219214_1696001683570.RPReplay_Final1696001647.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a307034471edf3f5
• Upwork Job ID: 1713885684306395136
• Last Price Increase: 2023-10-16

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[lanitochka17]

The issue is not reproducible with a 1.3.75.3 build

RPReplay_Final1696009728.mp4

[chiragsalian]

i cannot reproduce either, closing issue.

[bernhardoj]

@lanitochka17 @chiragsalian I can still reproduce it on the latest main.

Screen.Recording.2023-09-30.at.18.09.29.mov

[abdulrahuman5196]

@chiragsalian / @trjExpensify I am able to reproduce the issue in staging itself. Could you kindly check and re-open this issue?

[mountiny]

Still repro on the latest staging
https://github.com/Expensify/App/assets/36083550/016386c5-15a7-4b85-8e36-c380dded46fa

[mountiny]

@abdulrahuman5196 @bernhardoj have you got fixes on tour mind? Or offending pr?

[bernhardoj]

I switched to the commit before this PR and the issue is gone. I don't know how it's related though.

[bernhardoj]

Proposal

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

Pressing Enter doesn't add new lines on the mobile web.

What is the root cause of that problem?

TL;DR
The enter key is consumed by BaseReportActionContextMenu keyboard listener.

App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

Lines 111 to 127 in dee1632

	useKeyboardShortcut(
	CONST.KEYBOARD_SHORTCUTS.ENTER,
	(event) => {
	if (!menuItemRefs.current[focusedIndex]) {
	return;
	}
	// Ensures the event does not cause side-effects beyond the context menu, e.g. when an outside element is focused
	if (event) {
	event.stopPropagation();
	}
	menuItemRefs.current[focusedIndex].triggerPressAndUpdateSuccess();
	setFocusedIndex(-1);
	},
	{isActive: shouldEnableArrowNavigation},
	);

Details:
The enter keyboard listener is subscribed if BaseReportActionContextMenu is visible and not mini. The not mini usage of BaseReportActionContextMenu is on PopoverReportActionContextMenu.

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

Lines 288 to 317 in dee1632

	<PopoverWithMeasuredContent
	isVisible={isPopoverVisible}
	onClose={hideContextMenu}
	onModalShow={runAndResetOnPopoverShow}
	onModalHide={runAndResetOnPopoverHide}
	anchorPosition={popoverAnchorPosition.current}
	animationIn="fadeIn"
	disableAnimation={false}
	animationOutTiming={1}
	shouldSetModalVisibility={false}
	fullscreen
	withoutOverlay
	anchorRef={anchorRef}
	>
	<BaseReportActionContextMenu
	isVisible
	type={typeRef.current}
	reportID={reportIDRef.current}
	reportActionID={reportActionIDRef.current}
	draftMessage={reportActionDraftMessageRef.current}
	selection={selectionRef.current}
	isArchivedRoom={isRoomArchived}
	isChronosReport={isChronosReportEnabled}
	isPinnedChat={isChatPinned}
	isUnreadChat={hasUnreadMessages}
	anchor={contextMenuTargetNode}
	contentRef={contentRef}
	originalReportID={originalReportIDRef.current}
	/>
	</PopoverWithMeasuredContent>

Notice that isVisible is always true for BaseReportActionContextMenu. But that should not be the problem because we wrap it inside PopoverWithMeasuredContent which will not render the children if isVisible is false. Before this PR, when PopoverWithMeasuredContent isVisible is false, BaseReportActionContextMenu is hidden (not rendered). But that's not the case now. Now, BaseReportActionContextMenu is always rendered (but not visible to the user), thus the enter keyboard listener is still active.

That is because initially PopoverWithMeasuredContent will render the children (by wrapping it in a View) to measure the size.

App/src/components/PopoverWithMeasuredContent.js

Lines 136 to 157 in dee1632

	return isContentMeasured ? (
	<Popover
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...props}
	anchorPosition={shiftedAnchorPosition}
	>
	<View onLayout={measurePopover}>{props.children}</View>
	</Popover>
	) : (
	/*
	This is an invisible view used to measure the size of the popover,
	before it ever needs to be displayed.
	We do this because we need to know its dimensions in order to correctly animate the popover,
	but we can't measure its dimensions without first rendering it.
	*/
	<View
	style={styles.invisiblePopover}
	onLayout={measurePopover}
	>
	{props.children}
	</View>
	);

After the measurement is done (onLayout is called), PopoverWithMeasuredContent will wrap the children with Popover. However, onLayout is never called. I found that onLayout won't be called if the children don't have a size.

In our case BaseReportActionContextMenu is the children and it renders context menu items from ContextMenuActions that are filtered by each context menu shouldShow.

App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

Lines 64 to 79 in dee1632

	const shouldShowFilter = (contextAction) =>
	contextAction.shouldShow(
	props.type,
	reportAction,
	props.isArchivedRoom,
	props.betas,
	props.anchor,
	props.isChronosReport,
	props.reportID,
	props.isPinnedChat,
	props.isUnreadChat,
	isOffline,
	);
	const shouldEnableArrowNavigation = !props.isMini && (props.isVisible || shouldKeepOpen);
	const filteredContextMenuActions = _.filter(ContextMenuActions, shouldShowFilter);

But the problem is, all context menu is filtered out and that is because when PopoverReportActionContextMenu is not visible, props.type is an empty string

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

Line 15 in dee1632

const typeRef = useRef('');

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

Lines 302 to 304 in dee1632

	<BaseReportActionContextMenu
	isVisible
	type={typeRef.current}

and most of the context menu items rely on type, for example.

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Line 48 in dee1632

shouldShow: (type, reportAction) => type === CONTEXT_MENU_TYPES.REPORT_ACTION && _.has(reportAction, 'message') && !ReportActionsUtils.isMessageDeleted(reportAction),

Previously, props.type was undefined so it uses the default value which is CONTEXT_MENU_TYPES.REPORT_ACTION and the filtering gives us 2 items, copy to clipboard, and mark as unread.

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

The simplest solution is to set the initial value of type back to undefined.

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

Line 15 in dee1632

const typeRef = useRef('');

What alternative solutions did you explore? (Optional)

Honestly, it feels wrong to have 2 context menu items rendered by default when we are not opening any context menu. So, I think having an empty type is better and it's good to not render the invisible view of PopoverWithMeasuredContent if isVisible is false.

App/src/components/PopoverWithMeasuredContent.js

Lines 144 to 157 in 0c866f4

	) : (
	/*
	This is an invisible view used to measure the size of the popover,
	before it ever needs to be displayed.
	We do this because we need to know its dimensions in order to correctly animate the popover,
	but we can't measure its dimensions without first rendering it.
	*/
	<View
	style={styles.invisiblePopover}
	onLayout={measurePopover}
	>
	{props.children}
	</View>
	);

: props.isVisible && (invisibleView)

OR

{props.isVisible && props.children}

[abdulrahuman5196]

@bernhardoj

Your proposal here works fine. But I don't think it is the proper fix.

The simplest solution is to set the initial value of type back to undefined.

Same for the alternative solution as well.

: props.isVisible && (invisibleView)

We shouldn't even come to this place(children rendering) if the popover is not rendered.

AFAIK, The popover layout will only happen if user clicks to show popover. In First layout, an invisible view will be rendered to calculate size, followed by proper renders of popovers.
If as you the BaseReportActionContextMenu is swallowing the enter key newly, we should find why and fix the same?

I know you had tried to explain the same here. But could point to how this happens? I am not seeing any rendering logic changes based on isVisible param in the PR.

Before this #27221, when PopoverWithMeasuredContent isVisible is false, BaseReportActionContextMenu is hidden (not rendered). But that's not the case now.

Do Let me know if I am missing something.

[bernhardoj]

The popover layout will only happen if user clicks to show popover. In First layout, an invisible view will be rendered to calculate size, followed by proper renders of popovers.

This process is done even without clicking the button to show the popover. My alternative solution won't allow the measurement to happen if it's not visible.

I know you had tried to explain the same here. But could point to how this happens? I am not seeing any rendering logic changes based on isVisible param in the PR.

The type initial value change from the PR that causes the issue. The empty type making the measurement never completes.

[OlimpiaZurek]

Checked changes from this PR and have the same finding as in the proposal Setting the type as string instead of undefined causes this issue, so to fix is we should set the initial value of type back to undefined.

[mountiny]

Thanks @bernhardoj I have asked @OlimpiaZurek as a PR author to raise a fix PR but given we will use your solution, we will reward you 25% of standard bounty so $125

[bernhardoj]

@mountiny thanks!

[trjExpensify]

Assigning @bernhardoj to the issue to keep track of that for when this is ready for payment.

[melvin-bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[mountiny]

Assigning myself for mnagement and handling the PR review

[mountiny]

This is fixed in staging, removing blocker label

[melvin-bot]

@trjExpensify, @mountiny, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

What's the dealio here? We're ready to pay @bernhardoj $125 for this?

[mountiny]

Yep I think we are good to pay, the checklist is not needed I think, it was already clearly identified what pr was this regression from and the author has addressed it using fix provided by @bernhardoj

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a307034471edf3f5

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @situchan (Internal)

[trjExpensify]

(needed a job created in Upwork, removing @situchan).

[trjExpensify]

@bernhardoj sent you an offer for $125!

[bernhardoj]

Thanks! Accepted. @trjExpensify

[trjExpensify]

Paid!