search

benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Emptied event and pause event browser differences #354

Closed danielr18 closed 5 years ago

danielr18 commented 5 years ago

I've been looking for playback issues on mobile/desktop browsers these last days, that's why I still haven't done the #352 PR.

First of all, I think we should listen to the emptied event and change paused to true in the state. If you look at the spec, you'll see that after load() is invoked and the emptied event is fired, media element paused will be set to true. While Firefox triggers a pause event on that change, Chrome doesn't, thus paused in the cassette state isn't updated. You can recreate that by invoking load method in the media ref on Chrome, you should see that cassette paused is set to false.

That's why Firefox play/pause icons quickly change back and forth when you change tracks for example in https://www.owltail.com/. When you play another track, it will set the new sources and call load, then it would call play, but at the same time the emptied and pause events would be triggered, so paused is set to true in the state and then when play event is triggered, it'll we set back to false. This looks nicer in Chrome because pause event isn't triggered on load(), but once we add the event listener for emptied to set paused in the state, both browsers should behave the same.

If we wanted to avoid those quick changes in paused while still being able to set paused to true if you invoke load() in user land, then we should use awaitingPlay to prevent updates to paused. I've already got something working locally, but had to change when we set awaitingPlay to false.

To conclude: 1 - We should have an emptied event listener to set paused to true. 2 - We need to decide if we want to keep the paused state consistent to media.paused when changing to another track, or not for a better UX. If you choose consistency, then passing something like awaitingPlay in context would be useful so that lib users can have a better UX in their app by preventing those quick play/pause changes.

benwiley4000 commented 5 years ago

Good call! I'd take a PR for this if you have something working - thanks!

For the UX question - we have another case where we display the play/pause button as playing even when it's technically paused in the context state - awaitingResumeOnSeekComplete. You can see that here: https://github.com/benwiley4000/cassette/blob/next/packages/player/src/controls/PlayPauseButton.js#L30

About using awaitingPlay - I don't want to confuse the meaning of this piece of state which is currently just "the play method will be called on the next componentDidUpdate". It should never be set for more than a couple of milliseconds in a normal render. Maybe we should rename it to be more specific (awaitingPlayAfterNextUpdate?).

I think there should be a new distinct piece of state which is what you're referring to... not sure what the name should be.

Then maybe it can be passed through context props like you're saying, and maybe it should be merged together in the context props with awaitingResumeOnSeekComplete since the use case is basically the same. We could call the context prop awaitingPlayResume or something? (We can use awaitingPlay if we rename the other thing?) What do you think?

benwiley4000 commented 5 years ago

Made a few edits to my last comment.

danielr18 commented 5 years ago

Should I remove paused: !shouldPlay in getGoToTrackState to keep paused consistent with media.paused?

benwiley4000 commented 5 years ago

Good question.. I guess the answer is yes. Although I'm debating with myself how useful it is to know the actual paused state of the media element. I guess some UIs will not want to say they are "playing" unless actual playback is happening?

Note to myself

this will be a breaking change for anyone relying on the paused state.

benwiley4000 commented 5 years ago

@danielr18 I thought about this and decided it's fine to go ahead with this plan (making paused always consistent with the media element state).

benwiley4000 commented 5 years ago

Closed in #360.