-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$250] Video doesn't auto play after refreshing the page #57055
Comments
Triggered auto assignment to @CortneyOfstad ( |
🚨 Edited by proposal-police: This proposal was edited at 2025-02-22 09:28:21 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Video doesn't auto play after refreshing the page What is the root cause of that problem?When the page is refreshed, the video player attempts to autoplay the video. However, this fails due to specific error in the
What changes do you think we should make in order to solve the problem?To fix the playVideoPromiseRef.current = currentVideoPlayerRef.current?.setStatusAsync(newStatus).then()
.catch((error) => {
if (error?.code === 0) {// In this case, I determine the error based on the error code
currentVideoPlayerRef.current?.setStatusAsync({
isMuted: true
}).finally(() => {
currentVideoPlayerRef.current?.setStatusAsync(newStatus);
});
return
}
// Throw or log other error for tracking
}); Since App/src/components/VideoPlayerContexts/PlaybackContext.tsx Lines 87 to 94 in 0392e3a
(playVideoPromiseRef.current ?? Promise.resolve()).finally(() => {
stopVideo()
videoResumeTryNumberRef.current = 0;
setCurrentlyPlayingURL(null);
setSharedElement(null);
setOriginalParent(null);
currentVideoPlayerRef.current = null;
unloadVideo();
}); After applying the code above, I noticed the video is successfully muted. However, the mute button does not display the correct state, so we need to apply the code below here to handle the muted video. if (status.isMuted && !prevIsMuted.get() && prevVolume.get() !== 0) {
updateVolume(0)
} cc @Expensify/design for visibility, as this will change the autoplay behavior. ResultScreen.Recording.2025-02-22.at.02.14.54.mp4What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?N/A UI bug What alternative solutions did you explore? (Optional)N/A |
I got a constant spinner when attempting to recreate this, so going to get eyes on it! 2025-02-21_12-26-22.mp4 |
Job added to Upwork: https://www.upwork.com/jobs/~021893004673912508623 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
@linhvovan29546 Can you show us the end result of your changes? |
Proposal updated Added result |
I've updated result |
@linhvovan29546 Can you please add explanations for each step to your proposal as well? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Video does not start playing, instead the following error is thrown in console: What is the root cause of that problem?In the past, we already have the logic to update the volume to 0 here to prevent errors. But it's outdated now, we should update App/src/components/VideoPlayer/BaseVideoPlayer.tsx Lines 411 to 413 in 0b6d1a0
So the error appears when we're trying to auto-play here or here What changes do you think we should make in order to solve the problem?For now, we should update the
App/src/components/VideoPlayer/BaseVideoPlayer.tsx Lines 411 to 413 in 0b6d1a0
When we call
App/src/components/VideoPlayer/BaseVideoPlayer.tsx Lines 479 to 483 in 0b6d1a0
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?NA What alternative solutions did you explore? (Optional)ResultScreen.Recording.2025-02-22.at.15.46.28.movReminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
@parasharrajat What do you think about my proposal? |
Proposal updated |
Thanks, both of you. But you don't explain why are you making any changes on each step. We generally discourage code snippets in proposals without explanation. Proposals should have sufficient information to explain each step verbally.
Why? Did you notice something about auto-playing? @mkzie2 What does not ready mean where?
What is being broken here? @linhvovan29546 |
@parasharrajat My solution modifies playVideoPromiseRef.current, and since resetVideoPlayerData uses this ref, I need to update the code inside resetVideoPlayerData to maintain its original behavior |
I was checking the old code and it looks like App/src/components/VideoPlayer/BaseVideoPlayer.tsx Lines 411 to 413 in 0b6d1a0
And if we know already that some browsers prevent the auto-play if videos with audio, it is better to mute the video instead. This will also keep the same intentional behavior on all platforms. Maybe we can pass |
I tried, but the issue is not resolved. I think we only need to handle this on Chrome when an error occurs during playback and maybe remove that useEffect TestScreen.Recording.2025-02-22.at.23.08.31.mov |
@parasharrajat What I mean here is that the first time the video is loaded,
I think we only need to pass |
Ok, thanks. Let's go with @mkzie2 proposal. If we already know that many browsers will prevent the auto-play then waiting for the error to show up does not sound best. Instead, if are fine with showing muted vide on auto-play, then we should just play the video with mute from the start. 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @deetergp, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@parasharrajat @mkzie2 Just a reminder, since you updated the |
Nice Catch @linhvovan29546. I don't expect that to break but @mkzie2 Can you do a try run and let us know that your proposal does not break #37674 |
@parasharrajat Just tested and it doesn't break #37674 |
I only add a logic to update the ref in |
Thanks. |
📣 @mkzie2 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@parasharrajat The PR is ready. |
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: 9.1.0-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @linhvovan29546
Slack conversation (hyperlinked to channel name): #expensify bugs
Action Performed:
Expected Result:
Video should start playing
Actual Result:
Video does not start playing, instead the following error is thrown in console:
NotAllowedError: play() failed because the user didn't interact with the document first. https://goo.gl/xX8pDD
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @parasharrajatThe text was updated successfully, but these errors were encountered: