[HOLD for Payment 2024-05-29] [$250] Web - Attachment - The action bar flickers when downloading a video.

[kbecciv]

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: v1.4.60-1
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in.
3. Go to any chat
4. Add a video attachment
5. Play and Expand video using arrows in the right top corner
6. Open tree dot menu and press download

Expected Result:

The action bar doesn't flicker when downloading a video.

Actual Result:

he action bar flickers when downloading a video

Workaround:

n/a

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

Bug6437920_1712233909357.Video.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01756abc8d4e07a5e1
• Upwork Job ID: 1775894887205498880
• Last Price Increase: 2024-04-25
• Automatic offers:
• rojiphil | Reviewer | 0
• ikevin127 | Contributor | 0

Issue Owner

Current Issue Owner: @laurenreidexpensify

[melvin-bot]

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

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

[kbecciv]

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

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

This seems like a minor bug that does not effect users experience

[melvin-bot]

Upwork job price has been updated to $250

[nkdengineer]

Proposal

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

• Web - Attachment - The action bar flickers when downloading a video.

What is the root cause of that problem?

• We display the VideoPlayerControls based on this condition. When we open popover menu, isHovered is false. When we close the popover menu, it needs time to update isHovered to true because it is a state. So it leads to the flicker error.

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

• We can update the above condition to:

         shouldShowVideoControls && !isLoading && (isPopoverVisible || prevIsPopoverVisible || isHovered || canUseTouchScreen)

• In the above, I introduce a prevIsPopoverVisible:

        const prevIsPopoverVisible = usePrevious(isPopoverVisible)

What alternative solutions did you explore? (Optional)

• NA

[ikevin127]

Proposal

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

[Attachment modal]: The video player controls bar flickers when opening / closing the video's 3-dot popover menu.

What is the root cause of that problem?

Tip

These steps are enough to reproduce the flicker:

1. Go to any chat.
2. Add a video attachment (you don't need to upload it).
3. Click on the 3-dot (More) button to open the Popover (you should see the Playback speed option).
4. Click outside of the Popover to close it -> notice the flicker.

This happens due to our VideoPopoverMenu component using a portal, which when open comes on top of the BaseVideoPlayer Hoverable surface of the video. These are the events that occur which lead to our issue:

1. The video player controls 3-dot menu (More) is clicked -> Popover opens -> isHovered state is set to false.
2. User clicks outside of the Popover -> Popover closes -> isHovered state is set to false again.
3. Right after the Popover is gone, the Hoverable surface re-captures the mouse event -> isHovered state is set to true.

Our issue occurs between (2.) and (3.) in very quick succession which causes the flicker of the video player controls bar because of this condition here, specifically the isHovered state variable coming from the Hoverable component which wraps the video container, toggling false/true in quick succession right after the isPopoverVisible becomes false.

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

For a more holistic, well-rounded solution I propose we introduce a new boolean prop to the Hoverable component, named shouldFreezeCapture which will be used within the ActiveHoverable component to prevent setting the isHovered state to false the second time (unnecessarily) when an open portal (popover / modal) is being closed.

How do we use this new prop ? We only need to apply it in two places:

1. At the top of this updateIsHovered function to perform an early return if shouldFreezeCapture is true.
2. Within this useEffect's unsetHoveredIfOutside function we pass || shouldFreezeCapture to the already existing early return.

Note

Both of these are needed to prevent the unnecessary setIsHovered(false) state update.

Then we simply pass shouldFreezeCapture={isPopoverVisible} to the Hoverable within BaseVideoPlayer.

This will not only fix our issue but also add a new prop which can be leveraged in the future whenever a portal (popover / modal) is open and we want to avoid the extra renders caused by the unnecessary isHovered state update as a result of the popover closing and re-capturing of the hover event.

Videos (before / after)

MacOS: Chrome / Safari

Before	After
before.mov	after.mov

[mountiny]

@rojiphil Can you please review the proposal? thanks!

[rojiphil]

I will review the proposals today and share the update.

[rojiphil]

On trying to reproduce the issue with the latest main, I could not find the flickering behavior. Instead, it simply hides the action bar (i.e. on clicking outside of the popover or on clicking on a popover menu item). This makes sense to me as the user click may occur outside or inside the video player as shown in the video below. Later, whenever there is a hover over the video player, the action bar shows up which I think is expected too. So, I am wondering if any fix is required here.
@mountiny What are your thoughts on this? Also, attaching a test video for reference.

39598-flickering-issue.mp4

[mountiny]

That does make sense @kbecciv can you still reproduce this on latest staging?

[nkdengineer]

@rojiphil I still can reproduce the issue when playing video in "Expand" mode

[ikevin127]

@rojiphil @mountiny This issue happens within the Attachment modal while open with a video.

Tip

These steps are enough to reproduce the flicker:

1. Go to any chat.
2. Add a video attachment (you don't need to upload it).
3. Click on the 3-dot (More) button to open the Popover (you should see the Playback speed option).
4. Click outside of the Popover to close it -> notice the flicker.

MacOS: Chrome / Safari

before.mov

[ikevin127]

PR #41125 ready for review! 🎉

Note

This behaviour and fix can observed only on large layout devices where hover event is available, therefore testing on narrow layout devices which use touch is not necessary as there won't be any changes in behaviour.

@rojiphil Despite the notes from above, in case you're going to attempt testing iOS: mWeb on a real physical device, just know that there's a bug unrelated to this issue / PR:

• [HOLD for Payment 2024-05-15][$500] mWeb - Chat - Infinite loading on video preview before sending video #39078

You can still test iOS: mWeb on Simulator where this bug is not reproducible.

[rojiphil]

Thanks @ikevin127 for the PR and the relevant updates.
I will review and share the update today.

[melvin-bot]

This issue has not been updated in over 15 days. @rojiphil, @mountiny, @ikevin127 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!

[rojiphil]

PR deployed to staging. Awaiting production deployment.

[ikevin127]

⚠️ Automation failed: This was deployed to production today here, therefore should be [HOLD for Payment 2024-05-29] and have Awaiting Payment label. Additionally, it's missing the Bug label -> doesn't have a BZ team member assigned.

cc @mountiny

[melvin-bot]

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

[mountiny]

$250 to @ikevin127 and to @rojiphil

[laurenreidexpensify]

Payment only due next week

[laurenreidexpensify]

Payment tomorrow

[laurenreidexpensify]

Payment Summary:

• C+ @rojiphil $250 issued in upwork
• Contributor @ikevin127 $250 issued in upwork

Looks like this was reported by Applause, no regression test required. Closing