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

[$1000] Double clicking on search button selects placeholder and unable to type #15632

Closed
1 of 6 tasks
kavimuru opened this issue Mar 3, 2023 · 35 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering 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

@kavimuru
Copy link

kavimuru commented Mar 3, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Click the search button
  2. Type some random to trigger "No results found" message
  3. Close the modal
  4. Double click on search button back to back

Expected Result:

Text Input should be in focus and user should be able to write

Actual Result:

Placeholder gets selected and user is unable to write

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome
  • MacOS / Desktop

Version Number: 1.2.78-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
Notes/Photos/Videos:

Screen.Recording.2023-02-24.at.6.45.09.PM.1.mov
Recording.119.mp4

Expensify/Expensify Issue URL:
Issue reported by: @BhavyaKoshiya
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677244790953179

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01af2f520108bae0a4
  • Upwork Job ID: 1632346663529680896
  • Last Price Increase: 2023-03-05
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 3, 2023
@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 3, 2023
@MelvinBot
Copy link

MelvinBot commented Mar 3, 2023

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

@sophiepintoraetz
Copy link
Contributor

image

Can reproduce (though not every time)

@MelvinBot
Copy link

Triggered auto assignment to @techievivek (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@techievivek
Copy link
Contributor

This can be worked externally.

@techievivek techievivek added the External Added to denote the issue can be worked on by a contributor label Mar 5, 2023
@melvin-bot melvin-bot bot unlocked this conversation Mar 5, 2023
@melvin-bot melvin-bot bot changed the title Double clicking on search button selects placeholder and unable to type [$1000] Double clicking on search button selects placeholder and unable to type Mar 5, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

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

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

@rjharish333

This comment was marked as off-topic.

@MelvinBot

This comment was marked as duplicate.

@allroundexperts
Copy link
Contributor

allroundexperts commented Mar 5, 2023

Proposal

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

Double pressing search icon on dashboard results in search input placeholder being selected and unfocused.

What is the root cause of that problem?

The root cause of the issue is that the second click causes the focus to shift from the new screen back to the previous screen.

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

There are multiple solutions.

  1. We can set shouldDelayFocus to true here. This will cause the focus to happen on when screen is fully open.
  2. If we want to get even more accurate than shouldDelayFocus, we can use didScreenTransitionEnd property defined here to focus the input only when it becomes true.

What alternative solutions did you explore? (Optional)

We can show the search input only when the transition ends. This is similar to what we are doing for the user items as well. Since we're showing the loader at that time, I think it also makes sense to not show to search bar until the transition ends.
https://expensify.slack.com/archives/C01GTK53T8Q/p1678110078885399

@getusha
Copy link
Contributor

getusha commented Mar 5, 2023

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

Double clicking the search icon causes the input to lose focus and select the label text

What is the root cause of that problem?

The issue arises when the transition is still in progress and we click on the overlay, causing it to receive focus instead of the input, which remains blurred.

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

  1. Focus the input after the transition completes more details: [HOLD for payment 2023-04-04] [$4000] Web - NO focus on Search input field when opening FAB menu and then press on CTRL+K #15276 (comment)

  2. We can use InteractionManager to focus on input after all interactions instead of using shoudDelayFocus, which can cause issues as mentioned in [HOLD for payment 2022-05-20][$1000] Input is lost upon pressing ⌘K to open the Search View #6069. I have tested the use of InteractionManager in several scenarios, as explained in my proposal here [HOLD for payment 2023-04-04] [$4000] Web - NO focus on Search input field when opening FAB menu and then press on CTRL+K #15276 (comment), and found it to be effective without any negative impact. we can introduce shouldFocusAfterInteraction prop to apply InteractionManager.runAfterInteractions

What alternative solutions did you explore? (Optional)

N/A

@sophiepintoraetz
Copy link
Contributor

@rjharish333 - please email - as per the instructions - to contributors@expensify.com

@tienifr
Copy link
Contributor

tienifr commented Mar 6, 2023

Proposal

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

Placeholder gets selected and search input is not focused when double-clicking on the Search icon.

What is the root cause of that problem?

There're 2 problems here:

  1. The placeholder is currently selectable, so when we double click on the search input it will make the placeholder show the selected style

If we check other messaging apps like Slack & WhatsApp, the placeholder of text input is never selectable at all, so I think we should do the same.

  1. The search input it not focused, this is because when we double click, the second click places the focus on the whole screen's div element, causing the search input to blur (and not focused as we expect)

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

To fix 1, we need to introduce a didTransitionEnd prop to the OptionsSelector to indicate that the page transition is completed.
Then we need to add the userSelectNone to the OptionsSelector's TextInput here


like:

inputStyle={(!this.props.didTransitionEnd) ? [styles.userSelectNone] : []}

This will make sure that during page transition, the placeholder does not show selected style.

We can optionally use the styles.userSelectNone for TextInput in all cases (not just the above case) since it's consistent with other messaging apps like Slack & WhatsApp.

We can pass the didTransitionEnd into the OptionsSelector by using this didScreenTransitionEnd property of the screen

{({didScreenTransitionEnd, safeAreaPaddingBottomStyle}) => (

For the above to work, this Safari hack will also needs to be removed #3613, I've tested and confirmed that removing the hack will not reintroduce the bug (that the hack is fixing) with the latest code in main.

To fix 2, we need to add a logic to the OptionsSelector's componentDidUpdate to focus the input only after the page transition end like:

if (this.props.didTransitionEnd && !prevProps.didTransitionEnd && this.props.autoFocus) {
            this.textInput.focus();
        }

We need both fixes, because if we only fix 2 but not 1, the selectable style of the TextInput will flicker.

What alternative solutions did you explore? (Optional)

We can use shouldDelayFocus to delay the search input focus (and use to control its styles.userSelectNone style).

Result

Working well after the fix:

Screen.Recording.2023-03-06.at.16.27.45.mp4

@mollfpr
Copy link
Contributor

mollfpr commented Mar 6, 2023

Thank you for the proposals! I'll review this after finishing a prioritized PR.

@Ollyws
Copy link
Contributor

Ollyws commented Mar 6, 2023

Proposal

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

The placeholder is selected on double clicking the search icon.

What is the root cause of that problem?

If you double click on other buttons that open a modal, for example the Settings button you will see that it will simply opens and then closes the settings modal (very quickly), however when the Search button is double-clicked instead of the second click closing the modal it causes text to be selected.

The reason it doesn't close on the second click seems to be the presence of the textInput.

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

In my opinion it's better to match the behaviour of the Settings button and the modal should simply open on the first click and close on the second click.

At the moment we are delaying the rendering of the OptionsList using didScreenTransitionEnd, this displays a FullScreenLoadingIndicator along with a textInput until didScreenTransitionEnd is finished.

In my opinion rendering the textInput before the OptionsList is rendered is unnecessary and causes the unwanted behaviour we are seeing, so I propose we don't render the OptionsSelector component at all until didScreenTransitionEnd is true, and instead we render a FullScreenLoadingIndicator (as we already do in BaseOptionsSelector)

This can be easily implemented by wrapping the OptionsSelector in something like:

{didScreenTransitionEnd ? ( ... ) : <FullScreenLoadingIndicator />}

and removing this line.

Video of my proposed behaviour
searchexample.mp4

@muhammadbahroz
Copy link

Proposal

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

Double clicking on search button selects placeholder and unable to type #15632

What is the root cause of that problem?

Upon Double clicking on search button, on chrome and only on the macOS, the placeholder text gets highlighted and input gets disabled due to focus shift during the transition as the data from the search endpoint have also not been rendered yet in that time span.

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

We should capture the transition of the side modal, during transition and when everything have not been rendered yet we should disable the placeholder or we can delay the focus.

What alternative solutions did you explore? (Optional)

A quick fix would be to disable the double click on the search button since it certainly does nothing and then a delayed followed click should close the modal.

@MelvinBot
Copy link

📣 @muhammadbahroz! 📣

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. 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.
  2. 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.
  3. 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>

@muhammadbahroz
Copy link

Contributor details
Your Expensify account email: bahrozahmad1997@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01b5fd9245a2a9e38f

@MelvinBot
Copy link

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@MariaHCD
Copy link
Contributor

MariaHCD commented Mar 7, 2023

@sophiepintoraetz @techievivek @mollfpr Coming from #15692, I think the expected behavior in these two issues are conflicting.

In #15692, we're proposing the expected behavior should be that double clicking the search button opens and immediately close both drawers (similar to double-clicking the LHN avatar)

Thoughts?

@getusha
Copy link
Contributor

getusha commented Mar 7, 2023

I think both issues are similar just the way it was presented and the expected behavior differs, if i am not mistaken we should close the one

@MariaHCD
Copy link
Contributor

MariaHCD commented Mar 7, 2023

👍🏼 Once we agree on the expected behavior, one of these issues should be closed.

I'm personally leaning towards mimicking the same behavior as when double-clicking the LHN avatar. But we'll wait on @sophiepintoraetz @techievivek @mollfpr to weigh in!

@BhavyaKoshiya
Copy link

@MariaHCD expected behavior is a matter of perspective but this issue was reported first so even if you close one issue don't you think the other issue should be closed

@MelvinBot
Copy link

📣 @BhavyaKoshiya! 📣

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. 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.
  2. 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.
  3. 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>

@allroundexperts
Copy link
Contributor

👍🏼 Once we agree on the expected behavior, one of these issues should be closed.

I'm personally leaning towards mimicking the same behavior as when double-clicking the LHN avatar. But we'll wait on @sophiepintoraetz @techievivek @mollfpr to weigh in!

@MariaHCD For a little more context, please check this thread. This basically contains the reason why we are not using the full screen loader and showing the search input while the drawer is transitioning.

@BhavyaKoshiya
Copy link

Contributor details
Your Expensify account email: bhavyakoshiya555@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01a4213e283ebb2d46

@MelvinBot
Copy link

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@getusha
Copy link
Contributor

getusha commented Mar 7, 2023

Also if we decide to go with any of the above proposals we should put this on HOLD since there is firstly proposed solution (and all the proposals here are the same approach as it), which likely solve both issues here #15276 (comment)

cc @sophiepintoraetz @techievivek @mollfpr

@techievivek
Copy link
Contributor

Thanks, @MariaHCD, for bringing this to the discussion. I think we can close this issue because we don't want to fix the behaviour here allowing users to type after a double click. The solution that appeals to me is just to close the right-hand panel. Any thoughts?

@techievivek
Copy link
Contributor

Aaah, I see you have suggested the same here.

@sophiepintoraetz @mollfpr any thoughts?

@mollfpr
Copy link
Contributor

mollfpr commented Mar 7, 2023

I agree with @MariaHCD that the expected result in #15692 is the correct behavior.

@sophiepintoraetz
Copy link
Contributor

I'm actually going to take this conversation internally just to make sure we're being consistent in following programme expectations. Bear with me!

@sophiepintoraetz
Copy link
Contributor

sophiepintoraetz commented Mar 8, 2023

Thanks for your patience all! Given that #15692 has the expected behaviour outlined, I am going to close this issue in favour of that one.

If you have made a proposal here and think your proposal will also fix #15692 - please add it there and @s77rt will review them! (@allroundexperts @getusha @muhammadbahroz)

Additionally, the reporting bonus will be paid out to both @BhavyaKoshiya and @tienifr given that Conor and I missed the similarities between the two issues.

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 Engineering 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
None yet
Development

No branches or pull requests