-
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
Fix video sharing #38407
Merged
lakchote
merged 25 commits into
Expensify:main
from
software-mansion-labs:@Skalakid/fix-video-element-sharing
Mar 28, 2024
Merged
Fix video sharing #38407
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
b307865
Add window dimensions blocking POC
Skalakid 344c68d
Enhance bug fix code structure
Skalakid deec3ab
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid 4188a33
Fix TS errors
Skalakid 29a802a
Fix video sharing bug when changing window dimensions while video is …
Skalakid 4bbae6c
Restore code that is used in other places
Skalakid cab0496
Fix video starting playing when exiting fullscreen with paused video
Skalakid f54eb12
Change fix structure
Skalakid 05ed715
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid 1f04efc
Add locked screen dimensions updating when orientation changes
Skalakid 82ca579
Enhance device orientation checking
Skalakid 59ff4a2
Fix video pausing when dismissing fullscreen mode
Skalakid 4e06538
Change orginal size detction
Skalakid 1ca5515
Add comment
Skalakid 72b0fb1
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid df3db7d
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid ac35413
Add review changes
Skalakid 3f75fe3
Fix fullscreen mode bug
Skalakid 5b35c6e
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid 5622588
Fix fullscreen context types
Skalakid 328fbd8
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid e75f1c4
Add review changes
Skalakid 75896d2
Fix video pausing in fullscreen mode
Skalakid 9facbb7
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid 47f92d3
Merge branch 'main' into @Skalakid/fix-video-element-sharing
Skalakid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import React, {useCallback, useContext, useMemo, useRef} from 'react'; | ||
import type WindowDimensions from '@hooks/useWindowDimensions/types'; | ||
import type ChildrenProps from '@src/types/utils/ChildrenProps'; | ||
import type {FullScreenContext} from './types'; | ||
|
||
const Context = React.createContext<FullScreenContext | null>(null); | ||
|
||
function FullScreenContextProvider({children}: ChildrenProps) { | ||
const isFullScreenRef = useRef(false); | ||
const lockedWindowDimensionsRef = useRef<WindowDimensions | null>(null); | ||
|
||
const lockWindowDimensions = useCallback((newWindowDimensions: WindowDimensions) => { | ||
lockedWindowDimensionsRef.current = newWindowDimensions; | ||
}, []); | ||
|
||
const unlockWindowDimensions = useCallback(() => { | ||
lockedWindowDimensionsRef.current = null; | ||
}, []); | ||
|
||
const contextValue = useMemo(() => ({isFullScreenRef, lockedWindowDimensionsRef, lockWindowDimensions, unlockWindowDimensions}), [lockWindowDimensions, unlockWindowDimensions]); | ||
return <Context.Provider value={contextValue}>{children}</Context.Provider>; | ||
} | ||
|
||
function useFullScreenContext() { | ||
const fullscreenContext = useContext(Context); | ||
if (!fullscreenContext) { | ||
throw new Error('useFullScreenContext must be used within a FullScreenContextProvider'); | ||
} | ||
return fullscreenContext; | ||
} | ||
|
||
FullScreenContextProvider.displayName = 'FullScreenContextProvider'; | ||
|
||
export {Context as FullScreenContext, FullScreenContextProvider, useFullScreenContext}; | ||
Skalakid marked this conversation as resolved.
Show resolved
Hide resolved
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Skalakid I wonder why do we need to add
isFullScreenRef.current
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 elementThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me reproduce the bug if we remove
isFullScreenRef.current
? ThanksThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @Skalakid
I'm also curious about test steps that cause unexpected errors when we remove
isFullScreenRef.current
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to remove
isFullScreenRef.current
only in this useEffect if statement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Skalakid
Thank you for your confirm. I can't reproduce on my end.
Could you please let me know if this issue is reproducible on native devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed again, it's not reproducible on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tienifr oh thanks for information, in the video above I deleted
isFullScreenRef
in whole BaseVideo Player file 😅 So if it works after deletingisFullScreenRef.current
in useEffect, just do the test steps from this issue description and if everything works you can remove itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Skalakid
thank you for your check.