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.

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: replay button broken for native playback #8142

Merged
merged 7 commits into from
Mar 6, 2023
Merged

Conversation

adrums86
Copy link
Contributor

Description

During native playback in Safari (desktop or iOS) if the user reaches the end of content in fullscreen, the replay button will no longer function with a single click. Meaning, the button will work and change state to playing however the video does not restart. It seems to be an issue with the content not ending correctly as the ended event is firing, however video.ended is returning false. Since it's native, we don't have the option of calling endOfStream on the mediaSource.

Specific Changes proposed

If we detect native playback from the tech and the play toggle is still in the replay state, we can manually do a seek to 0 and call play.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@adrums86 adrums86 self-assigned this Feb 20, 2023
@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #8142 (205822a) into main (b306ce6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8142      +/-   ##
==========================================
+ Coverage   82.00%   82.03%   +0.03%     
==========================================
  Files         110      110              
  Lines        7339     7343       +4     
  Branches     1770     1773       +3     
==========================================
+ Hits         6018     6024       +6     
+ Misses       1321     1319       -2     
Impacted Files Coverage Δ
src/js/player.js 89.80% <100.00%> (+0.02%) ⬆️
src/js/tracks/text-track.js 94.87% <0.00%> (+0.64%) ⬆️
src/js/utils/browser.js 56.73% <0.00%> (+0.96%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Essk Essk left a comment

Choose a reason for hiding this comment

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

Looks good!

@gkatsev
Copy link
Member

gkatsev commented Feb 21, 2023

This seems like it probably should happen in player.play() since I'd expect the programmatic method would also fail in the same way.

@gkatsev
Copy link
Member

gkatsev commented Feb 21, 2023

Also, I'm really sad that it's needed. I think Video.js used to have it a long time ago but was removed because we thought it wasn't needed anymore.

@adrums86
Copy link
Contributor Author

@gkatsev It actually works like this, Safari doesn't seem to have a problem with the play call, tested on mac and iOS. That said, I agree it would fit better in the actual play function. This will need some refactoring anyway due to what @mister-ben mentioned.
I agree it is sad to see, luckily it seems isolated to native fullscreen only (ending in fullscreen, returning from fullscreen, hitting replay).

@adrums86
Copy link
Contributor Author

I moved everything over to player.js and added a unit test.

@adrums86 adrums86 merged commit b7116be into main Mar 6, 2023
@adrums86 adrums86 deleted the replay-native-fix branch March 6, 2023 18:32
adrums86 added a commit that referenced this pull request Mar 8, 2023
* fix: replay button broken for native playback

* remove debug logging

* move fix to player

* comment

* add unit test

* add native browser stubs

* reset stubs and test currentTime
adrums86 added a commit that referenced this pull request Mar 13, 2023
* fix: replay button broken for native playback

* remove debug logging

* move fix to player

* comment

* add unit test

* add native browser stubs

* reset stubs and test currentTime
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
* fix: replay button broken for native playback

* remove debug logging

* move fix to player

* comment

* add unit test

* add native browser stubs

* reset stubs and test currentTime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants