Classifier: Custom Video Controls

[goplayoutside3]

Blocked because this PR is based on #3645.

Package

lib-classifier

Linked Issue and/or Talk Post

#3612 and #3613

Describe your changes

This is a complete rewrite of the VideoController component. The controls are designed to sit below a html video component so that future drawing tools can be positioned over a video subject. Not all video subject projects will use drawing tools, but for consistency these custom video controls will always display.

File Re-organization

I consolidated all aspects of the controls into one component: VideoController. It lives in a /components folder in SingleVideoViewer. I also moved the react-player component into SingleVideoViewerContainer for readability. All of its handler functions are defined in SingleVideoViewerContainer.

Controls Functionality

I improved the video scrubbing by memoizing the react-player so it's onProgress and setTimeStamp functions don't call at the same time.

I edited formatTimeStamp() to display timestamps in the same units as popular video players such as native html video controls, youtube, vimeo, etc. (i.e. 0:20 means 20 seconds).

I added the ability to change volume and display a video subject in fullscreen.

Controls Styling

Screenshots for review:
Note that there is a 1px white border on top of the video controls because the Woodpecker Cavity Cam subjects have black borders, and I want the video controls' dimensions to be obvious. The white border only appears in the screenshots, not in the code.

Should the controls be re-arranged on a mobile view?

Documentation and Tests

I added documentation in a README file for VideoController and SingleVideoViewerContainer. I added unit tests using React Testing Library for VideoController.

How to Review

This PR is part of an effort to improve the video subject viewer code for migrating Woodpecker Cavity Cam to FEM, so I recommend that project for reviewing functionality.

Things to check a volunteer can do:

• Play / Pause the video
• See the timestamp and duration of video subject
• Scrub the video with the slider
• Change the playback rate
• Change the volume
• View the video subject in fullscreen

Video Controller also appears in SingleVideoViewer's storybook.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

• Tests are passing locally and on Github
• Documentation is up to date and changelog has been updated if appropriate
• You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
• FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
• FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

• All pages of a FEM project load: Home Page, Classify Page, and About Pages
• Can submit a classification
• Can sign-in and sign-out
• The component is accessible
  • Can be used with a screen reader BBC guide to VoiceOver
  • Can be used from the keyboard WebAIM guide to keyboard testing
  • It is passing accessibility checks in the storybook

New Feature

• The PR creator has listed user actions to use when testing the new feature
• Unit tests are included for the new feature
• A storybook story has been created or updated
  • Example of SlideTutorial component with docgen comments, and its story

[shaunanoordin]

PR Review (Update)

Follows initial review

The functionality of the Single Video Viewer and, more importantly, the rewritten Video Controller mostly checks out OK. 👍 I do have some comments, as usual:

• Basic functionality: 👍
  • I can view a video subject.
  • I can click on the play/pause button to play/pause the video.
  • I can view the current time and full duration of the video, e.g. 0:03 / 0:10
  • I can click on the video-time-scrubber-bar (what IS this called?) OR drag the time-position-handle-thing (what IS this also called?) to change the currently viewed time of the video.
    • ⚠️ NOTE: there is some lag between setting the time, and the video jumping to that time.
  • I can click on the the playback rate to display speed options, then select one to change the speed the video is played
  • I can click on the volume button to display and adjust the video volume.
  • I can click on the fullscreen button to enter fullscreen mode. (I can also exit this mode.)
• Accessibility (keyboard controls):
  • I can use a keyboard to navigate through the interactable video controller elements (play button, time-scrubber-bar, playback rate dropdown/dropup, volume controls, fullscreen button)
  • I can press Spacebar or Enter on the play/pause button to play/pause the video.
    • ⚠️ NOTE: there seems to be a small delay on the time-scrubber-bar thing. Like if I press pause at 0:02, the video correctly pauses, but the time-scrubber-bar thing keeps moving forward for a split-second as if it has some momentum then stops at 0:03. It doesn't make the video controls unusable, but it's weird.
  • ❌ I can't use the left/right arrow keys, on the time-scrubber-bar thing, to step back/step forward through the video time. Essentially, I can't use the keyboard to scrub.
  • ❌ While I'm on the time-scrubber-bar thing, pressing enter/spacebar does not pause/play the video. (This is standard behaviour for, say, a YouTube player)
  • I can use the keyboard to view & select the playback speed
  • ❌ I can use the keyboard to open the volume menu, but I can't use the up/down keys to adjust the volume on an open volume menu.
  • I can press Spacebar or Enter on the Fullscreen button to enable fullscreen mode, then press Esc to escape fullscreen mode.
• Accessibility (labelling/screen readers):
  • ❔ I have no useful comments here, sorry - might be worth seeking an additional opinion on this.
• Advanced?
  • ❓ When the video is in fullscreen, the video controller appears OVER (overlaps with) the video.
    • I'm not sure if fullscreen would affect any of the drawing tasks, but that's probably a problem for another PR.
    • I'm also not sure if this problem is specific to the size of the video + my full screen size.

Testing

Tested on localhost with macOS + Chrome 105, with Woodpecker Cavity Cam (production) as the primary test project.

Testing steps: see basic functionality/accessibility/etc above.

Status

Basic functionality checks out in most cases, so this PR gets a 👍 from me.

One major thing to note is that the keyboard controls are far more limited than what you'd expect from a standard video player (see all the ❌ icons above) - but I think these keyboard controls can be addressed in a separate PR. I can write the missing functionality up as an issue, if that helps?

[eatyourgreens]

That’s odd. The range input responds to the cursor keys by default, when it has focus.

I’m not sure that we should merge PRs that have an error like this. Thoughts?

[eatyourgreens]

When a range opens in a popup, like the volume control, you probably need to call volumeControl.focus() to focus it. Grommet might not do that for you. It’s also good practice to focus the first focusable element when a popup opens, and check that focus returns to the original button when the popup closes.

EDIT: the volume control should also test that the range input has focus when the popup opens.

[eatyourgreens]

Regarding poor performance of the range input for the scrubber, have you considered using an uncontrolled input rather than a controlled input. There’s a lot of internal component state at the moment, which would make the controls laggy. The current play time can be read from the video, via the Media API. Updating it via setState adds extra render cycles, which I think is what leads to the lag between pausing the video and updating the progress bar.

[shaunanoordin]

I’m not sure that we should merge PRs that have an error like this. Thoughts?

👌 I'm OK to have this PR merged first, and then have the keyboard issues fixed in a separate PR. My rationale: (1) this PR doesn't break anything that exists, and (2) a second smaller PR would be very helpful in understanding what was done to solve the keyboard issues for the video player.

To expand on (1), this is how the current video controls work on master (ad1d18b):

• When focused on the time-scrub-bar,
  • ...you can use keyboard left/right to adjust current time, but at a per millisecond rate. (Chrome 105, FF 105) This makes it practically unusable, as it takes approx 5-6 minutes to rewind the video by 1 second.
  • ...you still can't use spacebar to pause/play the video
• There's no explicit volume control.

(Side note: the poor/laggy performance of the video control when you scrub through the time bar also exists on the old version of the video player.)

[eatyourgreens]

The scrubber in the current player can be controlled from the keyboard, so that would be a breaking change here.

[eatyourgreens]

Actually I might be thinking of the video player in live projects. That should be our reference for breaking changes.

EDIT: I checked Woodpecker Cavity Cam in Firefox. The left and right cursor keys skip the scrubber forward and backwards in 5s increments. Is that a browser default?

[eatyourgreens]

Also on Woodpecker Cavity Cam, in Firefox, I can mute/unmute the volume control with the spacebar. When volume is unmuted, I can operate the volume slider with the cursor keys.

Screen.Recording.2022-10-03.at.15.01.19.mov

[goplayoutside3]

@eatyourgreens @SeanMiller @shaunanoordin how do you feel about the following scenario? I'd like to propose we do the following to simplify all the discussion initiated by this PR.

Our goal during FEM migration of the first few pathfinder projects is to migrate classification in an uninterrupted manner from PFE to FEM. Woodpecker Cavity Cam is a survey task project with video subjects without drawing tools. In PFE, the subjects are displayed with native browser controls that automatically show with an HTML video element.

Proposition: I modify this PR to show FEM custom video controls ONLY if a project has drawing tools, and Woodpecker Cavity Cam in FEM would have the same UX as it currently does in PFE. Then, I create Github Issues for each point you've commented in this PR as future work toward FEM video subjects with drawing tools and therefore with custom video controls.

While developing the video subject viewer specifically for an un-launched video + drawing project, I think we've missed the opportunity to build the simple video subject viewer based on PFE.

Thoughts?

[eatyourgreens]

That sounds like a really good idea to me. I'd definitely avoid the extra work of creating controls that already exist in the browser. 👍

Maybe have two video player components, rather than one player with branching logic and optional custom controls?

[shaunanoordin]

Proposition: I modify this PR to show FEM custom video controls ONLY if a project has drawing tools, and Woodpecker Cavity Cam in FEM would have the same UX as it currently does in PFE. Then, I create Github Issues for each point you've commented in this PR as future work toward FEM video subjects with drawing tools and therefore with custom video controls.

That gets a solid thumbs up from me. 👍

[eatyourgreens]

One thought: I think we may still need custom controls for video speed. Speed control might be available on Android and iOS but I'm not sure about other operating systems.

[seanmiller26]

Proposition: I modify this PR to show FEM custom video controls ONLY if a project has drawing tools, and Woodpecker Cavity Cam in FEM would have the same UX as it currently does in PFE. Then, I create Github Issues for each point you've commented in this PR as future work toward FEM video subjects with drawing tools and therefore with custom video controls.

If this means far less work, then I'm all for it. I'd like to keep in mind uniformity and conciseness as an end goal, though. We don't want to have multiple different players etc for slightly different subject tasks - which could lead to confusion from users.
So eventually transitioning to both these tasks having the same video controls would be ideal, even if that means waiting until we have the time to do it properly.

[goplayoutside3]

❓ When the video is in fullscreen, the video controller appears OVER (overlaps with) the video.
I'm not sure if fullscreen would affect any of the drawing tasks, but that's probably a problem for another PR.
I'm also not sure if this problem is specific to the size of the video + my full screen size.

Re: @shaunanoordin I don't think fullscreen works with drawing tools, so I've added a check that says to only display the fullscreen button when drawing tools are disabled.

One thought: I think we may still need custom controls for video speed. Speed control might be available on Android and iOS but I'm not sure about other operating systems.

Re: @eatyourgreens Playback speed is available in native desktop browser control. PFE does display custom buttons though, so I wrote a to-do Issue to add them to FEM as well (Issue #3758).

Update

I added an ADR to this PR describing the decision to use native browser video controls to aid in PFE to FEM migration. I also added stories and unit tests for the drawing tools disabled versus enabled UI. Accessibility issues are written in Issue #3759. I'm going to merge this PR this evening.