[$250] Android - Chat - Unable to close keyboard by tapping outside of it

[IuliiaHerets]

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

---

Version Number: 9.0.67-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

1. Open the Expensify app.
2. Open any chat that has messages on it.
3. Tap on compose box to open keyboard.
4. Tap outside the keyboard area to close it.
5. Verify that the keyboard is closed when tapping outside of it.

Expected Result:

Keyboard should be closed when tapping outside of it.

Actual Result:

Keyboard is not closed when tapping outside of it in chats that already have messages. Keyboard closes normally when the chat doesn´t have any message or content on it.

Workaround:

Unknown

Platforms:

• [x] Android: Standalone
• [x] Android: HybridApp
• [ ] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/89e50c5f-3001-4236-92ce-78327b3313b2

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021861915865037036576
• Upwork Job ID: 1861915865037036576
• Last Price Increase: 2024-11-27
• Automatic offers:
• daledah | Contributor | 105283329

Issue Owner

Current Issue Owner: @alitoshmatov

[melvin-bot[bot]]

Triggered auto assignment to @mallenexpensify (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.

[gadhiyamanan]

Proposal

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

keyboard does not close on Tap outside the keyboard

What is the root cause of that problem?

we are using keyboardShouldPersistTaps="handled" props in ReportActionsList.tsx because of that keyboard will not dismiss automatically on Tap outside the keyboard

https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/pages/home/report/ReportActionsList.tsx#L732-L756

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

we can use keyboardShouldPersistTaps="never" in ReportActionsList.tsx or remove keyboardShouldPersistTaps prop because by default the value of keyboardShouldPersistTaps is never

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[mallenexpensify]

👋 @gadhiyamanan , it's been a minute!
@alitoshmatov , plz attempt reproduction. If you're able to repro, please review Manan's proposal. I won't be able to test on Android til I'm back in the bank on Monday.

[huult]

Edited by proposal-police: This proposal was edited at 2024-11-28 04:11:57 UTC.

Proposal

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

Unable to close keyboard by tapping outside of it

What is the root cause of that problem?

We are using keyboardShouldPersistTaps with the value handled, but with this setup, we are unable to hide the keyboard when tapping outside

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionsList.tsx#L748

'handled', the keyboard will not dismiss automatically when the tap was handled by a children, (or captured by an ancestor). false, deprecated, use 'never' instead true, deprecated, use 'always' instead

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

To resolve this issue and avoid impacting the current logic and behavior, we should handle the onPress event of ReportActionItem to dismiss the keyboard. Something like this:

https://github.com/Expensify/App/blob/0b97bdef35a43e7fb39e612b90abd53be6b6a81a/src/pages/home/report/ReportActionsListItemRenderer.tsx#L162

// src/pages/home/report/ReportActionsListItemRenderer.tsx#L162
    <ReportActionItem
        shouldHideThreadDividerLine={shouldHideThreadDividerLine}
        parentReportAction={parentReportAction}
        report={report}
        transactionThreadReport={transactionThreadReport}
        parentReportActionForTransactionThread={parentReportActionForTransactionThread}
        action={action}
        reportActions={reportActions}
        linkedReportActionID={linkedReportActionID}
        displayAsGroup={displayAsGroup}
        shouldDisplayNewMarker={shouldDisplayNewMarker}
        shouldShowSubscriptAvatar={
            (ReportUtils.isPolicyExpenseChat(report) || ReportUtils.isInvoiceRoom(report)) &&
            [
                CONST.REPORT.ACTIONS.TYPE.IOU,
                CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW,
                CONST.REPORT.ACTIONS.TYPE.SUBMITTED,
                CONST.REPORT.ACTIONS.TYPE.APPROVED,
                CONST.REPORT.ACTIONS.TYPE.FORWARDED,
            ].some((type) => type === reportAction.actionName)
        }
        isMostRecentIOUReportAction={reportAction.reportActionID === mostRecentIOUReportActionID}
        index={index}
        isFirstVisibleReportAction={isFirstVisibleReportAction}
        shouldUseThreadDividerLine={shouldUseThreadDividerLine}
+        onPress={() => {
+            Keyboard.dismiss();
+        }}
    />

POC

https://github.com/user-attachments/assets/86dccf7c-6878-4f23-a8ad-2af46aa02d88

Note: For similar cases like this issue, we can reuse this solution

What alternative solutions did you explore? (Optional)

[daledah]

Edited by proposal-police: This proposal was edited at 2024-11-29 07:35:59 UTC.

Proposal

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

Keyboard is not closed when tapping outside of it in chats that already have messages. Keyboard closes normally when the chat doesn´t have any message or content on it.

What is the root cause of that problem?

• We are using:

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionsList.tsx#L748

'handled': the keyboard will not dismiss automatically when the tap was handled by children of the scroll view (or captured by an ancestor).

That means the keyboard will not be dismissed if the tap is handled by the children.

• The state mentioned in OP "Keyboard is not closed when tapping outside of it in chats that already have messages" is not fully correct. Given the below image:

The keyboard is still closed if we click on red area. When clicking on the white area, the keyboard is not closed. It is because the red area is just a normal View component:

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionItemCreated.tsx#L54

so it cannot handle the tap. So the keyboard is closed.

Otherwise, with the white area, it is the Pressable component:

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionItem.tsx#L943

so it can handle the tap. Based on the docs, the keyboard will not dismiss automatically when the tap was handled by children.

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

• We can use onStartShouldSetResponder in here:

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionItem.tsx#L961

                    <View
                        onStartShouldSetResponder={() => {
                            Keyboard.dismiss();
                            return false;
                        }}
                        style={highlightedBackgroundColorIfNeeded}
                    >

or

                    <View
                        onStartShouldSetResponder={() => {
                            setTimeout(() => {
                                Keyboard.dismiss();
                            }, 500); // 500ms to recognize the long press
                            return false;
                        }}
                        style={highlightedBackgroundColorIfNeeded}
                    >

• Explanation: Based on the docs:

onStartShouldSetResponder are called with a bubbling pattern, where the deepest node is called first. That means that the deepest component will become responder when multiple Views return true for *ShouldSetResponder handlers. This is desirable in most cases, because it makes sure all controls and buttons are usable.

This means that if the deepest component in:

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionItem.tsx#L943

is an interactive component (such as Button, Pressable, etc.), the onStartShouldSetResponder will not be called. However, if the deepest component is a non-interactive component (like View, Text, etc.), the onStartShouldSetResponder will be triggered, in other words, we call Keyboard.dismiss() in this case.

What alternative solutions did you explore? (Optional)

• We need to manually close the keyboard when user clicks on white area. To do it, we can update:

https://github.com/Expensify/App/blob/9909d61dd08f52066c9f3d288b9582a12d724813/src/pages/home/report/ReportActionItem.tsx#L945

            onPress={
                draftMessage === undefined
                    ? () => {
                          onPress?.();
                          Keyboard.dismiss();
                      }
                    : undefined
            }

or

            onPress={
                draftMessage === undefined
                    ? () => {
                          onPress?.();
                          Keyboard.dismiss();
                      }
                    : () => {
                          Keyboard.dismiss();
                      }
            }

• Before calling Keyboard.dismiss(), we can check whether the keyboard is opening or not. If so, call Keyboard.dismiss().

[ikevin127]

Edited by proposal-police: This proposal was edited at 2024-12-02 12:26:18 UTC.

Proposal

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

In chat reports that have messages, the keyboard is not closed when tapping outside of it on any of the messages.

What is the root cause of that problem?

The keyboardShouldPersistTaps="handled" prop is passed to the InvertedFlatList here (has been there for more than 18 months at this point), handled meaning:

handled: the keyboard will not dismiss automatically when the tap was handled by a children, (or captured by an ancestor).

in our case, the chat report message being a PressableWithSecondaryInteraction which already handles tap (onPress), the keyboard won't close when tapped on a message (onPress).

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

Given that keyboardShouldPersistTaps="handled" has been there for over 18 months, I don't think we should remove or change it because that would also remove the onLongPress behaviour of the message which opens the context menu (even with the keyboard open) and we would have to close the keyboard first then long press on a message in order to open the context menu (which is bad UX).

The ideal solution here would have to fulfil both:

• keep the current onLongPress message functionality (even with the keyboard open)
• dismiss the keyboard when simple tap (onPress) is performed on a message while the keyboard is opened

Therefore I'm coming forward with the following solution which allows to dynamically set the required behaviour based on a prop that stands at the root of our BaseGenericPressable component and can be easily passed only where we need it, in our ReportActionItem component to PressableWithSecondaryInteraction.

Implementation

1. In our BaseGenericPressable.tsx component add a new prop shouldDismissKeyboardOnPress (boolean) defaulting to false (make sure to add the type PressableProps).
2. In the same component, within the the onPressHandler function, before return onPress(event); add:

if (shouldDismissKeyboardOnPress && Keyboard.isVisible()) {
    Keyboard.dismiss();
}

leveraging our newly added prop and react-native's Keyboard to ensure we only dismiss the keyboard when required by the new prop and when the keyboard is actually visible.

3. In ReportActionItem.tsx pass the newly added shouldDismissKeyboardOnPress prop to PressableWithSecondaryInteraction as true (no need to add ={true}).

And we're done, this ensures we can dynamically add the dismiss functionality for the onPress using the shouldDismissKeyboardOnPress to elegantly get around the list's keyboardShouldPersistTaps="handled" prop and fulfil the issue's Expected result while providing a way for developers to use the same prop should similar issues arise in the future.

Here's a diff of all the changes for convenience:

DIFF

diff --git a/src/components/Pressable/GenericPressable/implementation/BaseGenericPressable.tsx b/src/components/Pressable/GenericPressable/implementation/BaseGenericPressable.tsx
index 1765560eaae..9bdeb82fc6a 100644
--- a/src/components/Pressable/GenericPressable/implementation/BaseGenericPressable.tsx
+++ b/src/components/Pressable/GenericPressable/implementation/BaseGenericPressable.tsx
@@ -2,7 +2,7 @@ import type {ForwardedRef} from 'react';
 import React, {forwardRef, useCallback, useEffect, useMemo, useState} from 'react';
 import type {GestureResponderEvent, View} from 'react-native';
 // eslint-disable-next-line no-restricted-imports
-import {Pressable} from 'react-native';
+import {Keyboard, Pressable} from 'react-native';
 import type {PressableRef} from '@components/Pressable/GenericPressable/types';
 import type PressableProps from '@components/Pressable/GenericPressable/types';
 import useSingleExecution from '@hooks/useSingleExecution';
@@ -37,6 +37,7 @@ function GenericPressable(
         accessible = true,
         fullDisabled = false,
         interactive = true,
+        shouldDismissKeyboardOnPress = false,
         ...rest
     }: PressableProps,
     ref: PressableRef,
@@ -115,6 +116,9 @@ function GenericPressable(
                 ref.current?.blur();
                 Accessibility.moveAccessibilityFocus(nextFocusRef);
             }
+            if (shouldDismissKeyboardOnPress && Keyboard.isVisible()) {
+                Keyboard.dismiss();
+            }
             return onPress(event);
         },
         [shouldUseHapticsOnPress, onPress, nextFocusRef, ref, isDisabled, interactive],
diff --git a/src/components/Pressable/GenericPressable/types.ts b/src/components/Pressable/GenericPressable/types.ts
index 61cb6db8ee7..d8f38a44fda 100644
--- a/src/components/Pressable/GenericPressable/types.ts
+++ b/src/components/Pressable/GenericPressable/types.ts
@@ -148,6 +148,11 @@ type PressableProps = RNPressableProps &
          * e.g., show disabled cursor when disabled
          */
         interactive?: boolean;
+
+        /** Used for when a list has keyboardShouldPersistTaps set to `handled`
+         * and we need the keyboard to be dismissed on onPress.
+         */
+        shouldDismissKeyboardOnPress?: boolean;
     };
 
 type PressableRef = ForwardedRef<HTMLDivElement | View | RNText | undefined>;
diff --git a/src/pages/home/report/ReportActionItem.tsx b/src/pages/home/report/ReportActionItem.tsx
index 399550069c0..963b2787158 100644
--- a/src/pages/home/report/ReportActionItem.tsx
+++ b/src/pages/home/report/ReportActionItem.tsx
@@ -968,6 +968,7 @@ function ReportActionItem({
             withoutFocusOnSecondaryInteraction
             accessibilityLabel={translate('accessibilityHints.chatMessage')}
             accessible
+            shouldDismissKeyboardOnPress
         >
             <Hoverable
                 shouldHandleScroll

What alternative solutions did you explore?

Alternatively, skip the addition of the shouldDismissKeyboardOnPress prop and simply add the keyboard dismiss logic in the same place as stated in the main solution.

Result video (before / after)

Android: Native (One Plus 7T - Real device)

Before	After

[daledah]

Proposal updated

[alitoshmatov]

@gadhiyamanan Thank you for your proposal, keyboardShouldPersistTaps="handled" is there for a reason, as other proposals showed removing it will affect onLongPress behavior

[alitoshmatov]

@huult Thank you for your proposal, your solution is solving the issue

[alitoshmatov]

@daledah Thank you for detailed RCA. I think onStartShouldSetResponder is overkill here and we could just use onPress instead

[alitoshmatov]

@ikevin127 Thank you for your proposal, While your solution does solve the issue I don't think we need to introduce a new attribute here as I don't think there is a much use-case for this. Even if we come across similar problem, then we can refactor code to implement similar attribute. For now, I think we can just use onPress

[alitoshmatov]

We can go with @huult 's proposal which proposes simple solution and does not affect long press behavior

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[daledah]

@alitoshmatov

• ~The selected solution can change the original behavior (can be considered as regression) as I explained in my alternative solution~:

This solution ensures that the app's behavior remains consistent for the TaskPreview component. In the case of the TaskPreview component, clicking on the checkbox does not dismiss the keyboard (Behavior in latest main). However, in my alternative solution below, clicking on the checkbox will dismiss the keyboard (Not the behavior in latest main).

And it does not fully fix the issue as well since we are using the ReportActionItem in a lot of places, not only one place.

• I don't think onStartShouldSetResponder is overkill here. It is only the one line change and we already the docs for it.

cc @AndrewGable

[alitoshmatov]

The https://github.com/Expensify/App/issues/53196#issuecomment-2505226545 can change the original behavior (can be considered as regression) as I explained in my https://github.com/Expensify/App/issues/53196#issuecomment-2507231123

@daledah But chosen proposal is not causing the regression you mentioned

https://github.com/user-attachments/assets/38a51090-58d2-4f2e-931c-6faf09c16fff

[ikevin127]

Thanks for the review!

I don't think we need to introduce a new attribute here as I don't think there is a much use-case for this. Even if we come across similar problem, then we can refactor code to implement similar attribute.

I'm curious to hear @AndrewGable's take on this as well since IMO we should implement new solutions with foresight in mind instead of refactoring later.

[!caution] Besides the question posed above, the selected solution only mentioned adding onPress to the ReportActionItem within ReportActionsListItemRenderer without considering ReportActionItemParentAction.

@alitoshmatov You might want to take a second look at this and reconsider my solution for a complete fix which would not require adding onPress multiple times in separate components - for this I added an alternative to my proposal where we don't introduce a new prop 🙂

Updated proposal

• added alternative to not introduce new prop

[daledah]

But chosen proposal is not causing the regression you mentioned

@alitoshmatov Sorry, I cannot reproduce the bug when applying the selected proposal anymore. Maybe I am adding something when apply that solution leads to the regression is encountered from my side.

And it does not fully fix the issue as well since we are using the ReportActionItem in a lot of places, not only one place.

What do you think about this? Such as in here, if ReportUtils.canCurrentUserOpenReportis false, so the onPress function is undefined, the bug is not fixed.

[alitoshmatov]

@ikevin127 I don't think closing keyboard on every onPress of BaseGenericPressable is a good idea, I assume it is pretty big change across the app, it might introduce new regressions as we can't test all the instance of its usage. I think we should focus on current issue alone

[alitoshmatov]

What do you think about this? Such as in here, if ReportUtils.canCurrentUserOpenReportis false, so the onPress function is undefined, the bug is not fixed.

You are right, I missed this one

[ikevin127]

Well, that's what the new prop would have been used for - to control when and where exactly we want to apply the fix without having to add onPress in multiple components.

That solution was designed with the DRY principle in mind, but please move forward however you consider best 👍

[huult]

@alitoshmatov

You are right, I missed this one

Did you manage to reproduce the case where onPress is undefined?

[alitoshmatov]

Did you manage to reproduce the case where onPress is undefined?

No, it seems hard to get to reproduce that case.

[huult]

No, it seems hard to get to reproduce that case.

@alitoshmatov Yes, I will test more cases and note that case. If I am able to reproduce it, I will fix it or add a check in case onPress is undefined. Do you agree?

[alitoshmatov]

Yes that is definitely a valid concern. Whether it is reproducible or not, it is still something to cover in the solution.

[alitoshmatov]

@daledah I still think we should apply onPress rather than onStartShouldSetResponder here. That is why I think we should go with @huult 's solution. I just wanted to make sure everyone is happy with this decision, if not we should consult @AndrewGable to make a final decision. @daledah What do you think?

[daledah]

I still think we should apply onPress rather than onStartShouldSetResponder here. That is why I think we should go with @huult 's solution.

@alitoshmatov My alternative solution works well and can make sure we do not miss any case such as this one or when draftMessage !== undefined in here. What do you think?

About the case draftmessage !== undefined, clicking on the area outside the input box (green area in the below image) should close the keyboard:

[alitoshmatov]

Note: This solution ensures that the app's behavior remains consistent for the TaskPreview component. In the case of the TaskPreview component, clicking on the checkbox does not dismiss the keyboard (Behavior in latest main). However, in my alternative solution below, clicking on the checkbox will dismiss the keyboard (Not the behavior in latest main).

@daledah But this introduces this regression, right?

[huult]

Whatever the outcome, I respect your choice, @alitoshmatov.

The solution for this ticket is to use onPress to trigger the call to dismiss the keyboard.

If we choose this solution, I believe my proposal should be selected because it was submitted first and provides the correct solution.

Regarding “onPress being undefined,” we can check this in the pull request phase because the proposal includes a code example to describe the solution I mentioned.

The contribution guide states that all new proposals must be different from existing proposals.