[$250] Attachment - Wrong video playback speed is highlighted

[lanitochka17]

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: 1.4.74.4
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing https://github.com/Expensify/Expensify/issues/383873

Action Performed:

1. Log in with any account
2. Open any chat or room
3. Upload a video attachment
4. Play the video
5. Click on the "..." button
6. Click on the "Playback speed" button
7. Change the speed
8. Click on the "Playback speed" button again

Expected Result:

The current playback speed should be highlighted

Actual Result:

Wrong video playback speed is highlighted. There is no highlight on Android and iOS app

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

Bug6486124_1716205749562.bandicam_2024-05-20_13-36-09-159.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bf77b70eb73d8074
• Upwork Job ID: 1793174201872076800
• Last Price Increase: 2024-06-26
• Automatic offers:
• Krishna2323 | Contributor | 102998073

Issue Owner

Current Issue Owner: @abdulrahuman5196

[melvin-bot]

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

[lanitochka17]

@JmillsExpensify FYI 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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[Krishna2323]

Proposal

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

Attachment - Wrong video playback speed is highlighted

What is the root cause of that problem?

The focused index in not updated correctly when currentMenuItems gets changed and isSelected isn't passed in subMenuItems option.

App/src/components/PopoverMenu.tsx

Lines 102 to 111 in e8ae3c5

	const selectItem = (index: number) => {
	const selectedItem = currentMenuItems[index];
	if (selectedItem?.subMenuItems) {
	setCurrentMenuItems([...selectedItem.subMenuItems]);
	setEnteredSubMenuIndexes([...enteredSubMenuIndexes, index]);
	} else {
	selectedItemIndex.current = index;
	onItemSelected(selectedItem, index);
	}
	};

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

Lines 60 to 67 in e8ae3c5

	subMenuItems: CONST.VIDEO_PLAYER.PLAYBACK_SPEEDS.map((speed) => ({
	icon: currentPlaybackSpeed === speed ? Expensicons.Checkmark : undefined,
	text: speed.toString(),
	onSelected: () => {
	updatePlaybackSpeed(speed);
	},
	shouldPutLeftPaddingWhenNoIcon: true,
	})),

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

1. Pass isSelected property in subMenuItems option.

isSelected: currentPlaybackSpeed === speed,

2. Update selectItem function to update the focused index whenever currentMenuItems gets changed.

    const selectItem = (index: number) => {
        const selectedItem = currentMenuItems[index];
        if (selectedItem?.subMenuItems) {
            const selectedItemIndexH = selectedItem?.subMenuItems.findIndex((option) => option.isSelected);
            setCurrentMenuItems([...selectedItem.subMenuItems]);
            setEnteredSubMenuIndexes([...enteredSubMenuIndexes, index]);
            setFocusedIndex(selectedItemIndexH);
        } else {
            selectedItemIndex.current = index;
            onItemSelected(selectedItem, index);
        }
    };

What alternative solutions did you explore? (Optional)

We can use useEffect to update the focused index.

    useEffect(() => {
        setFocusedIndex(-1);
        const selectedItemIndexH = currentMenuItems.findIndex((option) => option.isSelected);
        setFocusedIndex(selectedItemIndexH ?? -1);
    }, [currentMenuItems, setFocusedIndex]);

Alternative solution 2

If we don't want to highlight the selected option then we can just call setFocusedIndex(-1); inside selectItem callback.

App/src/components/PopoverMenu.tsx

Lines 102 to 111 in e8ae3c5

	const selectItem = (index: number) => {
	const selectedItem = currentMenuItems[index];
	if (selectedItem?.subMenuItems) {
	setCurrentMenuItems([...selectedItem.subMenuItems]);
	setEnteredSubMenuIndexes([...enteredSubMenuIndexes, index]);
	} else {
	selectedItemIndex.current = index;
	onItemSelected(selectedItem, index);
	}
	};

We already clear the index when selecting the option by pressing enter.

App/src/components/PopoverMenu.tsx

Lines 148 to 152 in e8ae3c5

	if (focusedIndex === -1) {
	return;
	}
	selectItem(focusedIndex);
	setFocusedIndex(-1); // Reset the focusedIndex on selecting any menu

[tienifr]

Proposal

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

Wrong video playback speed is highlighted. There is no highlight on Android and iOS app

What is the root cause of that problem?

• When we are in Download | Playback speed menu, click on Playback speed option will set the focused index to 1 in here.

• Then, the Playback speed menu opens. In here, the focused index is still 1 as we set before, leads to the current bug.

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

• We need to set onMouseDown={(e)=>e.preventDefault()} in here. We already have this logic in a lot of logics, such as here to prevent app from focusing on the mouse down option.

• We can also create a prop named shouldPreventDefaultFocusOnSelectRow to just apply the change e.preventDefault() if shouldPreventDefaultFocusOnSelectRow: true

What alternative solutions did you explore? (Optional)

NA

[JmillsExpensify]

Opening up to the community.

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

Proposal Updated

• Added alternative 2

[hurram-dev]

Proposal

Please re-state the problem that we are trying to solve in this issue.
While playing attachment video, wrong video playback speed is highlighted on the web. There is no highlight on Android and iOS app.

What is the root cause of that problem?
The actual cause for this problem is focus state is not changing according to selected menu. its value is persisting the initial value that is equal to index of 0.5 speed. So, the highlight of menu item is not working after selecting the menu item

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

 const selectItem = (index: number) => {
        const selectedItem = currentMenuItems[index];
        if (selectedItem?.subMenuItems) {
            setCurrentMenuItems([...selectedItem.subMenuItems]);
            setEnteredSubMenuIndexes([...enteredSubMenuIndexes, index]);
            
            setFocusedIndex(index) //this state needs to be updated after selecting item and it should only work for subMenutItems since focused index is not needed for normal menu item
        } else {
            selectedItemIndex.current = index;
            onItemSelected(selectedItem, index);
        }
    };

What alternative solutions did you explore? (Optional)

Another solution for this problem would be passing setFocusedIndex to

App/src/components/PopoverMenu.tsx

Line 160 in e8ae3c5

currentMenuItems[selectedItemIndex.current].onSelected?.();

this above function call and update focused index according to selected speed menu item index to get it highlighted

From this App/src/components/VideoPlayerContexts
/VideoPopoverMenuContext.tsx file i would change this code as following

 items.push({
            icon: Expensicons.Meter,
            text: translate('videoPlayer.playbackSpeed'),
            subMenuItems: CONST.VIDEO_PLAYER.PLAYBACK_SPEEDS.map((speed, index) => ({
                icon: currentPlaybackSpeed === speed ? Expensicons.Checkmark : undefined,
                text: speed.toString(),
                onSelected: (setFocusedIndex //receiving setstate from argument) => {
                    updatePlaybackSpeed(speed);
                    
                    setFocusedIndex(index); //while updating the speed, it also updates menu focus as well and get highlighted
                },
                shouldPutLeftPaddingWhenNoIcon: true,
            })),
        });

Let me know, if this proposal seems to work

[melvin-bot]

📣 @hurram-dev! 📣
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.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[hurram-dev]

Contributor details
Expensify account email: [email protected]
Upwork Profile: https://www.upwork.com/freelancers/~0130a0ff1a64915163

[melvin-bot]

@JmillsExpensify, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

[JmillsExpensify]

Waiting for proposal reviews. cc @abdulrahuman5196

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify, @abdulrahuman5196 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[abdulrahuman5196]

Hi, Will check on this tomorrow

[abdulrahuman5196]

Checking now

[abdulrahuman5196]

@lanitochka17 / @JmillsExpensify I don't see this happening from the OP - Wrong video playback speed is highlighted.
But I can see this in chrome web - There is no highlight on Android and iOS app

Should we make a fix to show the pre-selected playback speed highlighted?

Screen.Recording.2024-06-05.at.12.50.33.AM.mov

[melvin-bot]

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

[JmillsExpensify]

All yours @allroundexperts!

[allroundexperts]

Thanks for the proposals everyone.

Given the updated requirements here, @Krishna2323's main solution works good. Let's go with them.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

@allroundexperts PR ready for review ^

[Krishna2323]

@JmillsExpensify, PR was deployed to production 4 days ago, can you please update the title? Thanks

[Krishna2323]

@JmillsExpensify, payments due date was 17th July, could you please process the payments? Thanks

[Krishna2323]

@allroundexperts, can you please bump @JmillsExpensify for payments in slack 🙏🏻

[Krishna2323]

@JmillsExpensify @allroundexperts friendly bump

[allroundexperts]

Hi @Krishna2323!

I have messaged @JmillsExpensify. Thanks for your patience. Someone from the team should be handling this soon.

[allroundexperts]

Checklist

1. Video player #30829
2. https://github.com/Expensify/App/pull/30829/files#r1713070836
3. N/A
4. A checklist would be helpful here.

Regression test

1. Open any chat or room
2. Upload a video attachment
3. Play the video
4. Click on the "..." button
5. Click on the "Playback speed" button
6. Verify that the first option is highlighted
7. Change the speed
8. Click on the "..." button
9. Click on the "Playback speed" button again
10. Verify the selected speed is highlighted

Do we 👍 or 👎 ?

[JmillsExpensify]

@Krishna2323 All paid out. Always feel free to reach out to me via Slack if you don't see a response here.

[JmillsExpensify]

Payment summary:

• Contributor: @Krishna2323 $250
• Reviewer: @allroundexperts $250

[melvin-bot]

This issue has not been updated in over 15 days. @JmillsExpensify, @cristipaval, @allroundexperts, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

^^ Payment summary here confirmed is good.

[JmillsExpensify]

$250 approved for @allroundexperts