[HOLD for payment 2023-12-07] [$2000] magic code input remains highlighted even after not being focused

[kavimuru]

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. go to login page
2. enter email and click on continue button
3. click on outside of the input field to lose focus

Expected Result:

input should not be highlighted after losing focus

Actual Result :

input remains highlighted even after losing focus

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
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: dev Reproducible in staging?: needs reproduction Reproducible in production?: needs reproduction 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: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/235510438-ae9f3979-18c0-4bfb-b55a-c55837d1c30f.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01790579d65d182333
• Upwork Job ID: 1653522863070584832
• Last Price Increase: 2023-05-15

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Christinadobrzyn]

Looks like we have a few focus/cursor issues but I think this one is different. I assume what we want is either

• The cursor to remain on the "Phone or email" line even when clicking the other parts of the landing page
• Prohibit highlighting the landing page as we do with old dot and on the mobile apps with NewExpensify

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[chafikchaban]

Proposal

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

magic code input is autofocused

What is the root cause of that problem?

a few things combined:

• MagicCodeInput's initial state value for focusedIndex is 0 ( index of the first input which gets highlighted ).
• in BaseValidateCodeForm we not only add autoFocus prop to the MagicCodeInput component but also insist on auto focus by invoking MagicCodeInput's focus() method manually when the component mounts

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

  two things:

• initialise focusedIndex state as undefined ( no input is initially focused )
• pass autoFocus prop to MagicCodeInput with the value of false
• remove this.inputValidateCode.focus() call on BaseValidateCodeForm's componentDidMount level

  What alternative solutions did you explore? (Optional)

  none.

Result

https://user-images.githubusercontent.com/30408070/235804446-21a3f157-caa2-44d6-8fd8-3285e60794ca.mov

[alitoshmatov]

Proposal

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

DEV: magic code input remains highlighted even after not being focused

What is the root cause of that problem?

We are just setting focus when pressed or mounted, but we should also remove focus when onblured

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

We should add onBlur method to magic code input. In this method we should check if focus is really being sent outside of input groups like this: _.includes(this.inputRefs, e.nativeEvent.relatedTarget) and if it does, set focusedIndex and editIndex to undefined. If we do not check for the focus destination, when entering numbers focus will flicker, and also deleting numbers won't work as expected as well.

What alternative solutions did you explore? (Optional)

[situchan]

Proposal

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

Magic code input remains highlight even after being blurred

What is the root cause of that problem?

We're setting highlight when pressed but no logic to remove highlight when blurred

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

Add onBlur callback on TextInput in MagicCodeInput. In this callback, if this.inputRefs doesn't contain e.nativeEvent.relatedTarget, clear focusedIndex, editIndex to undefined. This condition is needed for left, right arrows to work. We should not clear focusedIndex, editIndex values when focus is transferred from one input to another input. They should be cleared only when all 6 inputs are blurred.

Improvement: to make Tab/Shift+Tab key work similar to left/right arrow, use onFocus callback instead of onPress callback and deprecate old onFocus callback.

Note: my proposal cannot be similar to @alitoshmatov's proposal. Here's the first reason: https://github.com/Expensify/App/issues/15486#issuecomment-1480119094

[Santhosh-Sellavel]

@situchan What's different in your proposals?

As far as I can see the @alitoshmatov proposed the solution first here even including edits.

[situchan]

yes, I had thought if edited proposal was earlier, it would have been acceptable. But in https://github.com/Expensify/App/issues/15486, my proposal was not accepted even though my edited proposal was earlier than accepted proposal, only because I didn't post new comment "Proposal updated".

[situchan]

cc: @marcochavezf @mallenexpensify as you were assigned on related issue

[alitoshmatov]

I just thought it was ok since there were not any proposals after mine at the time when I was updating

[Santhosh-Sellavel]

I do not have the full context of #15486. Just my thoughts on this matter.

It's okay to edit the proposal before C+ starts reviewing. But once the proposal review has started it's important to leave an updated comment message. If the changes are just improving the existing approach it's okay to edit, But if the approach changes or more changes it's better to leave a new comment mentioningProposal 2orupdated proposal at the start of the comment.

@situchan I think this is not the place to continue this discussion here. If still something unresolved please start a discussion in Slack in Expensify-bugs or Expensify-open-source, thanks!

[situchan]

Here's is full context ☝️

I don't mind which proposal is accepted here but just for consistency between issues so no one has doubt.

[Santhosh-Sellavel]

As I mentioned early here

@situchan I think this is not the place to continue this discussion here. If still something unresolved please start a discussion in Slack in Expensify-bugs or Expensify-open-source, thanks!

Thanks again!

[aimane-chnaif]

I suggest to hold this for https://github.com/Expensify/App/pull/18398 where MagicCodeInput component is being refactored to functional component.

[dangrous]

I don't think it'll hugely matter either way since it'll come up as merge conflicts for whoever tries to push first but yeah let's hold for now so we don't have to make changes twice (not a big issue so the delay will be fine)

[Christinadobrzyn]

Can we move this toweekly while it's on hold? Just so we don't get overdue notifications.

[Christinadobrzyn]

Looks like this is getting worked on - https://github.com/Expensify/App/pull/18398 - might be able to move back to this soon?

cc @aimane-chnaif @Santhosh-Sellavel

[Santhosh-Sellavel]

Not sure @aimane-chnaif what should we do here?

[dangrous]

The functional component is on main so we can get to updating (if needed), testing, and choosing proposals! Taking this off hold and bringing back to daily

[Santhosh-Sellavel]

Should we do something about this? @Expensify/design

[shawnborton]

We should treat these inputs like any other inputs, so yeah, if there is no focus, there shouldn't be focus styles.

[aimane-chnaif]

Not sure @aimane-chnaif what should we do here?

Sorry for late. Now that component refactor is merged, this issue can be resumed.

[chafikchaban]

@Santhosh-Sellavel Should we discuss my proposal then ?

[Santhosh-Sellavel]

Thanks @shawnborton!

[Santhosh-Sellavel]

@chafikchaban First thing we need autofocus here removing that is a bad idea. And your proposal doesn't address the issue when there is a focus on the input, the user clicks else where it gets reset it.

[Santhosh-Sellavel]

@alitoshmatov Can you elaborate on your proposal and share snippets, please?

Thanks!

[alitoshmatov]

@Santhosh-Sellavel So basically we want to lose focus when outside of input is clicked, to achieve this we are going to lose focus in each individual input when onblur is triggered, here https://github.com/Expensify/App/blob/676ca3bfbdd316d9d24510f13a20bde6844d33ff/src/components/MagicCodeInput.js#L270-L294 The highlighted input is represented by this state https://github.com/Expensify/App/blob/676ca3bfbdd316d9d24510f13a20bde6844d33ff/src/components/MagicCodeInput.js#L84 We want to set it to undefined when focus is lost But we should also consider that we are manually focusing another input in a lot of instances. And when focus is called in next focus input onblur is going to be called to the current focused input. To prevent focused state from being: A -> undefined -> B and flickering effect, we should check if onblur is called outside of magic code input elements. We can check if onBlur is getting called due to focus event, and if so which is the target of this focus event like this:

onBlur={(e)=>{
    if(_.includes(this.inputRefs, e.nativeEvent.relatedTarget)){
      //Do nothing since another input is focused and focused state is going to be handled on another place
    } else {
    setFocusedIndex(undefined); // Not any input elements is focus, thus do not highlight anything
      }
  }
}

I hope it was helpful

[melvin-bot[bot]]

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

[Santhosh-Sellavel]

@alitoshmatov It does not work for me when clicking outside it's still focused

[alitoshmatov]

@Santhosh-Sellavel It looks like the code I provided to you is not working since component has been refactored to functional component. Try this one:

onBlur={(e)=>{
    if(_.includes(inputRefs.current, e.nativeEvent.relatedTarget)){
      // Do nothing since another input is focused and focused state is going to be handled on another place
      } else {
       setFocusedIndex(undefined); // Not any input elements is focus, thus do not highlight anything
       }
     }}

But logic still holds

[Santhosh-Sellavel]

@alitoshmatov Sorry, I missed that today. Yeah, seems to be working now.

We broke this here When switching between tabs here focus is lost. But we need to set back the focus when switching back again.

[dangrous]

updating the price here, looks like we're close though!

[melvin-bot[bot]]

Upwork job price has been updated to $2000

[alitoshmatov]

@Santhosh-Sellavel

We broke this here

I think you wanted to link something, but it is not. Can you provide the link

[Santhosh-Sellavel]

I meant this @alitoshmatov

When switching between tabs here focus is lost. But we need to set back the focus when switching back again.

[alitoshmatov]

@Santhosh-Sellavel It looks like we regain the focus when tab switches, if you type after switching tabs it will work, but we are not setting highlight when input is automatically focused after switching tabs.

Solution: we should set highlight when onFocus is called here https://github.com/Expensify/App/blob/478de9c4dcc8d7ffa11de0f29df9cda53f2ab7ba/src/components/MagicCodeInput.js#L140-L149

Like this:

const onFocus = (event, index) => {
        event.preventDefault();
        setInput('');
        setFocusedIndex(index);
        setEditIndex(index);
    };

[alitoshmatov]

if you are trying above code you should also change this line to pass both event and index https://github.com/Expensify/App/blob/478de9c4dcc8d7ffa11de0f29df9cda53f2ab7ba/src/components/MagicCodeInput.js#L297

Also I think this line is misleading, when I press on input it is triggering onPress event not onFocus(It might not be the case in mobile) https://github.com/Expensify/App/blob/478de9c4dcc8d7ffa11de0f29df9cda53f2ab7ba/src/components/MagicCodeInput.js#L140-L145

[Santhosh-Sellavel]

I got that but the current index is not focused correctly when trying your solution.

[Santhosh-Sellavel]

There is a separate issue for the wrong input box being picked when switching tabs but we have a solution for that here. Applying yours kind of breaks that!

[getusha]

Proposal

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

Input remains highlighted when blurring the input by clicking away

What is the root cause of that problem?

There is currently no handling for blur using onBlur to update the state

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

1. Add onBlur and set the focusedIndex to undefined.
2. instead of using onPress to set the focusedIndex we should use onFocus to set the focused index because when we return from another tab the previously focused input will be focused and onFocus will set the correct focusedIndex at the time.

https://github.com/Expensify/App/blob/8206180c5113ff68e11e9d7334a9eade69ae2d10/src/components/MagicCodeInput.js#L330

// pass the index to onFocus
onFocus={(e) => onFocus(e, index)}

// set focused index right after the focus
const onFocus = (event, index) => {
      event.preventDefault();
      setFocusedIndex(index);
      setEditIndex(index);
};

Also when entering a number will switch the focusIndex to the next one but not focusing the actual input so we should also add the following code to focus the right updated input. here:

https://github.com/Expensify/App/blob/8206180c5113ff68e11e9d7334a9eade69ae2d10/src/components/MagicCodeInput.js#L208-L210

if (!_.isUndefined(updatedFocusedIndex)) {
    inputRefs.current[updatedFocusedIndex].focus();
}

Result

https://github.com/Expensify/App/assets/75031127/3cc53260-6271-4afe-bfa1-b36cd261fba5

What alternative solutions did you explore? (Optional)

None

[Santhosh-Sellavel]

Everyone, please include complete solutions/snippets so we can quickly test this one as this could be a regression prone issue, thanks!

[getusha]

@Santhosh-Sellavel should i post the diff?

[Santhosh-Sellavel]

No just important snippets, with explanations

[getusha]

we can use state or a ref both will work define prevFocusedRef using useRef above.

                               onBlur={() => {
                                   prevFocusedRef.current = focusedIndex;
                                   setFocusedIndex(undefined);
                               }}

                               onFocus={(e) => {
                                   setFocusedIndex(prevFocusedRef.current);
                                   onFocus(e);
                               }}

[melvin-bot[bot]]

@dangrous @Christinadobrzyn @Santhosh-Sellavel 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!

[Christinadobrzyn]

hey @Santhosh-Sellavel how are you feeling about these proposals? Any update on accepting one?