[Awaiting Contributor Response for Payment - Payment 27th June] [$250] Incorrect video is paused after turning on and off full screen mode

[m-natarajan]

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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @blimpich / @KMichel1030
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716239323512349

Action Performed:

1. Upload two videos (Video1, Video2)
2. Play Video1
3. Play Video2
4. Click full screen mode button of Video1 while Video2 is playing
5. Exit full screen mode by pressing Esc key
6. Click play button of Video1

Expected Result:

Video1 should be played

Actual Result:

Video2 is played

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

bandicam.2024-05-17.08-42-04-936.mp4

Recording.97.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0153a7421c659e1e1b
• Upwork Job ID: 1793023328329351168
• Last Price Increase: 2024-05-28
• Automatic offers:
• hoangzinh | Reviewer | 102587027

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[trjExpensify]

@KMichel1030 are you interesting in proposing a solution for this?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0153a7421c659e1e1b

[melvin-bot]

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

[jacobnguyen0000]

Proposal

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

Incorrect video is paused after turning on and off full screen mode

What is the root cause of that problem?

When entering full screen mode, currentlyPlayingURL is updated, but currentVideoPlayerRef is not updated because we have isFullScreenRef check here.
Since we use currentVideoPlayerRef when play and pause video, incorrect video is paused.

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

We should remove isFullScreenRef check here.
We also need to modify last parameter of this line to prevent auto play when entering full screen mode.

What alternative solutions did you explore? (Optional)

[melvin-bot]

📣 @jacobnguyen0000! 📣
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>

[tienifr]

Proposal

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

What is the root cause of that problem?

• Let say we have two video A and B.

• When we play B then enter the fullscreen mode with A, we call updateCurrentlyPlayingURL in here and we do not call shareVideoPlayerElements in here. Now currentlyPlayingURL refers to A, but currentVideoPlayerRef refers to B.

• Then, after we turn off fullscreen mode, currentlyPlayingURL still refers to A, but currentVideoPlayerRef still refers to B.

• So when we click on the "Play" button in A, in here, because isCurrentlyURLSet is true, so the updateCurrentlyPlayingURL(url) is not called.

• So this useEffect is not called because one of its dependences, currentlyPlayingURL is not updated. That leads to currentVideoPlayerRef still refers to B. That why we click on "Play" button in A, B is played.

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

• In here, we should create a new param, named shouldPlayVideo, and update logic:
  

  App/src/components/VideoPlayerContexts/PlaybackContext.tsx

  Lines 59 to 61 in e8ae3c5

  	if (!isUploading) {
  	playVideo();
  	}

  
  to:

 if (!isUploading && shouldPlayVideo) { 
     playVideo(); 
 }

• Then, we can call this logic manually in here with param shouldPlayVideo: false

What alternative solutions did you explore? (Optional)

• We can call updateCurrentlyPlayingURL(null) in here to reset the currently playing url.

[mstindia-com]

Proposal

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

Incorrect video is paused after turning on and off full-screen mode on a video.

What is the root cause of that problem?

The initial value for currectVideoPlayerRef was set by VideoWithOnFullScreenUpdate.
That's why only play action sets the currectVideoPlayerRef. and not the fullscreen button action.
Because of the reference to the last played video, the incorrect video is paused.

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

currectVideoPlayerRef should be updated or assigned as per action taken on videos.
Initially set currentVideoPlayerRef as null to fix errors on load.
currentVideoPlayerRef will be assigned when the video player play button is triggered.

1. Update the following line
   

   App/src/components/VideoPlayer/VideoPlayerControls/index.tsx

   Line 65 in e8ae3c5

   const {updateCurrentlyPlayingURL} = usePlaybackContext();

   
   with following

const {updateCurrentlyPlayingURL, currentVideoPlayerRef} = usePlaybackContext();

2. In the function enterFullScreenMode after the following line
   

   App/src/components/VideoPlayer/VideoPlayerControls/index.tsx

   Line 77 in e8ae3c5

   videoPlayerRef.current?.presentFullscreenPlayer();

   
   add the following line of code

currentVideoPlayerRef?.current = videoPlayerRef.current;

3. Update the following line
   

   App/src/components/VideoPlayerContexts/PlaybackContext.tsx

   Line 16 in e8ae3c5

   const currentVideoPlayerRef = useRef<VideoWithOnFullScreenUpdate | null>(null);

   
   with the following

const currentVideoPlayerRef = useRef(null);

What alternative solutions did you explore? (Optional)

[tienifr]

I updated alternative solution

[hoangzinh]

Thanks for proposals everyone, I will manage to review today

[hoangzinh]

@jacobnguyen0000 Thanks for proposal #42427 (comment). I don't think we can remove isFullScreenRef.current from here. Thanks to @tienifr, he already confirmed reason with author of that line here

[hoangzinh]

@mstindia-com Thanks for proposal #42427 (comment). I don't think your solution is correct. It makes sense to set isFullScreenRef.current = true from here so player knows it's on full screen mode.

[hoangzinh]

@tienifr Thanks for proposal #42427 (comment). Your RCA is great, it's very clear to help me understand RC. But I'm confused your main solution, why having shouldPlayVideo would solve this issue, or your alternative solution, could you help me understand why set updateCurrentlyPlayingURL(null) solve this issue? Base on your RCA, I think that is it better if we also set currentVideoPlayerRef refers to A here?

[tienifr]

@hoangzinh In my main solution, the solution to fix the bug is just calling this logic manually in here. shouldPlayVideo is used to fix the bug that: the video is always played after turning off fullscreen mode.

[tienifr]

could you help me understand why set updateCurrentlyPlayingURL(null) solve this issue

As I mentioned in the RCA:

So this useEffect is not called because one of its dependences, currentlyPlayingURL is not updated. That leads to currentVideoPlayerRef still refers to B. That why we click on "Play" button in A, B is played.

If we call updateCurrentlyPlayingURL(null) after turning off fullscreen mode, it will set the currentlyPlayingURL to null. When we click on play button, currentlyPlayingURL will be set to A. Because null !== A, so the useEffect is called, so the shareVideoPlayerElements is called.

[jacobnguyen0000]

@hoangzinh
Thank you for your review.

@jacobnguyen0000 Thanks for proposal #42427 (comment). I don't think we can remove isFullScreenRef.current from here. Thanks to @tienifr, he already confirmed reason with author of that line #38407 (comment)

I've tested on my end but it seems removing isFullScreenRef.current doesn't any problem.

[mstindia-com]

Proposal

Updated

[mstindia-com]

@hoangzinh Thanks for your reply. I managed to find a solution. updated my proposal.

[hoangzinh]

@tienifr @jacobnguyen0000 @mstindia-com thanks for updates. After considering proposals, I think:

• @jacobnguyen0000 solution could lead to the unknown bug here [Awaiting Contributor Response for Payment - Payment 27th June] [$250] Incorrect video is paused after turning on and off full screen mode #42427 (comment)
• @tienifr you have great RCA, but your solution doesn't look good to me. We have to use an additional param shouldPlayVideo to avoid a bug: the video is always played after turning off fullscreen mode. As you mentioned here [Awaiting Contributor Response for Payment - Payment 27th June] [$250] Incorrect video is paused after turning on and off full screen mode #42427 (comment)
• @mstindia-com your solution looks good to me. Could you update RCA with line of code reference so the 2nd reviewer (internal engineer) quickly understands what you mean? Thanks

[tienifr]

@hoangzinh Thanks for your feedback.

• As mentioned in here, the video player should not be set when turning on fullscreen mode. But this solution does not follow this rule. So it can lead to regression.

• With the above point, the RCA "When entering full-screen mode on a video currentlyPlayingURL is updated, but currentVideoPlayerRef is not updated." is not correct as well. Because "currentVideoPlayerRef is not updated" aims to make sure this rule, but it is considered as the RCA leads to the bug.

• Also, I don't think that using an additional param shouldPlayVideo is the problem. It is very straightforward.

[tienifr]

@hoangzinh Also, what do you think about my alternative solution?

[jacobnguyen0000]

@hoangzinh

@jacobnguyen0000 solution could lead to the unknown bug here #42427 (comment)

Thank you for your check.

isFullScreenRef is responsible for locking the video player when screen orientation changes in full-screen mode. It prevents video players from changing their state because of device rotation or window size changes. Changing state causes the full screen to dismiss. We need it here to block updating currently shared video element

I've tested above cases and confirmed it's working properly with my proposal.

[jacobnguyen0000]

@hoangzinh
Checking with author here

[trjExpensify]

Bump @jacobnguyen0000 on providing your Upwork profile for payment.

[grgia]

awaiting payment details, not overdue

[trjExpensify]

Melvin, stop it.

[trjExpensify]

@jacobnguyen0000 bump here!

[hoangzinh]

He hasn't had any activities this July yet https://github.com/jacobnguyen0000. We can change to weekly label while waiting for @jacobnguyen0000 response 🤔

[trjExpensify]

Cool, dropped to weekly. I'll likely just close this after a couple of weeks, and @jacobnguyen0000 can let me know when he's back if he wants to get paid.

[trjExpensify]

Closing.

[jacobnguyen0000]

Hi, @trjExpensify
Sorry for late response. Can I get payment now?

Contributor details
Your Expensify account email: jacobnguyen0000@outlook.com
Upwork Profile Link: https://www.upwork.com/freelancers/~014cac515a92876fc5

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[jacobnguyen0000]

@mallenexpensify
It seems @trjExpensify is not here.
Could you please complete payment here?

[trjExpensify]

Sent an offer.

[jacobnguyen0000]

Thank you, @trjExpensify.
I've accepted offer.

[trjExpensify]

Paid!