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

feat: playback rate button now opens the menu rather than changing the playback rate #7779

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented May 25, 2022

Description

This comes out of discussion in #7121 and its associated PR, #7193.

Currently, in Video.js 7, the PlaybackRateMenuButton will change to the next playback rate when it is clicked, cycling through the list of items. However, this approach has accessibility issues (which were addressed in #7193 as far as I can see) necessitating a workaround.

In Video.js 8, the PlaybackRateMenuButton will behave like other buttons when it is clicked: it will open its associated menu persistently. This is considered a breaking change because it changes the behavior of a control bar menu button.

I'd love to get feedback from @gkatsev and/or @OwenEdwards on whether or not further accessibility updates are warranted here - or if we could potentially remove the workaround introduced in #7193.

Specific Changes proposed

  • Remove the handleClick method, which causes us to fall back to MenuButton#handleClick
  • Remove the updateARIAAttributes method, which was never called anywhere.
  • Add some basic tests because it's the right thing to do!

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
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #7779 (0c8ac86) into next (337a2a0) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             next    #7779      +/-   ##
==========================================
+ Coverage   81.05%   81.07%   +0.01%     
==========================================
  Files         113      113              
  Lines        7441     7433       -8     
  Branches     1802     1802              
==========================================
- Hits         6031     6026       -5     
+ Misses       1410     1407       -3     
Impacted Files Coverage Δ
...ar/playback-rate-menu/playback-rate-menu-button.js 100.00% <ø> (+3.70%) ⬆️
src/js/tracks/text-track.js 92.94% <0.00%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 337a2a0...0c8ac86. Read the comment docs.

@misteroneill misteroneill marked this pull request as ready for review May 26, 2022 23:20
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Seems like it was that easy.
Did a quick test with VoiceOver and seems to work. For whatever reason, VoiceOver is added 1 in front of the actively selected item, both for this and for the captions menu. Probably worth looking at, but it's separate from this PR.

@misteroneill misteroneill merged commit 4cdb2ab into next Jun 2, 2022
@misteroneill misteroneill deleted the pbr-btn branch June 2, 2022 16:07
misteroneill added a commit that referenced this pull request Nov 23, 2022
…e playback rate (#7779)

BREAKING CHANGE: This changes the behavior of the playback rate button.
misteroneill added a commit that referenced this pull request Nov 23, 2022
…e playback rate (#7779)

BREAKING CHANGE: This changes the behavior of the playback rate button.
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
…e playback rate (videojs#7779)

BREAKING CHANGE: This changes the behavior of the playback rate button.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants