feat: add skip forward/backward buttons #8147

usmanonazim · 2023-02-21T13:57:52Z

Description

With the upgrade to videojs font 4.0.0, font icons for skipping forward and backward have been introduced. This PR adds the option to add a skip forward or skip backward button to the control bar. These buttons are placed after the PlayToggle and before the VolumeControl, as shown in the image below.

The user also has the option to enable only one of the two skip buttons, as shown in the image below.

Code pen example with all icons: https://codepen.io/usmanomar/pen/MWqjWjj
Relevant docs: videojs/videojs.com#167

Options Description

controlBar.skipButtons.forward

Type: number

The number of seconds forward that should be skipped on button click (can be 5, 10, or 30)

controlBar.skipButtons.backward

Type: number

The number of seconds backward that should be skipped on button click (can be 5, 10 or 30)

Specific Changes proposed

Implement a SkipForward button that skips forward through a video a configurable amount of seconds
Implement a SkipBackward button skips backward through a video a configurable amount of seconds
Use the vjs-icon-forward-x and vjs-icon-replay-x for the SkipForward and SkipBackward buttons respectively

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 (sandbox/skip-buttons.html.example)
Reviewed by Two Core Contributors

codecov · 2023-02-21T14:05:14Z

Codecov Report

Merging #8147 (9a8d070) into main (0a47175) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8147      +/-   ##
==========================================
+ Coverage   82.02%   82.14%   +0.11%     
==========================================
  Files         110      112       +2     
  Lines        7344     7391      +47     
  Branches     1773     1782       +9     
==========================================
+ Hits         6024     6071      +47     
  Misses       1320     1320

Impacted Files	Coverage Δ
src/js/control-bar/control-bar.js	`83.33% <ø> (ø)`
src/js/control-bar/skip-buttons/skip-backward.js	`100.00% <100.00%> (ø)`
src/js/control-bar/skip-buttons/skip-forward.js	`100.00% <100.00%> (ø)`

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

mister-ben · 2023-02-21T15:38:31Z

The numbers on the icons are pretty illegible at the default size of the controls. That may well be flagged in an acessiblity audit.

src/js/control-bar/skip-buttons/skip-forward.js

usmanonazim · 2023-02-21T16:19:57Z

The numbers on the icons are pretty illegible at the default size of the controls. That may well be flagged in an acessiblity audit.

Do you know if there's a good way to fix that?

mister-ben · 2023-02-21T20:34:44Z

The numbers on the icons are pretty illegible at the default size of the controls. That may well be flagged in an acessiblity audit.

Do you know if there's a good way to fix that?

I think it needs to be different icons that are readble at that size, or plain arrows. The control text could (and for accessibility probably should) state the increment to be skipped by.

roman-bc-dev · 2023-02-22T17:57:21Z

The numbers on the icons are pretty illegible at the default size of the controls. That may well be flagged in an acessiblity audit.

Do you know if there's a good way to fix that?

The icons used here are from the standard Material Design set. Unless we jerry-rig a solution with existing videojs font icons, we'd need to decide on brand new ones and then update the font repo with them to then be used here, which seems outside the scope of the changes described.

@mister-ben The empty round arrow icon is already used for replay UI and plain arrows are not descriptive of the intended action here. Are there any other icons from the Material Design or Font Awesome collections that you would recommend for legibility?

…forward/back

mister-ben · 2023-02-23T11:04:51Z

I've seen the rewind / fast forward icons used for this in OTT apps, but they are also problematic as they can be understood as changing the playback rate.

Some also use a similar looped arrow with a double arrowhead, but there's no such icon in Material Icons or Font Awesome. Netflix is like this. (They also show numbers, but have the icons large enough for this to work.)

Here I used rotated arrows to distinguish them from replay.

Maybe showing the number, larger, when the button has focus could work? If we really want numbers they need to be readable, I might try tweaking the SVG icons.

It might be easier if we could consider using "restart alt" from Material icons for replay with a fuller, broken loop.

mister-ben · 2023-02-23T14:36:39Z

Here's one idea, modifying Material Icons's SVG

usmanonazim · 2023-02-23T15:20:41Z

Here's one idea, modifying Material Icons's SVG

That looks great - thank you for the suggestion! Is it ok if I use that?

roman-bc-dev · 2023-02-23T15:23:20Z

Here's one idea, modifying Material Icons's SVG

Love that icon idea.

@usmanonazim Are you comfy with modifying SVGs? You have the tools to do so?

mister-ben · 2023-02-23T15:32:38Z

Thinking about it, the icon could just be that partial arrow, and the number can be text. That's a lot less SVG work, we don't have to impose the increments that can be used, and the number can be localised for languages that don't use "arabic numerals", like actual arabic numbers.

usmanonazim · 2023-02-23T16:09:33Z

Thinking about it, the icon could just be that partial arrow, and the number can be text. That's a lot less SVG work, we don't have to impose the increments that can be used, and the number can be localised for languages that don't use "arabic numerals", like actual arabic numbers.

Just to clarify, do you mean that the icon used would just be the partial arrow, and then the control text would specify the number, or that the number would go underneath the arrow icon? If it's the second one, setting a number that's too large (ie 1000) could ruin the look of the icon right?

mister-ben · 2023-02-23T17:17:48Z

The control text should be something useful for screenreaders, like "skip back 10 seconds".

I've updated the codepen example with a version that adds a span for the number, with aria-hidden so it's not read out, and can be localised to match the player language.

You're right, a high number won't work well. Maybe accept =< 99?

roman-bc-dev · 2023-02-23T20:31:42Z

You'd also have to consider dynamic typeface sizing/interactions within an SVG icon, which gets finicky, especially with hover effects, etc. For the purposes of the intended work here, it may be best to implement the expected icons with the 5, 10, and 30 increments as SVGs and then build a more expanded solution later down the line.

usmanonazim · 2023-02-24T13:07:08Z

You'd also have to consider dynamic typeface sizing/interactions within an SVG icon, which gets finicky, especially with hover effects, etc. For the purposes of the intended work here, it may be best to implement the expected icons with the 5, 10, and 30 increments as SVGs and then build a more expanded solution later down the line.

I agree - I've created a codepen example with the new icons. @mister-ben @roman-bc-dev please let me know what you think of them 😄

mister-ben · 2023-02-24T13:55:44Z

Nice. Are we sticking with SVG or getting these into the font? (In general I'd prefer to move to SVG directly).

usmanonazim · 2023-02-24T14:41:46Z

Nice. Are we sticking with SVG or getting these into the font? (In general I'd prefer to move to SVG directly).

I was planning to replace the existing icons for replay-x and forward-x in font with the SVGs that I've created.

usmanonazim · 2023-02-27T11:15:20Z

I've now created a PR in videojs/font to update the existing icons: videojs/font#49

mister-ben · 2023-02-28T16:43:58Z

src/js/control-bar/skip-buttons/skip-backward.js

+    this.skipTime = this.getSkipBackwardTime();
+
+    if (this.skipTime && this.validOptions.includes(this.skipTime)) {
+      this.controlText(`Skip backward ${this.skipTime} seconds`);


This way means the translation file need a translation for each allowed increment. It's relatively manageable with 3 permitted values, but it is possible to use a variable in these translations: this.localize('Skip backward {seconds} seconds', [this.skipTime])

Either way there should be at least an entry for each string in en.json, so there's a template to translate from.

I agree.

It should be safe to pass a pre-localized string to controlText, like:

this.controlText(this.localize('Skip backward {seconds} seconds', [this.skipTime]));

The reason for this is that translators will want a tokenized static string. The {seconds} token tells them what sort of value to expect in that position.

Then the en.json should have a new entry:

"Skip backward {seconds} seconds": "Skip backward {seconds} seconds"

The reason to include these in en.json is that is used as a reference by the script that parses out missing translations.

EDIT: adding a bit more context here...

The controlText method will pass the string through localize again, but this will be a pass-thru because the replacement of {seconds} will have already happened. It's unlikely there would be a matching localization key for something like Skip backward 1 seconds.

Oh ok thank you for the explanation! How does translation normally work - am I only expected to add this to en.json and then translators can add the translations to the other json files?

mister-ben

The deploy preview is looking good! Just the note about translations. At the least there should be an en.json. If swithcing to variables is complicated we can tackle that later (and there's an issue with variables and languagechange event that is out of scope for this PR).

Could you also add a link to the sandbox page in /index.html?

misteroneill

Looks great, Usman! I think just addressing the localization thing should be all that's needed.

misteroneill · 2023-02-28T17:55:29Z

src/js/control-bar/skip-buttons/skip-backward.js

+    this.skipTime = this.getSkipBackwardTime();
+
+    if (this.skipTime && this.validOptions.includes(this.skipTime)) {
+      this.controlText(`Skip backward ${this.skipTime} seconds`);


I agree.

It should be safe to pass a pre-localized string to controlText, like:

this.controlText(this.localize('Skip backward {seconds} seconds', [this.skipTime]));

The reason for this is that translators will want a tokenized static string. The {seconds} token tells them what sort of value to expect in that position.

Then the en.json should have a new entry:

"Skip backward {seconds} seconds": "Skip backward {seconds} seconds"

The reason to include these in en.json is that is used as a reference by the script that parses out missing translations.

EDIT: adding a bit more context here...

The controlText method will pass the string through localize again, but this will be a pass-thru because the replacement of {seconds} will have already happened. It's unlikely there would be a matching localization key for something like Skip backward 1 seconds.

misteroneill · 2023-02-28T17:55:57Z

src/js/control-bar/skip-buttons/skip-forward.js

+    this.skipTime = this.getSkipForwardTime();
+
+    if (this.skipTime && this.validOptions.includes(this.skipTime)) {
+      this.controlText(`Skip forward ${this.skipTime} seconds`);


Same comment here about localization.

I've now moved the control text strings from skip-forward and skip-backward to en.json for localization.

usmanonazim · 2023-03-01T10:03:31Z

The deploy preview is looking good! Just the note about translations. At the least there should be an en.json. If swithcing to variables is complicated we can tackle that later (and there's an issue with variables and languagechange event that is out of scope for this PR).

Could you also add a link to the sandbox page in /index.html?

I've added the link to the sandbox page now 😁

misteroneill

LGTM, thanks Usman!

* remove duplicate icons from icon example * create initial forward and back button classes * add logic for back/forward buttons on click * change icon used based on option passed into player * move logic from forward and back buttons into one component * add jsdoc comments for clarity * create initial test file * refactor button logic into separate files * update skip button example and add test files * test both the forward and backward buttons * test handleClick fns for both forward and backward btns * update skip buttons example * update jsdocs for skip backward and forward buttons * make control text accessible and use seekableEnd/Start when skipping forward/back * update font version to use updated icons * set control text only if config is valid * add link to sandbox page & use localization * update translations needed

usmanonazim added 13 commits February 13, 2023 15:27

remove duplicate icons from icon example

7201818

create initial forward and back button classes

02a0a23

add logic for back/forward buttons on click

465ac8a

change icon used based on option passed into player

eaf8e42

move logic from forward and back buttons into one component

ee6a2b0

add jsdoc comments for clarity

2614c36

create initial test file

0e5b43f

refactor button logic into separate files

d5b9651

update skip button example and add test files

09cbe2a

test both the forward and backward buttons

d32de74

test handleClick fns for both forward and backward btns

eeb0efd

update skip buttons example

560ab73

update jsdocs for skip backward and forward buttons

5ba8cfd

usmanonazim marked this pull request as ready for review February 21, 2023 15:17

mister-ben reviewed Feb 21, 2023

View reviewed changes

src/js/control-bar/skip-buttons/skip-forward.js Show resolved Hide resolved

make control text accessible and use seekableEnd/Start when skipping …

d087fc7

…forward/back

update font version to use updated icons

4c5aa13

usmanonazim requested a review from mister-ben February 27, 2023 16:20

usmanonazim mentioned this pull request Feb 28, 2023

docs: skip forward/backward buttons videojs/videojs.com#167

Merged

set control text only if config is valid

6964f39

mister-ben reviewed Feb 28, 2023

View reviewed changes

mister-ben requested changes Feb 28, 2023

View reviewed changes

misteroneill requested changes Feb 28, 2023

View reviewed changes

usmanonazim added 2 commits March 1, 2023 09:59

add link to sandbox page & use localization

45bc81e

update translations needed

c8a26f0

Merge branch 'main' into feat/forward-back-buttons

9a8d070

usmanonazim requested review from misteroneill and mister-ben March 1, 2023 10:23

misteroneill approved these changes Mar 3, 2023

View reviewed changes

mister-ben approved these changes Mar 6, 2023

View reviewed changes

usmanonazim merged commit 8f3f32c into videojs:main Mar 6, 2023

usmanonazim deleted the feat/forward-back-buttons branch March 6, 2023 09:52

ittipatken mentioned this pull request May 5, 2023

perf: update videojs-related dependencies docchula/el-player#6

Merged

toby63 mentioned this pull request May 8, 2023

[Feature Request]: Add skip forward/backward buttons FreeTubeApp/FreeTube#3520

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add skip forward/backward buttons #8147

feat: add skip forward/backward buttons #8147

usmanonazim commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

mister-ben commented Feb 21, 2023

usmanonazim commented Feb 21, 2023

mister-ben commented Feb 21, 2023

roman-bc-dev commented Feb 22, 2023

mister-ben commented Feb 23, 2023

mister-ben commented Feb 23, 2023

usmanonazim commented Feb 23, 2023

roman-bc-dev commented Feb 23, 2023

mister-ben commented Feb 23, 2023

usmanonazim commented Feb 23, 2023

mister-ben commented Feb 23, 2023 •

edited

Loading

roman-bc-dev commented Feb 23, 2023

usmanonazim commented Feb 24, 2023

mister-ben commented Feb 24, 2023

usmanonazim commented Feb 24, 2023 •

edited

Loading

usmanonazim commented Feb 27, 2023 •

edited

Loading

mister-ben Feb 28, 2023

misteroneill Feb 28, 2023 •

edited

Loading

usmanonazim Feb 28, 2023

mister-ben left a comment

misteroneill left a comment

misteroneill Feb 28, 2023 •

edited

Loading

misteroneill Feb 28, 2023

usmanonazim Mar 1, 2023

usmanonazim commented Mar 1, 2023

misteroneill left a comment

feat: add skip forward/backward buttons #8147

feat: add skip forward/backward buttons #8147

Conversation

usmanonazim commented Feb 21, 2023 • edited Loading

Description

Options Description

Specific Changes proposed

Requirements Checklist

codecov bot commented Feb 21, 2023 • edited Loading

Codecov Report

mister-ben commented Feb 21, 2023

usmanonazim commented Feb 21, 2023

mister-ben commented Feb 21, 2023

roman-bc-dev commented Feb 22, 2023

mister-ben commented Feb 23, 2023

mister-ben commented Feb 23, 2023

usmanonazim commented Feb 23, 2023

roman-bc-dev commented Feb 23, 2023

mister-ben commented Feb 23, 2023

usmanonazim commented Feb 23, 2023

mister-ben commented Feb 23, 2023 • edited Loading

roman-bc-dev commented Feb 23, 2023

usmanonazim commented Feb 24, 2023

mister-ben commented Feb 24, 2023

usmanonazim commented Feb 24, 2023 • edited Loading

usmanonazim commented Feb 27, 2023 • edited Loading

mister-ben Feb 28, 2023

Choose a reason for hiding this comment

misteroneill Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

usmanonazim Feb 28, 2023

Choose a reason for hiding this comment

mister-ben left a comment

Choose a reason for hiding this comment

misteroneill left a comment

Choose a reason for hiding this comment

misteroneill Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

misteroneill Feb 28, 2023

Choose a reason for hiding this comment

usmanonazim Mar 1, 2023

Choose a reason for hiding this comment

usmanonazim commented Mar 1, 2023

misteroneill left a comment

Choose a reason for hiding this comment

usmanonazim commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

mister-ben commented Feb 23, 2023 •

edited

Loading

usmanonazim commented Feb 24, 2023 •

edited

Loading

usmanonazim commented Feb 27, 2023 •

edited

Loading

misteroneill Feb 28, 2023 •

edited

Loading

misteroneill Feb 28, 2023 •

edited

Loading