Improve ref handling in VideoPlayer component

[joshfarrant]

Currently, refs provided to the VideoPlayer component are never forwarded to the underlying <video> element, e.g.

// Scenario 1 — Doesn't work ❌
<VideoPlayer ref={ref} />

// Scenario 2 — Doesn't work ❌
<VideoPlayer.Provider>
  <VideoPlayer ref={ref} />
</VideoPlayer.Provider>

// Scenario 3 — Works ✅
<VideoPlayer.Provider ref={ref}>
  <VideoPlayer />
</VideoPlayer.Provider>

Proposed change

I'd suggest updating the component so that both Scenario 1 and Scenario 3 work as expected, with the ref being forwarded to the underlying <video> element.

// Scenario 1 — Works ✅
<VideoPlayer ref={ref} />

// Scenario 2 — Doesn't work ❌
// We can detect this and print a dev-only console.warn advising users to
// move the ref to the Provider instead
<VideoPlayer.Provider>
  <VideoPlayer ref={ref} />
</VideoPlayer.Provider>

// Scenario 3 — Works ✅
<VideoPlayer.Provider ref={ref}>
  <VideoPlayer />
</VideoPlayer.Provider>

Background — Why do we have a separate VideoPlayer and VideoPlayer.Provider?

The VideoPlayer relies on a VideoPlayer.Provider internally. For convenience, we typically add that provider automatically to allow users to do this:

<VideoPlayer />

instead of this:

<VideoPlayer.Provider>
  <VideoPlayer />
</VideoPlayer.Provider>

In some cases, however, users might like to have more control over video playback. To enable this, we also provide a useVideo hook (useVideo documentation) to allow programmatic control of the component. This hook not only enables interaction with the <video> element (e.g. via seek, mute, and togglePlaying functions) but also with some of our custom VideoPlayer controls (e.g. enableCC, enterFullScreen, etc). We also use the hook internally for these same controls.

If the user would like to take advantage of the useVideo hook then they will need to ensure they call useVideo from inside the VideoPlayer.Provider, since the useVideo hook relies on state/effects/etc which are managed in that provider. We've got an example of this in our documentation.

When a user has provided their own VideoPlayer.Provider, we detect this to make sure we don't automatically wrap the component in a second provider.

Implementation

The implementation here is pretty straightforward:

return context ? (
- <Root {...props} ref={ref} />
+ <Root {...props} />
) : (
- <VideoProvider>
-   <Root {...props} ref={ref} />
+ <VideoProvider ref={ref}>
+   <Root {...props}/>
  </VideoProvider>
)

I don't foresee any risk of regressions as the current implementation always forwards the ref to the Root component, which doesn't use forwardRef and so can't actually receive any of the refs which have been forwarded. The result of this is that any refs passed to VideoPlayer don't actually work (as shown in the above Scenarios).

To be safe though, I'd recommend unit and interaction tests to assert this behaviour.

As part of previous VideoPlayer-related work I have a branch which implements this change, along with related tests.

joshfarrant/videoplayer-ref-improvements