[HOLD for payment 2024-04-15] [$500] Attachments - Next video starts playing when change Playback speed

[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.56-0
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
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Issue found when executing PR #37425

Action Performed:

1. Upload few videos to one chat
2. Expand the first video
3. Go to the 3-dots menu> Playback speed> Change the speed

Expected Result:

Video playing speed should be changed

Actual Result:

When change one video speed next video starts playing

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

Bug6423497_1711131431269.Recording__2677.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d33a91241a495ee7
• Upwork Job ID: 1772739460916158464
• Last Price Increase: 2024-03-26
• Automatic offers:
• ShridharGoel | Contributor | 0

[melvin-bot]

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

[lanitochka17]

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

[ShridharGoel]

Proposal

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

Next video starts playing when change playback speed or next attachment opens.

What is the root cause of that problem?

withoutOverlay is being passed as true in the PopoverMenu.

App/src/components/VideoPopoverMenu/index.tsx

Lines 31 to 40 in 191d932

	<PopoverMenu
	onClose={hidePopover}
	onItemSelected={hidePopover}
	isVisible={isPopoverVisible}
	anchorPosition={anchorPosition}
	menuItems={menuItems}
	anchorRef={videoPlayerMenuRef}
	withoutOverlay
	/>
	);

Because of this, the playback speed options popover doesn't have an overlay behind it. So, when an option is selected, the click goes to the 'next' option behind the playback speed menu, and it moves to the next attachment.

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

withoutOverlay should not be passed to PopoverMenu from VideoPopoverMenu.

<PopoverMenu
    onClose={hidePopover}
    onItemSelected={hidePopover}
    isVisible={isPopoverVisible}
    anchorPosition={anchorPosition}
    menuItems={menuItems}
    anchorRef={videoPlayerMenuRef}
/>

Playback.mov

[ShridharGoel]

@johncschuster Can you check this when possible?

[melvin-bot]

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

[melvin-bot]

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

[parasharrajat]

It seems that @ShridharGoel's proposal is working fine but I wouldn't consider that as the best solution to the issue as it does not solve the root cause. Also, I am not sure why this option was used. I didn't see any visible changes with or without this withoutOverlay prop. This is used to create the node outside the normal flow of DOM on web.

We should try to explain why clicking on any item on the playback modal also hits the next button and solve this problem.

We might face this issue again somewhere else in the App. @ShridharGoel Could you please look for more details on this issue?

[ShridharGoel]

@parasharrajat This is happening because the click event in the popover menu gets propagated to the pressable used for the next attachment button.

e.stopPropagation() also doesn't work (Explanation). So I think we should not use PopoverWithoutOverlay in such scenarios and use the Modal component since UI wise both are looking same.

[parasharrajat]

Ok, thanks. I am kind of fine with this change given that there are no UI changes. Please make sure that there are no issues from this change before moving to PR. I will keep this issue open for a few hours.

[ShridharGoel]

@parasharrajat Tested again, can't see any issues with this solution.

[brandonhenry]

Proposal

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

The problem we are trying to solve is the unexpected behavior where changing the playback speed of a video attachment triggers the next video to start playing or the next attachment to open. This deviates from the expected behavior, which is that only the playback speed of the current video should be affected without any unintended side effects.

What is the root cause of that problem?

The root cause of the issue lies in how touch events are handled within the PopoverMenu component. When the withoutOverlay prop is set to true, the popover menu is rendered without an overlay behind it. This allows touch events to pass through the popover menu and reach the underlying components, such as the "next" button or another attachment. As a result, when a user interacts with the playback speed options, the touch events propagate to the underlying components, triggering unintended actions.

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

To address this issue, we can modify the PopoverMenu component itself to handle touch events properly and prevent their propagation. Here's the proposed solution:

1. Inside the PopoverMenu component, wrap the entire content with a TouchableWithoutFeedback component.
2. When a touch event occurs within the PopoverMenu, call event.stopPropagation() to prevent the event from bubbling up to the parent components.

Here's an example of how the modified PopoverMenu component could look:

import React from 'react';
import {TouchableWithoutFeedback, View} from 'react-native';

// ...

function PopoverMenu({
    // ...
    withoutOverlay = false,
    // ...
}: PopoverMenuProps) {
    // ...

    const handleContentPress = (event: any) => {
        event.stopPropagation();
    };

    return (
        <PopoverWithMeasuredContent
            // ...
            withoutOverlay={withoutOverlay}
            // ...
        >
            <TouchableWithoutFeedback onPress={handleContentPress}>
                <View style={isSmallScreenWidth ? {} : styles.createMenuContainer}>
                    {/* ... */}
                </View>
            </TouchableWithoutFeedback>
        </PopoverWithMeasuredContent>
    );
}

By wrapping the content of the PopoverMenu with a TouchableWithoutFeedback component and calling event.stopPropagation() when a touch event occurs, we can effectively "gobble" the event and prevent it from propagating to the parent components. This ensures that the touch events are contained within the popover menu and do not trigger any unintended actions on the underlying components, even when the withoutOverlay prop is set to true.

This approach allows us to keep the withoutOverlay prop and avoid creating a separate overlay, while still addressing the issue of touch events propagating to the underlying components.

What alternative solutions did you explore?

An alternative solution that was considered involved removing the withoutOverlay prop and allowing the default overlay behavior. The overlay acts as a barrier, preventing touch events from passing through to the components behind the popover menu. However, this approach would require modifying the usage of the PopoverMenu component and potentially impact the desired functionality or user experience in certain scenarios.

Another alternative was to create a makeshift overlay using a TouchableWithoutFeedback component wrapped around the PopoverMenu component. This overlay would capture touch events and prevent them from reaching the underlying components. However, this approach introduces an additional component and may not be as clean or maintainable as modifying the PopoverMenu component itself.

Cc. @parasharrajat

[parasharrajat]

Ok, let's roll. @ShridharGoel's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @ShridharGoel 🎉 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]

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.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Video playback speed change should not move to next video #39292

If no regressions arise, payment will be issued on 2024-04-15. 🎊

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

• @parasharrajat requires payment through NewDot Manual Requests
• @ShridharGoel requires payment automatic offer (Contributor)

[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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] 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:
• [@parasharrajat] 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:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] 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.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[johncschuster]

Payment Summary:

C+ reviewer: @parasharrajat - $500 paid via NewDot Manual Requests
Contributor: @ShridharGoel - $500 paid through Upwork

@ShridharGoel, can you please accept the Upwork offer so I can issue payment? Thank you!

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Video player #30829
• [@parasharrajat] 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: Video player #30829 (comment)
• [@parasharrajat] 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: NA
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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.

Regression Test Steps

1. Open a chat.
2. Upload multiple attachments to the chat.
3. Expand any video which has some attachments after it.
4. Change the playback speed.
5. Attachment should not change.

Do you agree 👍 or 👎 ?

[johncschuster]

Note to self: Waiting on @ShridharGoel to accept the Upwork invite so I can pay them. Once payment has been issued, this can be closed.

[ShridharGoel]

@johncschuster I've accepted the offer, thanks.

[johncschuster]

Payment has been issued! Thanks, everyone!

[parasharrajat]

Payment requested as per #38831 (comment)

[JmillsExpensify]

$500 approved for @parasharrajat