[$250] Android - Chat - Unable to close keyboard by tapping outside of it #53196
Comments
IuliiaHerets
added
Daily
|
Triggered auto assignment to @mallenexpensify ( |
ProposalPlease 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 App/src/pages/home/report/ReportActionsList.tsx Lines 732 to 756 in c62ab78 What changes do you think we should make in order to solve the problem?we can use |
mallenexpensify
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~021861915865037036576 |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov ( |
|
👋 @gadhiyamanan , it's been a minute! |
|
Edited by proposal-police: This proposal was edited at 2024-11-28 04:11:57 UTC. ProposalPlease 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
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 // 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();
+ }}
/>POCScreen.Recording.2024-11-28.at.11.07.51.movNote: For similar cases like this issue, we can reuse this solution What alternative solutions did you explore? (Optional) |
|
Edited by proposal-police: This proposal was edited at 2024-11-29 07:35:59 UTC. ProposalPlease 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?
That means the keyboard will not be dismissed if the tap is handled by the children.
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 so it cannot handle the tap. So the keyboard is closed. Otherwise, with the white area, it is the 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?
<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}
>
This means that if the deepest component in: is an interactive component (such as Button, Pressable, etc.), the What alternative solutions did you explore? (Optional)
onPress={
draftMessage === undefined
? () => {
onPress?.();
Keyboard.dismiss();
}
: undefined
}or onPress={
draftMessage === undefined
? () => {
onPress?.();
Keyboard.dismiss();
}
: () => {
Keyboard.dismiss();
}
}
|
|
Edited by proposal-police: This proposal was edited at 2024-12-02 12:26:18 UTC. ProposalPlease re-state the problem that we are trying to solve in this issueIn 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
in our case, the chat report message being a What changes do you think we should make in order to solve the problem?Given that The ideal solution here would have to fulfil both:
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 Implementation
if (shouldDismissKeyboardOnPress && Keyboard.isVisible()) {
Keyboard.dismiss();
}leveraging our newly added prop and react-native's
And we're done, this ensures we can dynamically add the dismiss functionality for the onPress using the Here's a diff of all the changes for convenience: DIFFdiff --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
shouldHandleScrollWhat alternative solutions did you explore?Alternatively, skip the addition of the Result video (before / after)Android: Native (One Plus 7T - Real device)
|
|
@gadhiyamanan Thank you for your proposal, |
|
@huult Thank you for your proposal, your solution is solving the issue |
|
@daledah Thank you for detailed RCA. I think |
|
@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 |
|
Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
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.
cc @AndrewGable |
@daledah But chosen proposal is not causing the regression you mentioned Screen.Recording.2024-12-02.at.5.07.42.PM.mov |
|
Thanks for the review!
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 @alitoshmatov You might want to take a second look at this and reconsider my solution for a complete fix which would not require adding Updated proposal
|
@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.
What do you think about this? Such as in here, if |
|
@ikevin127 I don't think closing keyboard on every onPress of |
@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? |
|
Yes that is definitely a valid concern. Whether it is reproducible or not, it is still something to cover in the solution. |
|
@daledah I still think we should apply |
@alitoshmatov My alternative solution works well and can make sure we do not miss any case such as this one or when About the case
|
@daledah But this introduces this regression, right? |
|
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. |
No. I just tested the alternative solution and there is no regression. I updated the proposal to remove that note. |
|
@alitoshmatov I just tested the case draftMessage !== undefined, and we are able to dismiss the keyboard when tapping outside. Screen.Recording.2024-12-05.at.00.40.52.mp4 |
|
@daledah Anyway, I think @alitoshmatov had more context to decide which one to go with. Let's wait for the result 🙂 |
@alitoshmatov I'm not, I don't understand why any of my proposed solutions are not meeting the requirements even after adding the no new prop as alternative after it was mentioned in #53196 (comment).
Then you backpedaled that decision basically invalidating my alternative solution (excluding the new prop), then I mentioned in #53196 (comment) how and why I proposed the new prop in my main solution - because it allows to control exactly when and where we want the So far none of the other proposals offered a solution that targets the root I'm still wondering why, given the main / alternative solutions added to my proposal - it's still being ignored in the favor of any of the other proposals. Maybe @AndrewGable can give me a straight answer to that question from a technical point of view, I would appreciate it. |
|
@ikevin127 Thanks for your feedback, I really appreciate it. I didn't approve applying changes to
I haven't seen any compelling argument here for why Moreover @huult's proposal suggested adding one My above points might come of as aggressive as it’s difficult to convey tone through writing, I just want to show you my reasoning and thought process. That said, there is always a chance I might be wrong and I open to reevaluating all the proposals again. If you think so I will be happy to invite another C+ and reassign this issue to them. |
|
No need to reassign, I just wanted to understand the reasoning. Thanks! |
|
@AndrewGable, @mallenexpensify, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too... |
|
Okay, after a lot of consideration I think we should go with @daledah 's alternative solution which suggests to update C+ reviewed 🎀 👀 🎀 cc: @AndrewGable |
|
Current assignee @AndrewGable is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @daledah 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
@AndrewGable @mallenexpensify @alitoshmatov @daledah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
melvin-bot
bot
added
Reviewing
|
This issue has not been updated in over 15 days. @AndrewGable, @mallenexpensify, @alitoshmatov, @daledah 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! |
|
PR is moving along! |
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:
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:
Screenshots/Videos
Bug6677688_1732684892160.Tap_Keyboard.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @alitoshmatovThe text was updated successfully, but these errors were encountered: