Closed izarutskaya closed 1 week ago
Triggered auto assignment to @isabelastisser (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.
We think this issue might be related to the #vip-vsb
Job added to Upwork: https://www.upwork.com/jobs/~013bc99e56ff65d5ac
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External
)
issue is reproducible from the steps mentioned in the issue. In browser, send message button from the chat doesn't register tap event when responsive-mode is turned-on-and-or-off from devtools, until the browser is refreshed.
For send button, we are using GestureDetector
from react-native-gesture-handler
lib's tap gesture, which is not necessary, and a quick search through codebase results in about 2 places where GestureDetector
is used exclusively for tap interactions, while in other cases, PressableWithFeedback
or PressableWithoutFeedback
is used.
Places where GestureDetector
is used for detecting a tap:
The straight forward solution would be to remove the GestureDetector
wrapper, and use TouchableWithFeedback
's onPress
callback instead. I have tested it locally, and it works fine.
The SendButton component would look like:
<View
style={styles.justifyContentEnd}
// Keep focus on the composer when Send message is clicked.
onMouseDown={(e) => e.preventDefault()}
>
<Tooltip text={translate('common.send')}>
<PressableWithFeedback
style={({pressed, isDisabled}) => [
styles.chatItemSubmitButton,
isDisabledProp || pressed || isDisabled ? undefined : styles.buttonSuccess,
isDisabledProp ? styles.cursorDisabled : undefined,
]}
role={CONST.ROLE.BUTTON}
accessibilityLabel={translate('common.send')}
disabled={isDisabledProp}
onPress={handleSendMessage}
>
...
Thinking of confirming if it's an upstream issue with react-native-gesture-handler
library for web, if we really need to use GestureDetector
in this particular case.
π£ @vaib-contrib! π£ Hey, it seems we donβt have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
β Contributor details stored successfully. Thank you for contributing to Expensify!
When we resize window, the gesture.Taps() below: https://github.com/Expensify/App/blob/2a641d98760dfdc1a2ebe9b958787c9cfacb02c2/src/pages/home/report/ReportActionCompose/SendButton.tsx#L26 does not trigger.
In react-native-gesture-handler, when the window is resized, the layout of the page changes. This change can invalidate the context in which the gesture was initially recognized. The react-native-gesture-handler library might still be working with the old context and dimensions, leading to gestures not being recognized correctly.
In this logic, https://github.com/software-mansion/react-native-gesture-handler/blob/0b983ed9dd6c7e11d3b6906a85faa535346a16e0/src/handlers/gestures/GestureDetector/index.tsx#L154-L160, the attachHandler
function (that will make the gesture works properly) is only called one time when the component is mounted.
So when we change to "mobile" view, the attachHandler
is not called again.
To address this issue, we should add the key
prop, and every time a component's key changes React will create a new component instance rather than update the current one.
To do it, in this component: https://github.com/Expensify/App/blob/2a641d98760dfdc1a2ebe9b958787c9cfacb02c2/src/pages/home/report/ReportActionCompose/SendButton.tsx#L36 add:
key={`${isSmallScreenWidth}`}
with:
const {isSmallScreenWidth} = useWindowDimensions();
key={screenWidth}
to make sure the button always re-renders if the layout changed.Bump @hoangzinh to review the proposals above. Thanks!
Thanks for proposals, everyone.
https://github.com/Expensify/App/assets/9639873/6685efb0-0f70-4f1b-8fed-824a372722ec
@hoangzinh The video player progress uses Gesture.Pan()
, not Gesture.Tap()
so there is no issue with it.
@truph01 how about MagicCode? https://github.com/Expensify/App/blob/3e1d94a22e93bdd66753f8f5a955ec5e3b5d366a/src/components/MagicCodeInput.tsx#L214
@hoangzinh I see that the MagicCodeInput
always mounts again one we change from large screen to small screen, so there is no similar bug. You can try to add the below logic to that file:
useEffect(()=>{
console.log("mounted")
}, [])
@hoangzinh As you can see in this logic, https://github.com/software-mansion/react-native-gesture-handler/blob/0b983ed9dd6c7e11d3b6906a85faa535346a16e0/src/handlers/gestures/GestureDetector/index.tsx#L154-L160, the attachHandler
function (that will make the gesture works properly) is only called one time when the component is mounted.
That is why in my solution, I added the key
prop, and every time a component's key changes React will create a new component instance rather than update the current one.
Looks good, can you add above into your proposal so internal engineer can review it as well? Thanks
@hoangzinh I updated proposal
@truph01's proposal https://github.com/Expensify/App/issues/43650#issuecomment-2167195682 looks good to me
πππ C+ reviewed
Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @hoangzinh π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @truph01 π 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 π
@hoangzinh PR https://github.com/Expensify/App/pull/43976 can be reviewed
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.1-19 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-02. :confetti_ball:
For reference, here are some details about the assignees on this issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:
For reference, here are some details about the assignees on this issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
Bump @hoangzinh to complete the checklist above before payment. Thanks!
@hoangzinh $250 automatic offer (Reviewer), please accept the offer in Upwork, and I will process the payment. Thanks!
@truph01 automatic offer (Contributor). The payment is processed in Upwork.
all set!
Accepted. Thanks @isabelastisser
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: v1.4.82-4 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team
Action Performed:
Expected Result:
Message sent in mobile view as in normal
Actual Result:
Send button not response. Also, after returning to normal view, Send button not response. User need to reload page
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
https://github.com/Expensify/App/assets/115492554/28e95393-c8a0-448b-8788-9908726acbed
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @isabelastisser