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

lanitochka17 commented 3 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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:

Log in with any account
Open any chat or room
Upload a video attachment
Play the video
Click on the "..." button
Click on the "Playback speed" button
Change the speed
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
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/a9df940b-70eb-4874-b4d8-2c1fd7ed2442

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[bot] commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

Krishna2323 commented 3 months ago

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. https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/PopoverMenu.tsx#L102-L111 https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx#L60-L67

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

Pass isSelected property in subMenuItems option.

isSelected: currentPlaybackSpeed === speed,

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. https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/PopoverMenu.tsx#L102-L111

We already clear the index when selecting the option by pressing enter. https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/PopoverMenu.tsx#L148-L152

tienifr commented 3 months ago

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 commented 3 months ago

Opening up to the community.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

Krishna2323 commented 3 months ago

Proposal Updated

Added alternative 2

hurram-dev commented 3 months ago

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 https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/PopoverMenu.tsx#L160

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[bot] commented 3 months ago

📣 @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:

Make sure you've read and understood the contributing guidelines.
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.
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.
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 commented 3 months ago

Contributor details Expensify account email: xurrambeks@gmail.com Upwork Profile: https://www.upwork.com/freelancers/~0130a0ff1a64915163

melvin-bot[bot] commented 3 months ago

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

JmillsExpensify commented 3 months ago

Waiting for proposal reviews. cc @abdulrahuman5196

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

abdulrahuman5196 commented 3 months ago

Hi, Will check on this tomorrow

abdulrahuman5196 commented 3 months ago

Checking now

abdulrahuman5196 commented 3 months ago

@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?

https://github.com/Expensify/App/assets/46707890/3bfd57f9-2c2b-45a4-a2e9-dd25651b540a

melvin-bot[bot] commented 3 months ago

@JmillsExpensify @abdulrahuman5196 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!

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 2 months ago

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

JmillsExpensify commented 2 months ago

I'm good with that appraoch @abdulrahuman5196

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@JmillsExpensify, @abdulrahuman5196 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 2 months ago

@JmillsExpensify @abdulrahuman5196 this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 2 months ago

@JmillsExpensify, @abdulrahuman5196 12 days overdue. Walking. Toward. The. Light...

JmillsExpensify commented 2 months ago

Same as above.

melvin-bot[bot] commented 2 months ago

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

abdulrahuman5196 commented 2 months ago

Hi, Sorry I was OOO couple of days, Will check the proposals as per the mentioned approach in my morning

melvin-bot[bot] commented 2 months ago

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

JmillsExpensify commented 2 months ago

Still waiting on proposal review/next steps.

abdulrahuman5196 commented 2 months ago

Hi, @JmillsExpensify Sorry for the delay, I have limited availability at the moment. So unassigning myself. Kindly reassign another C+ for review.

allroundexperts commented 2 months ago

I can take this on!

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

JmillsExpensify commented 2 months ago

All yours @allroundexperts!

allroundexperts commented 2 months ago

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[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

📣 @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 commented 2 months ago

@allroundexperts PR ready for review ^

Krishna2323 commented 1 month ago

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

Krishna2323 commented 1 month ago

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

Krishna2323 commented 1 month ago

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

Krishna2323 commented 3 weeks ago

@JmillsExpensify @allroundexperts friendly bump

allroundexperts commented 3 weeks ago

Hi @Krishna2323!

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

allroundexperts commented 3 weeks ago

Checklist

https://github.com/Expensify/App/pull/30829
https://github.com/Expensify/App/pull/30829/files#r1713070836
N/A
A checklist would be helpful here.

Regression test

Open any chat or room
Upload a video attachment
Play the video
Click on the "..." button
Click on the "Playback speed" button
Verify that the first option is highlighted
Change the speed
Click on the "..." button
Click on the "Playback speed" button again
Verify the selected speed is highlighted

Do we 👍 or 👎 ?

JmillsExpensify commented 3 weeks ago

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

JmillsExpensify commented 3 weeks ago

Payment summary:

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

Expensify / App