Skip to content
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.

Sign up for GitHub

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

fix error when reloading video attachment #57374

Merged
merged 4 commits into from
Mar 7, 2025
Merged

fix error when reloading video attachment #57374

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4affb60
fix error when reloading video attachment
mkzie2 Feb 25, 2025
052a466
Merge branch 'main' into mkzie2-issue/57055
mkzie2 Mar 4, 2025
0a335ba
Merge branch 'main' into mkzie2-issue/57055
mkzie2 Mar 5, 2025
9cb6f40
Merge branch 'main' into mkzie2-issue/57055
mkzie2 Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/components/VideoPlayer/BaseVideoPlayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import VideoPopoverMenu from '@components/VideoPopoverMenu';
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import addEncryptedAuthTokenToURL from '@libs/addEncryptedAuthTokenToURL';
import {isMobileSafari} from '@libs/Browser';
import {canUseTouchScreen as canUseTouchScreenLib} from '@libs/DeviceCapabilities';
import CONST from '@src/CONST';
import shouldReplayVideo from './shouldReplayVideo';
Expand Down Expand Up @@ -89,6 +90,7 @@ function BaseVideoPlayer({
const videoPlayerElementParentRef = useRef<View | HTMLDivElement | null>(null);
const videoPlayerElementRef = useRef<View | HTMLDivElement | null>(null);
const sharedVideoPlayerParentRef = useRef<View | HTMLDivElement | null>(null);
const isReadyForDisplayRef = useRef(false);
const canUseTouchScreen = canUseTouchScreenLib();
const isCurrentlyURLSet = currentlyPlayingURL === url;
const isUploading = CONST.ATTACHMENT_LOCAL_URL_PREFIX.some((prefix) => url.startsWith(prefix));
Expand Down Expand Up @@ -351,7 +353,13 @@ function BaseVideoPlayer({
if (shouldUseSharedVideoElement || url !== currentlyPlayingURL || reportID !== currentlyPlayingURLReportID) {
return;
}
shareVideoPlayerElements(videoPlayerRef.current, videoPlayerElementParentRef.current, videoPlayerElementRef.current, isUploading || isFullScreenRef.current);
// On mobile safari, we need to auto-play when sharing video element here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In mobile safari, the video isn't auto-play after our change, my safari development doesn't work when I try to see the log to have a deep RCA. Can you please help to log in when running the Safari mobile web on the logic here? Thanks.

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 481 to 483 in 7d0bcb4

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you want me to log? I didn't understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat I want to add a log like this. My safari develop console doesn't work when I run the mobile safari web.

if (isCurrentlyURLSet && !isUploading) { 
     console.log("runned play video");
     playVideo(); 
 }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this log. Do you have any specific steps that I should perform? Anyways, Looks like when we reload the page, safari debugger disconnects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat The step is just simply clicking on a video thumbnail to play. If we only use isReadyForDisplayRef.current, the video isn't auto play.

shareVideoPlayerElements(
videoPlayerRef.current,
videoPlayerElementParentRef.current,
videoPlayerElementRef.current,
isUploading || isFullScreenRef.current || (!isReadyForDisplayRef.current && !isMobileSafari()),
);
}, [currentlyPlayingURL, shouldUseSharedVideoElement, shareVideoPlayerElements, url, isUploading, isFullScreenRef, reportID, currentlyPlayingURLReportID]);

// Call bindFunctions() through the refs to avoid adding it to the dependency array of the DOM mutation effect, as doing so would change the DOM when the functions update.
Expand Down Expand Up @@ -409,7 +417,7 @@ function BaseVideoPlayer({
}, [shouldPlay, updateCurrentlyPlayingURL, url]);

useEffect(() => {
videoPlayerRef.current?.setStatusAsync({volume: 0});
videoPlayerRef.current?.setStatusAsync({isMuted: true});
}, []);

return (
Expand Down Expand Up @@ -478,6 +486,7 @@ function BaseVideoPlayer({
resizeMode={resizeMode as ResizeMode}
isLooping={isLooping}
onReadyForDisplay={(e) => {
isReadyForDisplayRef.current = true;
if (isCurrentlyURLSet && !isUploading) {
playVideo();
}
Expand Down