[$500] Chat - Emoji suggestion list can not be dismissed when tap outside

lanitochka17 commented 11 months ago

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.3.96-0 Reproducible in staging?: Y Reproducible in production?: Y 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Open New Dot app
Navigate to any chat
Type :sm
Tap outside the opened dropdown to dismiss

Expected Result:

When user tap outside the dropdown - emoji suggestion page should close

Actual Result:

Emoji suggestion list can not be dismissed when tap outside

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/2e10ee9f-1701-463f-8b6b-17fba1954524

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01dbf05e8d1e6a1b24
Upwork Job ID: 1721953857078456320
Last Price Increase: 2024-01-16
Automatic offers:

s77rt | Contributor | 27873672

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

dukenv0307 commented 11 months ago

Proposal

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

Emoji suggestion list can not be dismissed when tap outside

What is the root cause of that problem?

In native platforms, the Composer doesn't get blurred when tapping outside, so the emoji suggestion will not dismiss

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

We can listen to touches/presses in the ScreenWrapper of the screens that have the emoji suggestion list, eg. by using onTouchStart/onTouchEnd
In the event object, check if the target element is the Composer or the suggestion list item (we can do this eg. by using setting nativeID to the Composer/suggestion list item and compare it)
If yes, do nothing
If no, that means we touch outside of the suggestion list/composer, we'll dismiss the suggestion list

This is the high level solution, the exact methods to use for each step might vary.

What alternative solutions did you explore? (Optional)

NA

s-alves10 commented 11 months ago

Proposal

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

Emoji suggestions list can't be dismissed by tapping outside in native platforms

What is the root cause of that problem?

In web and desktop platforms, when tapping outside, the composer gets blurred and suggestions list is reset https://github.com/Expensify/App/blob/24184e4a987aba011b5231680da66dcef11f6a24/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L286-L288

But this isn't the case for native platforms, that is, the composer doesn't get blurred when tapping outside. This is the root cause

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

I suggest to add the PressableWithoutFeedback at the green rectangle area in the above screenshot. Blur the composer or reset the suggestions in its onPress. This way we can close the suggestion list when tapping outside

I recommend blurring the composer personally This works as expected

Result

https://github.com/Expensify/App/assets/126839717/bbb15cc3-08b4-444b-9b7a-8da80e84d32f

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 11 months ago

@puneetlath, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 11 months ago

@puneetlath, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 11 months ago

@puneetlath @thesahindia 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] commented 11 months ago

@puneetlath, @thesahindia 12 days overdue now... This issue's end is nigh!

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

puneetlath commented 11 months ago

@thesahindia can you take a look at the proposals?

thesahindia commented 11 months ago

I am not in a good shape. I am going to unassign myself. Sorry for the delay.

s77rt commented 11 months ago

Taking this as C+

s77rt commented 11 months ago

@dukenv0307 Thanks for the proposal. I don't think your RCA is correct. keyboardShouldPersistTaps is only related to the keyboard. If we happen to have the keyboard closed already then clicking outside will still not dismiss the suggestion list.

https://github.com/Expensify/App/assets/16493223/be2906d6-9a7b-49aa-8c80-9d6ed602014e

s77rt commented 11 months ago

@s-alves10 Thanks for the proposal. Your RCA is correct. However I don't like the idea of having a global click event listener. Can we maybe as an alternative use the remaining space above the suggestion list to render a pressable (kinda backdrop) that hides the suggestion list?

s-alves10 commented 11 months ago

@s77rt

Yes, we can avoid global listener. I just suggested that for its use in other places.

Can we maybe as an alternative use the remaining space above the suggestion list to render a pressable (kinda backdrop) that hides the suggestion list?

Here is my opinion. If we need to consider the suggestions list only, we can fix the issue simply by using the outside press handler. But I think we need to consider the composer as well. When tapping outside the suggestions list but inside the composer, the suggestions list should not be dismissed, I think

Please let me know your thoughts

s77rt commented 11 months ago

@s-alves10 Let's not use that outside press handler. It seems an overkill to add a new package to solve this. Did you get a chance to explore the suggested alternative? (i.e. render a pressable above the suggestion list)

dukenv0307 commented 11 months ago

@dukenv0307 Thanks for the proposal. I don't think your RCA is correct. keyboardShouldPersistTaps is only related to the keyboard. If we happen to have the keyboard closed already then clicking outside will still not dismiss the suggestion list.

@s77rt I think in your video you're disabling software keyboard, not having keyboard closed. For the App it will still consider that the keyboard is opened. Can you try removing the keyboardShouldPersistTaps="handled"? You'll see the issue will not happen.

s77rt commented 11 months ago

@dukenv0307 I disabled the software keyboard on purpose to show that keyboardShouldPersistTaps is not the root cause. You can experience same bug on Android (on Android the keyboard can actually be closed)

https://github.com/Expensify/App/assets/16493223/3ceed674-dd57-490c-b2db-4aec7b4a88e0

s-alves10 commented 11 months ago

Proposal

Updated

@s77rt

Will you take a look again?

s77rt commented 11 months ago

@s-alves10 Thanks for the update. Why is the rectangle covering the suggestion list? What would happen if we click on the suggestion list in this case?

s-alves10 commented 11 months ago

@s77rt Tapping on the suggestion list works as is now

s77rt commented 11 months ago

But will it register as a press on the other pressable?

s-alves10 commented 11 months ago

The suggestion list is placed above the Pressable and onPress isn't called when tapping on the list

s77rt commented 11 months ago

Okay I see. Can you please explain how/where the pressable is added?

s-alves10 commented 11 months ago

In AutoCompleteSuggestions component

function AutoCompleteSuggestions({measureParentContainer, onClose, ...props}) {
    const {windowHeight} = useWindowDimensions();
    const styles = useThemeStyles();
    return (
        <Portal hostName="suggestions">
            {props.suggestions?.length > 0 && (
                <PressableWithoutFeedback
                    style={[styles.autoCompleteOutsideContainer, {top: -windowHeight, height: windowHeight}]}
                    onPress={onClose}
                />
            )}
            {/* eslint-disable-next-line react/jsx-props-no-spreading */}
            <BaseAutoCompleteSuggestions {...props} />
        </Portal>
    );
}

s-alves10 commented 11 months ago

In styles.ts

        autoCompleteOutsideContainer: {
            left: -20,
            right: -20,
            position: 'absolute',
        },

20 is the padding value of ReportFooter

s77rt commented 11 months ago

@s-alves10 This is looking good so far but there is an inconsistency between Android an iOS. On Android we can scroll the messages but on iOS we can't. It would be better if we keep the scroll functionality on iOS too. Also how are we going to call resetSuggestions?

https://github.com/Expensify/App/assets/16493223/0f0b80db-607a-423c-88a4-eddcf031b477

dukenv0307 commented 11 months ago

@dukenv0307 Thanks for the proposal. I don't think your RCA is correct. keyboardShouldPersistTaps is only related to the keyboard. If we happen to have the keyboard closed already then clicking outside will still not dismiss the suggestion list.

@s77rt sorry if I wasn't clear, but keyboardShouldPersistTaps is related to the composer being focused, not just the keyboard. So if keyboardShouldPersistTaps is handled, if the composer is being focused, tapping on a report action will not blur the composer. Meanwhile if you remove keyboardShouldPersistTaps="handled", tapping on a report action will blur the composer.

But this isn't the case for native platforms, that is, the composer doesn't get blurred when tapping outside. This is the root cause

@s77rt if you think this root cause is correct, then whatever the keyboardShouldPersistTaps, the composer will never gets blurred when tapping outside right.

Then why if keyboardShouldPersistTaps="handled" is removed, the composer now gets blurred when tapping outside?

Can we maybe as an alternative use the remaining space above the suggestion list to render a pressable (kinda backdrop) that hides the suggestion list?

@s77rt for this, why do we need to render a Pressable when we already have all the report actions which are Pressables, and we can just apply the event to it as mentioned in my proposal?

s-alves10 commented 11 months ago

@s77rt

I'm going to pass props onClose to Suggestions component in ComposerWithSuggestions

It would be better if we keep the scroll functionality on iOS too

It looks tricky to pass touch events through a view without blocking onPress. How about to prevent background scrolling?

s77rt commented 11 months ago

@dukenv0307

Meanwhile if you remove keyboardShouldPersistTaps="handled", tapping on a report action will blur the composer.

It won't. In this case tapping outside will dismiss the keyboard. The keyboard dismissal action is what blurs the input and not the outside press.

if you think this root cause is correct ... Then why if keyboardShouldPersistTaps="handled" is removed, the composer now gets blurred when tapping outside?

As explained above, it's not the root cause because it's not the main action that blurs the input. Tapping outside will not blur the input. Tapping outside will dismiss the keyboard. If the keyboard got dismissed the input will get blurred. It's a chain reaction. If keyboard is closed then the input will stay focused.

https://github.com/Expensify/App/assets/16493223/5d4be113-7848-44ab-acb8-3bf539187a90

why do we need to render a Pressable when we already have all the report actions which are Pressables

Because components shouldn't be coupled. We should be able to close the suggestion box no matter what is rendered underneath.

s77rt commented 11 months ago

@s-alves10

How about to prevent background scrolling?

Scrolling looks like a feature here. I think it would be better to keep it.

melvin-bot[bot] commented 11 months ago

@puneetlath this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 11 months ago

@puneetlath Eep! 4 days overdue now. Issues have feelings too...

s77rt commented 11 months ago

Still looking for proposals

s77rt commented 11 months ago

@puneetlath Please assign me here

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 11 months ago

📣 @s77rt 🎉 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 📖

s77rt commented 10 months ago

Still looking for proposals

s-alves10 commented 10 months ago

@s77rt

We can solve this issue by simply changing this line to

                    keyboardShouldPersistTaps="never"

The only drawback of this solution is that it won't work if hardware keyboard is connected and so keyboard isn't opened when the composer input is focused. I'd like to know we should consider this case as well

s77rt commented 10 months ago

@s-alves10 This is basically the solution suggested by @dukenv0307. On Android you can close the software keyboard and then tapping outside won't close the suggestion list.

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

@puneetlath @s77rt this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 10 months ago

Current assignee @s77rt is eligible for the Internal assigner, not assigning anyone new.

s77rt commented 10 months ago

@puneetlath Should we bring this back External?

melvin-bot[bot] commented 10 months ago

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

s77rt commented 10 months ago

Still looking for proposals

s77rt commented 10 months ago

Same ^

Expensify / App