Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
4.03k stars 3.03k forks source link

[$250] Tooltip - Tooltip appears after dismissing confirmation pop-up when RHP is still opened #55730

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 weeks 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: 9.0.89-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+100106kh@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Create a new account
  3. Do not interact with FAB so that FAB tooltip is not dismissed
  4. Go to Account settings > Workspaces
  5. Create a new workspace
  6. Go to workspace chat
  7. Click + > Create expense > Manual
  8. Enter amount > Next
  9. Click Merchant
  10. Note that no tooltip appears when RHP is opened
  11. Enter anything on the merchant field
  12. Click outside the merchant RHP
  13. Click Cancel

Expected Result:

No tooltip will appear as long as RHP remains opened

Actual Result:

Tooltip appears after dismissing confirmation pop-up when RHP is still opened

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/4d9ae288-1818-4656-9aa8-3e05aa2db766

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021883956231443633358
  • Upwork Job ID: 1883956231443633358
  • Last Price Increase: 2025-01-27
Issue OwnerCurrent Issue Owner: @getusha
melvin-bot[bot] commented 3 weeks ago

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.

daledah commented 3 weeks ago

Proposal

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

Tooltip appears after dismissing confirmation pop-up when RHP is still opened

What is the root cause of that problem?

Tooltip will not show when modal onyx value isVisible is true: https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/components/ProductTrainingContext/index.tsx#L102-L104 And when opening a RHP, isVisible becomes true: https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L201-L212 https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L551-L556 But when we make actions to the discard changes modal, we set isVisible to false: https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/components/Modal/BaseModal.tsx#L91 This makes tooltip visible.

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

Add a param shouldSetModalVisibility to DiscardChangesConfirmation and pass it to ConfirmModal here: https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/pages/iou/request/step/DiscardChangesConfirmation.tsx#L34-L48 Although we only use this component in two places: IOURequestStepDescription and IOURequestStepMerchant, we should use a param instead of straight passing false for future proof. And use shouldSetModalVisibility as false when needed, currently we should use it in both places above.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA, UI Bug

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

FitseTLT commented 3 weeks ago

Proposal

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

Tooltip - Tooltip appears after dismissing confirmation pop-up when RHP is still opened

What is the root cause of that problem?

Normally, the tooltip is not displayed if modal is visible https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/components/ProductTrainingContext/index.tsx#L102-L104 but the problem here is although we set modal visible when opening the RHP here https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L203 we are setting it to false when dismissing the discard confirm modal https://github.com/Expensify/App/blob/bdf216ee216f874bef2ed235ba14407f865953f2/src/components/Modal/BaseModal.tsx#L91

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

We need to solve the problem from the root cause so as to prevent similar problems. So BaseModal should not set modal visibility to false on hideModal if modal was visible when the BaseModal was made visible because when multiple modals are stacked like in the current case the last modal under the stack should set the modal visibility to false when it is closed

So on handleShowModal here we will set a ref wasModalVisible if a modal was visible https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/components/Modal/BaseModal.tsx#L132-L134

const [modal] = useOnyx(ONYXKEYS.MODAL);
    const wasModalVisibleRef = useRef(false);

    const handleShowModal = () => {
        if (shouldSetModalVisibility) {
            if (modal?.isVisible) {
                wasModalVisibleRef.current = true;
            } else {
                Modal.setModalVisibility(true);
            }

and https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/components/Modal/BaseModal.tsx#L90-L91

if (shouldSetModalVisibility && !wasModalVisibleRef.current) {
                    Modal.setModalVisibility(false);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA as it is a UI bug

What alternative solutions did you explore? (Optional)

bernhardoj commented 3 weeks ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-24 19:14:05 UTC.

Proposal

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

The educational tooltip appears when closing the discard confirmation modals. This also happens when closing 3-dots popover menu.

What is the root cause of that problem?

As the others have pointed out, the tooltip hides when a modal is visible, including RHP. When we close the discard confirm modal, areAllModalsHidden is true, so the modal visibility is set to false. https://github.com/Expensify/App/blob/9043b14838fce3486e339ad2021a085ceead49fd/src/components/Modal/BaseModal.tsx#L86-L92

That causes the tooltip to show back.

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

Since we set modal visibility based on RHP too, we need to check if the last screen isn't a modal screen.

if (Modal.areAllModalsHidden() && (navigationRef.getRootState().routes.at(-1)?.name !== "RightModalNavigator" || ... "LeftModalNavigator" || ...)) {

OR

I think it's better to create a separate onyx state for the screen modal visibility.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

github-actions[bot] commented 3 weeks ago

⚠️ @bernhardoj Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

mallenexpensify commented 3 weeks ago

Checking in #qa about . I looked in TestRail to see if we're not supposed to show tooltips when RHP is open. Also... don't think it's a huge deal so not rushing to triage/fix

melvin-bot[bot] commented 2 weeks ago

Current assignee @mallenexpensify is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 2 weeks ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

mallenexpensify commented 2 weeks ago

We do want to fix this and.... per usual.... if there are other instances we can find, I'd love to fix those too. From the chat I raised

I do think its a bug since the Tool Tip is appearing over the overlay. It's pointing at a button that the user should not be able to click with the RHP open

Also, I was able to reproduce

Image

@getusha can you review the above proposals plz? Thx

melvin-bot[bot] commented 2 weeks ago

@mallenexpensify, @getusha, @dubielzyk-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

getusha commented 2 weeks ago

Reviewing, will update in short.

getusha commented 2 weeks ago

I think updating the modal status based on whether RHP is opened or not is the best solution. We can proceed with @bernhardoj's proposal.

🎀 👀 🎀 C+ Reviewed.

melvin-bot[bot] commented 2 weeks ago

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

bernhardoj commented 1 week ago

PR is ready

cc: @getusha