[$250] iOS - Composer - Composer highlight flickers after closing attachment view

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.79-0
Reproducible in staging?: Yes
Reproducible in production?: No
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: iPhone 15 Pro Max / iOS 18.2
App Component: Chat Report View

Action Performed:

1. Launch ND or hybrid app.
2. Go to DM.
3. Send an image to the DM.
4. Go back to LHN and reopen DM (so that composer is auto-focused without keyboard opening).
5. Tap on the image.
6. Tap app back button.

Expected Result:

Composer highlight will be persistant.

Actual Result:

Composer is highlighted, unhighlighted and then highlighted again.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6702848_1735292381914.ScreenRecording_12-27-2024_17-36-29_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021872607582148372045
• Upwork Job ID: 1872607582148372045
• Last Price Increase: 2024-12-27
• Automatic offers:
• shubham1206agra | Contributor | 105502719
• ikevin127 | Contributor | 105503621

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

Triggered auto assignment to @Beamanator (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

[melvin-bot]

Triggered auto assignment to @sonialiap (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.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

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

[melvin-bot]

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

[Beamanator]

This doesn't look big enough to be a blocker. Also I wonder if this was caused by #54189 🤔 just cuz it looks like a similar topic 😅

Though it doesn't look it necessarily affects the main composer which is what this issue is about 🤔

cc @QichenZhu @brunovjk in case y'all can take a look

[brunovjk]

Thanks @Beamanator, I will take a look now

[Beamanator]

Thanks!!

[brunovjk]

@Beamanator I reverted our PR, but the issue here is still reproducible, IMO it doesn't seem related to our changes. I will continue investigating to find the root cause and a solution. Any additional insights are welcome!

[ikevin127]

Proposal

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

Composer highlight (focus state styles) flicker after closing attachment view.

What is the root cause of that problem?

The flicher is caused by the logic implemented by PR #34404, which fixed issue #33466. The problem with the logic implemented by the PR is that in our issue's case, once we close the modal we already have logic in place to re-focus the input, but the additional logic added by the PR does that once again -> refocusing twice which causes our issue.

Details

• in Composer/index.native.tsx the useResetComposerFocus hook is used and shouldResetFocusRef is set to true
• that above implementation causes the useResetComposerFocus hook logic to focus input again as mentioned in the hook comment, which can be verified by commenting out this line which would fix our issue

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

The problem is that we can't simply revert the PR logic since that would reintroduce issue #33466, therefore here's what I propose in order to have both issues fixed.

We have to ensure that the useResetComposerFocus logic is only triggered when the #33466 issue steps are executed, and when our issue steps are executed -> skip useResetComposerFocus logic.

You might ask: How can we distinguish between the report details page being opened from the header while edit input is focused, then closed to allow useResetComposerFocus to execute VS the attachment modal opening while composer is focused, then closed in order to block useResetComposerFocus from executing ?

This is simple since we know that attachment modal is a modal, we can simply use the logic from ComposerWithSuggestions to detect whether a modal was previously opened:

const [modal] = useOnyx(ONYXKEYS.MODAL);
const prevIsModalVisible = usePrevious(modal?.isVisible);

using this, we modify the useResetComposerFocus hook as follows:

// new imports
import {useOnyx} from 'react-native-onyx';
import ONYXKEYS from '@src/ONYXKEYS';
import usePrevious from './usePrevious';

export default function useResetComposerFocus(inputRef: MutableRefObject<TextInput | null>) {
    const isFocused = useIsFocused();
    const shouldResetFocusRef = useRef(false);
    const [modal] = useOnyx(ONYXKEYS.MODAL); // <- add this like
    const prevIsModalVisible = usePrevious(modal?.isVisible); // <- add this like

    useEffect(() => {
        if (!isFocused || !shouldResetFocusRef.current || prevIsModalVisible) { // <- add additional OR check to return early
            return;
        }
        inputRef.current?.focus(); // focus input again
        shouldResetFocusRef.current = false;
    }, [isFocused, inputRef, prevIsModalVisible]);

    return {isFocused, shouldResetFocusRef};
}

this way we fix both issues, take a look:

iOS: Native

Our Issue	The Other Issue
after-main.mp4	after-other.mp4

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

N/A

Result video (before / after)

iOS: Native

Before	After
before.mp4	after-main.mp4

@Beamanator If proposal makes sense, I'd like to work on this as Contributor and have somebody else step in as Reviewer, as per the C+ process doc:

If C+ wants to submit a proposal to fix the issue they do so and tag the internal engineer (by adding “ 🎀👀🎀”) to review the proposal. All other aspects of working on the issue as the same as other contributors (as of Feb 2024 this is very rare)

[melvin-bot]

Current assignee @Beamanator is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[huult]

Proposal

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

Composer highlight flickers after closing attachment view

What is the root cause of that problem?

When I debugged this issue, I observed that it only occurs on native iOS and does not happen on native Android. Additionally, the behavior differs from the web version.

The RCA is that we call textInput focus while the transition and animation are not complete, causing the highlight to appear and then disappear again.

App/src/hooks/useResetComposerFocus.ts

Line 14 in ae1c034

inputRef.current?.focus(); // focus input again

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

To resolve this issue, we just need to delay until the transition and animation are complete, then call focus, like this:

App/src/hooks/useResetComposerFocus.ts

Lines 14 to 15 in ae1c034

	inputRef.current?.focus(); // focus input again
	shouldResetFocusRef.current = false;

Add InteractionManager.runAfterInteractions when calling focus. This issue only occurs on native iOS, and we will implement it as shown below:

1. Create InputFocusResetHandler folder in

https://github.com/Expensify/App/blob/93260a60eac6ce6200ec164fb967529db35ff071/src/libs

• index.ts

  import type InputFocusResetHandler from './types';

  const inputFocusResetHandler: InputFocusResetHandler = {
      handleInputFocusReset: (inputRef, shouldResetFocusRef) => {
          if (!inputRef.current || !shouldResetFocusRef.current) {
              return;
          }

          inputRef?.current?.focus();
          const resetFocusRef = shouldResetFocusRef;
          resetFocusRef.current = false;
      },
  };

  export default inputFocusResetHandler;

• index.ios.ts

  import {InteractionManager} from 'react-native';
  import type InputFocusResetHandler from './types';

  const inputFocusResetHandler: InputFocusResetHandler = {
      handleInputFocusReset: (inputRef, shouldResetFocusRef) => {
          if (!inputRef.current || !shouldResetFocusRef.current) {
              return;
          }

          InteractionManager.runAfterInteractions(() => {
              inputRef?.current?.focus();
              const resetFocusRef = shouldResetFocusRef;
              resetFocusRef.current = false;
          });
      },
  };

  export default inputFocusResetHandler;

• types.ts

  import type {MutableRefObject} from 'react';
  import type {TextInput} from 'react-native';

  type InputFocusResetHandler = {
      handleInputFocusReset: (inputRef: MutableRefObject<TextInput | null>, shouldResetFocusRef: React.MutableRefObject<boolean>) => void;
  };

  export default InputFocusResetHandler;

2. Update useResetComposerFocus hook to:

App/src/hooks/useResetComposerFocus.ts

Lines 6 to 19 in 9412d21

	export default function useResetComposerFocus(inputRef: MutableRefObject<TextInput | null>) {
	const isFocused = useIsFocused();
	const shouldResetFocusRef = useRef(false);
	useEffect(() => {
	if (!isFocused || !shouldResetFocusRef.current) {
	return;
	}
	inputRef.current?.focus(); // focus input again
	shouldResetFocusRef.current = false;
	}, [isFocused, inputRef]);
	return {isFocused, shouldResetFocusRef};
	}

export default function useResetComposerFocus(inputRef: MutableRefObject<TextInput | null>) {
    const isFocused = useIsFocused();
    const shouldResetFocusRef = useRef(false);

    useEffect(() => {
        if (!isFocused || !shouldResetFocusRef.current) {
            return;
        }

        inputFocusResetHandler.handleInputFocusReset(inputRef, shouldResetFocusRef);
    }, [isFocused, inputRef]);

    return {isFocused, shouldResetFocusRef};
}

POC

Screen.Recording.2024-12-28.at.14.26.52.mp4

Test branch

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

I think we don’t need to test this because it is a 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.

[brunovjk]

@Beamanator If proposal makes sense, I'd like to work on this as Contributor and have somebody else step in as Reviewer,

I would love to help with the review if needed.

[shubham1206agra]

Lets get this party started.

@ikevin127's proposal makes sense.

Note (Minor) - It was regression from different PR. See #35572.

Context - 2nd focus is coming from ComposeFocusManager, which was put place in later for advanced focus management when opening/closing modals. It will be really great if we can extend the same logic for RHP and LHP. @adamgrzybowski If you know how to do that, please let us know.

Please assign @ikevin127 as contributor for now.

🎀👀🎀 C+ Reviewed

[melvin-bot]

Current assignee @Beamanator is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[huult]

@shubham1206agra I see this only happens on iOS native and does not occur on Android native. This is because the focus was added later for advanced focus management when opening or closing modals. Therefore, I think delaying it is still a better solution to cover the remaining cases. What do you think?

Screen.Recording.2024-12-30.at.14.12.31.mov

[Beamanator]

@shubham1206agra since you jumped in and reviewed, I will assign you as C+ this time, but curious why you skipped over @brunovjk ? 🤔

[melvin-bot]

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

[Beamanator]

Also @shubham1206agra can you please review @huult 's thoughts and let us know what you think? @ikevin127 i'd be curious for your opinion too if you don't mind

[shubham1206agra]

Oh shit. I didn't see the message. You can assign @brunovjk here if you want for C+

I am really sorry for this.

[shubham1206agra]

@shubham1206agra I see this only happens on iOS native and does not occur on Android native. This is because the focus was added later for advanced focus management when opening or closing modals. Therefore, I think delaying it is still a better solution to cover the remaining cases. What do you think?

Screen.Recording.2024-12-30.at.14.12.31.mov

Nope, I don't think time delays are a good solution.

[Beamanator]

Oh shit. I didn't see the message. You can assign @brunovjk here if you want for C+

I am really sorry for this.

Appreciate it! I would say you just "owe @brunovjk one" or some chocolate or something like that, but y'all can figure it out 😅

[huult]

Nope, I don't think time delays are a good solution.

It’s simpler than checking the modal, RHP, or LHP to return early. If we miss one or the other, this issue will happen again. Anyway, I appreciate your selection. Thanks for your feedback!

[ikevin127]

@ikevin127 i'd be curious for your opinion too if you don't mind

@Beamanator Definitely not for adding delay in this case as that doesn't fix the issue from the root, just patches the symptom. The root being that we focus the composer twice, which we shouldn't do in the first place.

I agree with @shubham1206agra's review and comments on the other proposal.

[brunovjk]

Oh shit. I didn't see the message. You can assign @brunovjk here if you want for C+
I am really sorry for this.

Appreciate it! I would say you just "owe @brunovjk one" or some chocolate or something like that, but y'all can figure it out 😅

Hey team, no worries on my end about the review! I’m still available to help if needed but I’m confident @shubham1206agra will do a great job as well 🚀 🚀 Thank you @Beamanator ❤️

[melvin-bot]

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

[Beamanator]

Alright let's get goin' w/ @ikevin127 's proposal! Thanks all!

[ikevin127]

Will open the PR sometime today (PST), thanks everyone!

[ikevin127]

PR #54670 is now ready for review! 🚀