Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Chat - The contact is highlighted when you create a conversation #37800

Closed
6 tasks done
kbecciv opened this issue Mar 5, 2024 · 56 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kbecciv
Copy link

kbecciv commented Mar 5, 2024

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.4.47-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: https://expensify.testrail.io/index.php?/tests/view/4380775
Issue reported by: Applause - Internal Team

Action Performed:

  1. Open https://staging.new.expensify.com/
  2. Log in to your HT account
  3. Tap on the green plus button (FAB)
  4. Select Start chat

Expected Result:

In mWeb and native apps, highlighting contacts should not happen since there is no arrow navigation.

Actual Result:

The contact is highlighted when you create a conversation

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6403281_1709668015258.IMG_001814.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017825971f9646dbf3
  • Upwork Job ID: 1767170648931237888
  • Last Price Increase: 2024-04-29
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 5, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Mar 5, 2024

@sonialiap I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@kbecciv
Copy link
Author

kbecciv commented Mar 5, 2024

We think that this bug might be related to #vip-vsb
@quinthar

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 6, 2024

Proposal

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

  • Chat - The contact is highlighted when you create a conversation

What is the root cause of that problem?

  • When implementing ArrowKeyFocusManager, we do not consider if the device is small width device or not

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

  • We just need to remove the "highlight" effect in mobile device
  • We can update this
        const isItemFocused = !DeviceCapabilities.canUseTouchScreen() && ...

What alternative solutions did you explore? (Optional)

  • We can update this to:
        isActive: !DeviceCapabilities.canUseTouchScreen(),

@dragnoir
Copy link
Contributor

dragnoir commented Mar 6, 2024

The main component for this behaviour is OptionsSelector and it's deprecated #37628. I think we can close this.

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 7, 2024

@dragnoir The component using SelectionList also encountered this behavior

@melvin-bot melvin-bot bot added the Overdue label Mar 8, 2024
@sonialiap
Copy link
Contributor

I can reproduce this

@sonialiap sonialiap added the External Added to denote the issue can be worked on by a contributor label Mar 11, 2024
@melvin-bot melvin-bot bot changed the title Chat - The contact is highlighted when you create a conversation [$500] Chat - The contact is highlighted when you create a conversation Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@dragnoir
Copy link
Contributor

dragnoir commented Mar 11, 2024

Proposal

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

Wrong highlighted item.

What is the root cause of that problem?

We will not use OptionsSelector or update it. Deprecated.

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

Replace OptionSelector with SelectionList.

As discussed here: https://expensify.slack.com/archives/C01GTK53T8Q/p1709156803359359 We are replacing OptionSelector by SelectionList.

What alternative solutions did you explore? (Optional)

@Krishna2323
Copy link
Contributor

Krishna2323 commented Mar 11, 2024

Proposal

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

Chat - The contact is highlighted when you create a conversation

What is the root cause of that problem?

We use BaseOptionsSelector which has default focused index and updates focus index when selection updates.

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

Use SelectionList instead of BaseOptionsSelector. Apart from that we also need to make few changes in the code because if user enters something in the text input the first option gets highlighted.

useEffect(() => {
// Avoid changing focus if the textInputValue remains unchanged.
if (prevTextInputValue === textInputValue || flattenedSections.allOptions.length === 0) {
return;
}
// Remove the focus if the search input is empty else focus on the first non disabled item
const newSelectedIndex = textInputValue === '' ? -1 : 0;
updateAndScrollToFocusedIndex(newSelectedIndex);
}, [canSelectMultiple, flattenedSections.allOptions.length, prevTextInputValue, textInputValue, updateAndScrollToFocusedIndex]);

We need to make few updates:

  1. Update updateAndScrollToFocusedIndex to accept separate values for focusIndex & scrollIndex, because we want to still scroll to top when user enters something but w don't want to focus.
  2. Determine newFocusIndex based on canUseTouchScreen value.
        const newFocusIndex = textInputValue === '' || (canUseTouchScreen && focusedIndex < 0) ? -1 : 0;
        const newScrollIndex = textInputValue === ''  ? -1 : 0;
        
        updateAndScrollToFocusedIndex(newFocusIndex,newScrollIndex);

Result

@superAI4003
Copy link

Hello.
I am a freelancer on upwork.
I am very interested in your project.
Thanks

Copy link

melvin-bot bot commented Mar 11, 2024

📣 @superAI4003! 📣
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:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@dragnoir
Copy link
Contributor

@fedirjh this issue is fixed with #34356

Copy link

melvin-bot bot commented Mar 12, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@nkdengineer
Copy link
Contributor

@fedirjh I just tested but still can reproduce

@fedirjh
Copy link
Contributor

fedirjh commented Mar 13, 2024

this issue is fixed with #34356

@dragnoir It seems the PR was reverted.

I think we should hold this issue until the refactor is done.

@fedirjh
Copy link
Contributor

fedirjh commented Mar 13, 2024

I just tested but still can reproduce

@nkdengineer The PR that fixed this was reverted in #38182

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 13, 2024

@fedirjh The same behavior also appears in other similar components, for example the search page. And it is because we are using ArrowKeyFocusManager in mobile device, that does not belong to only New chat page

@dragnoir
Copy link
Contributor

@fedirjh I think we should hold for #34356 refactor

@melvin-bot melvin-bot bot added the Overdue label Mar 15, 2024
@sonialiap
Copy link
Contributor

PR is merged! Waiting for deploy

@melvin-bot melvin-bot bot removed the Overdue label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

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

@dragnoir
Copy link
Contributor

This issue is fixed within #38610

@melvin-bot melvin-bot bot added the Overdue label Apr 17, 2024
@sonialiap
Copy link
Contributor

Thanks Rachid, closing this one out

@melvin-bot melvin-bot bot removed the Overdue label Apr 18, 2024
@nkdengineer
Copy link
Contributor

@sonialiap I still can reproduce in staging:

Screen.Recording.2024-04-18.at.20.44.45.mov

@nkdengineer
Copy link
Contributor

@fedirjh Can you help check this comment ?

@fedirjh
Copy link
Contributor

fedirjh commented Apr 22, 2024

In mWeb and native apps, highlighting contacts should not happen since there is no arrow navigation.

@nkdengineer I am still able to repo, but I am having second thoughts on the expected behavior. In my opinion, it does not seem correct. However, you can use the soft keyboard to navigate to the highlighted contact. So it makes sense to highlight the contact.

CleanShot.2024-04-22.at.06.49.37.mp4

@sonialiap sonialiap reopened this Apr 22, 2024
@dragnoir
Copy link
Contributor

@fedirjh can you pls re-check with the latest main

20240422_102528.mp4

Copy link

melvin-bot bot commented Apr 22, 2024

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

@nkdengineer
Copy link
Contributor

@fedirjh In my opinion, I believe that on mobile devices, we should avoid having the "highlight" effect as seen in the expected behavior in the original post. This is because the scenario where the user utilizes the "soft keyboard" is infrequent.

@fedirjh
Copy link
Contributor

fedirjh commented Apr 23, 2024

can you pls re-check with the latest main

@dragnoir Try to type any characters, it will highlight the first one.

I believe that on mobile devices, we should avoid having the "highlight" effect as seen in the expected behavior in the original post.

Hmm, this will also create inconsistency between the Web version and mobile version. I believe the current behaviour is universal and consistent.

cc @Expensify/design I would love to hear your opinion on this. Do you think it would be a good idea to disable the highlighting of contacts within the search list on mobile devices?

@dubielzyk-expensify
Copy link
Contributor

cc @Expensify/design I would love to hear your opinion on this. Do you think it would be a good idea to disable the highlighting of contacts within the search list on mobile devices?

Yeah that sounds reasonable to me. Given we focus on the input field for contact when clicking "Start chat" I don't think anything should be selected until you tab out of it. And given on mobile that's not an option (unless you have external keyboard hooked up) it doesn't make sense to show any highlight at all. Keen to hear what rest of @Expensify/design thinks though.

@fedirjh
Copy link
Contributor

fedirjh commented Apr 24, 2024

@dubielzyk-expensify On mobile, we can use the soft keyboard return key to navigate to the highlighted contact, check below video for iOS and this video for android

CleanShot.2024-04-24.at.02.21.08.mp4

@shawnborton
Copy link
Contributor

Yeah, I think I generally agree with this from the OP:

Expected Result:
In mWeb and native apps, highlighting contacts should not happen since there is no arrow navigation.

@shawnborton
Copy link
Contributor

I see the point about the enter key though... and if we decide to keep that kind of behavior, then it would make sense to keep the highlighting.

@dannymcclain
Copy link
Contributor

I also agree that no list items should be highlighted/focused until you specifically tab down into the list.

@fedirjh
Copy link
Contributor

fedirjh commented Apr 24, 2024

no list items should be highlighted/focused until you specifically tab down into the list.

@dannymcclain Should this be applied only for mobile or it should be on all platforms?

I see the point about the enter key though... and if we decide to keep that kind of behavior, then it would make sense to keep the highlighting.

@shawnborton This behavior is consistent across all platforms, using the enter key will navigate you to the highlighted contact. You can check the expected behaviour in this issue #39631.

@dannymcclain
Copy link
Contributor

Ok looking through this again, is this the correct expected behavior:

  • No items highlighted when you first load the list (and have not typed anything)
  • Once you start typing, we filter the list based on your query and highlight the first list item
  • If you press Enter/Return, we navigate you to the highlighted list item

If this is the expected behavior, I think it is ok to keep it on mobile as well, since it's been pointed out that using the mobile enter key still functions the same way.

Seems like the video in the description of this issue is showing that the first list item is highlighted before a search term is even entered, which is inconsistent with the expected behavior bullets I outlined above (if those are the correct expected behaviors!)

@dragnoir
Copy link
Contributor

Seems like the video in the description of this issue is showing that the first list item is highlighted before a search term is even entered, which is inconsistent with the expected behavior bullets I outlined above (if those are the correct expected behaviors!)

Not anymore after #38610

@fedirjh
Copy link
Contributor

fedirjh commented Apr 25, 2024

@dannymcclain Thank you for your feedback. I believe that the expected behavior bullets outlined above accurately reflect what we currently have.

Seems like the video in the description of this issue is showing that the first list item is highlighted before a search term is even entered,

This seems to be already fixed, So we are good to close this issue. cc @sonialiap


If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4380775

I propose updating the expected behavior of this regression test to accurately reflect its intended functionality. The revised expected behavior is as follows:

  • Upon initially loading the list and before any text is typed, no items should be highlighted.
  • When a user starts typing, the first contact in the list (if it exists) should be highlighted.

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

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

@sonialiap
Copy link
Contributor

Closing 👍

@melvin-bot melvin-bot bot removed the Overdue label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
No open projects
Archived in project
Development

No branches or pull requests

10 participants