[HOLD for payment 2023-10-27] [$500] Focus does not shift directly from "Manual" option to "Distance" option

[kavimuru]

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

---

Action Performed:

1. Click on the FAB menu
2. Select "Request money"
3. Choose "Manual > Choose "Distance"

Expected Result:

The focus should seamlessly transition to "Distance" option

Actual Result:

The focus unexpectedly jumps to "Scan" option

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 / Safari
• MacOS / Desktop

Version Number: 1.3.70-5
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: Any additional supporting documentation

81e4953d-72f3-4184-87df-691373f8670c.MP4

az_recorder_20230915_155305.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @aman-atg
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694026046030719

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a90de5f335cc93e3
• Upwork Job ID: 1702773443318988800
• Last Price Increase: 2023-10-06
• Automatic offers:
• ahmedGaber93 | Contributor | 27116604
• aman-atg | Reporter | 27116605

Issue Owner

Current Issue Owner: @sakluger

[ahmedGaber93]

Proposal

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

Focus does not shift directly from "Manual" option to "Distance" option

What is the root cause of that problem?

App/src/components/TabSelector/TabSelector.js

Lines 76 to 86 in b11bddc

	const getBackgroundColor = (position, routesLength, tabIndex) => {
	if (routesLength > 1) {
	const inputRange = Array.from({length: routesLength}, (v, i) => i);
	return position.interpolate({
	inputRange,
	outputRange: _.map(inputRange, (i) => (i === tabIndex ? themeColors.midtone : themeColors.appBG)),
	});
	}
	return themeColors.midtone;
	};

In getBackgroundColor and getOpacity we set them base on position by position.interpolate.
You can notice by slowly swipe tabs like the video

Screen.Recording.2023-09-10.at.4.52.45.PM.mov

And in this issue case, when we move from the tab index 0 to 2 by click, the tabs swipe horizontal and position.interpolate will change, and this change will affect on the three animation style.
The change for tab index 1 will be (note we move from tab 0 to tab2 so tab 1 should not affect)

// tab 1 background
outputRange: _.map(inputRange, (i) => (i === tabIndex ? themeColors.midtone : themeColors.appBG)), 
// the tab index is 1 so the result will be
outputRange: [themeColors.appBG, themeColors.midtone, themeColors.appBG]

so while the swipe from tab 0 to tab 2 the tab 1 will flicker in the middle

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

To fixing this issue without change any animation, we need to apply the animation to affected tabs only, and prevent other tab from changing its style.

For example, while we move from tab1 to tab3 by click, the affected tabs are [tab1, tab3] only.

For that we can do the following:

1. the default affected tabs will be all tabs thats for swipe.
2. when tab click set affected tabs [currentTab, clickedTab].
3. after transition end back affected tabs to default to be all tabs.

The animation outputRange will be

// outputRange: _.map(inputRange, (i) => (i === tabIndex ? activeValue : inactiveValue))
outputRange: _.map(inputRange, (i) => (affectedTabs.includes(tabIndex) && i === tabIndex ? activeValue : inactiveValue))

when tab press set affected tabs = [currentTab, clickedTab]

// using ref rather than state here to prevent any lags and the `TabSelector` will already re-render when navigation state change
affectedAnimatedTabs.current = [state.index, index];

reset affected tabs after transition end

    const [, reRender] = useReducer(x => !x, false);

    const defaultAffectedAnimatedTabs = Array.from({length: state.routes.length}, (v, i) => i);
    const affectedAnimatedTabs = useRef(defaultAffectedAnimatedTabs);


    React.useEffect(() => {
        setTimeout(() => {
            affectedAnimatedTabs.current = defaultAffectedAnimatedTabs;
            // re-render is important to re-defining opacity and background base on new affected tabs, to be ready for user swiping.
            reRender();
        }, CONST.ANIMATED_TRANSITION)
    }, [state.index]);

branch

result

20231005103606957.mp4

old proposal

I think using position.interpolate is not a good idea to use with backgroundColor and opacity when tabs length > 2 because it works good when swipe between tabs and not good from jump between Non-adjacent tabs.

Solution 1: use translateX with position.interpolate and this is the good usage of it like whats app, this will work fine swipe and clicks

const getTranslateX = (position, routesLength, TabWidth) => {
    const inputRange = Array.from({length: routesLength}, (v, i) => i);
    return position.interpolate({
        inputRange,
        outputRange: _.map(inputRange, (i) => i * TabWidth),
    });
};

function TabSelector({state, navigation, onTabPress, position}) {
    const {translate} = useLocalize();

    const [width, setWidth] = React.useState(0);
    const TabWidth = (width - 40) / state.routes.length;


    return (
        <View onLayout={(event)=> setWidth(event.nativeEvent.layout.width)} style={styles.tabSelector}>
            <Animated.View
                style={{
                    position : 'absolute',
                    width : TabWidth,
                    height : 40,
                    borderRadius : 20,
                    backgroundColor : themeColors.border,
                    transform: [{translateX: getTranslateX(position, state.routes.length, TabWidth)}]
                }} />


            {_.map(state.routes, (route, index) => {
                ...
                return (
                    <TabSelectorItem
                        ...
                        // activeOpacity={activeOpacity}
                        // inactiveOpacity={inactiveOpacity}
                        // backgroundColor={backgroundColor}
                    />
                );
           ...

Screen.Recording.2023-09-10.at.5.46.31.PM.mov

Solution 2: use backgroundColor and opacity directly in props, this will work fine in clicks but not affected by slow swipe because there is no position.interpolate here

<TabSelectorItem
    ...
    activeOpacity={isFocused ? 1 : 0}
    inactiveOpacity={isFocused ? 0 : 1}
    backgroundColor={isFocused ? themeColors.midtone : themeColors.appBG}
/>

Screen.Recording.2023-09-10.at.6.01.44.PM.mov

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Pluto0104]

This is an issue with the tab navigator library. There are numerous problems related to it.
#26407
#26516

For now, this issue is expected.

[ahmedGaber93]

@Pluto0104 this issue not related to the mentioned issues above.
This issue when move by clicking the tab bar item from tab 0 to tab 2 the tab 1 is flickering.

[Pluto0104]

@Pluto0104 this issue not related to the mentioned issues above.

I didn't mention that this issue is related to other problems. What I meant is that the underlying cause for all these problems is the module issue.

If you don't update the module, you won't be able to fully resolve this issue. Furthermore, your proposed solution is incorrect. It only addresses the tab selector background color. You can observe that the tab content is misaligned when switching from 'manual' to "distance".

[Pluto0104]

@aldo-expensify @s77rt how do you think about this problem?

[ahmedGaber93]

What I meant is that the underlying cause for all these problems is the module issue.

I still disagree with you about this issue root cause, but let us wait for C+ to decide about that.
Thanks!

[Pluto0104]

Hey @ahmedGaber93, I noticed that I can still see this issue in your video demonstrating your solution. The issue is visible not only in the tab selector but also in the tab content. Please take a closer look.

[s77rt]

@Pluto0104 I think it's natural that the tab content gets visible for a brief moment since we are passing by it. The tab styling however feels broken, it feels like we go to step 2 stop for a brief moment then continue. The tab animation is from our side so I think we can fix that. The animation provided by @ahmedGaber93 in his first solution looks better but it's missing the opacity.

Bad	Good

[sakluger]

The animation provided by @ahmedGaber93 in his first solution looks better but it's missing the opacity.

@s77rt should @ahmedGaber93 update their proposal to fix the opacity?

[aldo-expensify]

@aldo-expensify @s77rt how do you think about this problem?

Thanks @Pluto0104 for helping around.

My opinion on this is that this is not a bug, I don't see a problem with the tab in the middle to become "focused" as we move from Distance to Manual. The example from the library page does exactly that: https://snack.expo.dev/?platform=web

Screen.Recording.2023-09-18.at.5.56.01.PM.mov

Or am I getting it wrong what is described here as a bug?

[ahmedGaber93]

I don't see a problem with the tab in the middle to become "focused" as we move from Distance to Manual. The example from the library page does exactly

@aldo-expensify I think the example is just a fast demo for explanation, but famous apps don't do that.

[burczu]

Ok. This might be a regression from this PR: #26022, but during the discussion in the issue we agreed not to change anything with animation while switching tabs, so I think this may not be a bug at all.

cc @shawnborton @allroundexperts

[s77rt]

@sakluger

should @ahmedGaber93 update their proposal to fix the opacity?

Yes, I think so. But @shawnborton may be the best to ask about the expected behaviour here.

[s77rt]

@aldo-expensify The item being focused is okay I think (or not too much concerning) but the background pill feels broken on our App. In your linked video check the bottom blue bar, it scrolls smoothly, the background pill should do the same instead of partially fading.

Check #27563 (comment)

[shawnborton]

Ah interesting. I agree that it is weird that the middle tab gets a flash highlight when navigating to the outer tab. @AndrewGable I think you implemented this, any idea what is going on here?

cc @neil-marcellini as well.

[shawnborton]

In terms of expected behavior, yes, I would imagine clicking on a tab just highlights the new tab without highlighting the middle tab first

[pecanoro]

Sounds good, reviewed! Assigning @ahmedGaber93

[melvin-bot]

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

[melvin-bot]

📣 @aman-atg 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[ahmedGaber93]

@burczu PR #29231 is ready for review.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @ahmedGaber93 got assigned: 2023-10-10 09:06:50 Z
• when the PR got merged: 2023-10-18 16:26:08 UTC
• days elapsed: 6

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.87-12 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix money request tabs animation #29231

If no regressions arise, payment will be issued on 2023-10-27. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @burczu does not require payment (Contractor)
• @ahmedGaber93 requires payment offer (Contributor)
• @aman-atg requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR:
• [@burczu] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@burczu] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@burczu] Determine if we should create a regression test for this bug.
• [@burczu] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sakluger]

I completed payments for @aman-atg and @ahmedGaber93.

@burczu do you mind completing the BZ checklist before we close the issue? Thanks!

[sakluger]

Trying to remove overdue, one more comment should do it.

[burczu]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR: I don't think this is a regression - the animation while switching tabs was just not correctly done since the beginning.
• [@burczu] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: n/a
• [@burczu] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: n/a
• [@burczu] Determine if we should create a regression test for this bug. I don't think we need regression test here - the bug wasn't impactful on any important flow of the and it was just UI/UX fix.
• [@burczu] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sakluger]

Thanks!