Skip to content

Accessibility Feature: Configurable Keyboard shortcut: toggleFullscreenOnfPress: V2#3270

Closed
Ketan2010 wants to merge 6 commits intoshaka-project:mainfrom
Ketan2010:master
Closed

Accessibility Feature: Configurable Keyboard shortcut: toggleFullscreenOnfPress: V2#3270
Ketan2010 wants to merge 6 commits intoshaka-project:mainfrom
Ketan2010:master

Conversation

@Ketan2010
Copy link

@Ketan2010 Ketan2010 commented Mar 22, 2021 •
edited
Loading

Description

for better accessibility keyboard shortcuts need to be added. This PR gives implementation of keyboard shortcut (f) which toggles fullscreen view of video. Which can be configured as existing UIConfigurations as follow.
Defaults to false

const config = {
    toggleFullscreenOnfPress: true,
 };

Such more shortcuts can be implemented for other frequent controls like Picture in Picture, Mute and Unmute, Captions etc.

Fixes # (partially)
#3223

Screenshots (optional)

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@Ketan2010 Ketan2010 mentioned this pull request Mar 22, 2021
Closed
8 tasks
@surajkumar-sk
Copy link
Contributor

surajkumar-sk commented Mar 22, 2021 •
edited
Loading

@Ketan2010 you added this.pressedKeys_.delete(event.key); on line 1274, you are not adding event to pressed keys so you don't need to delete it, please remove the line, it might cause unnecessary bugs with navigation using tab and shift.

@surajkumar-sk
Copy link
Contributor

surajkumar-sk commented Mar 22, 2021 •
edited
Loading

@Ketan2010 and also I don't think you would need lines 1267 and 1269 also , because you are listening to keyboard event on window and it doesn't matter if any container element is active , you could directly call mousemove() function to display the control panel elements. When shortcut are used.

@surajkumar-sk
Copy link
Contributor

surajkumar-sk commented Mar 22, 2021

@Ketan2010 i am sorry for posting with long breaks, did you test the working of your code , that is when f is pressed it goes to full screen and video controls and seek bar shows up .

@Ketan2010
Copy link
Author

Ketan2010 commented Mar 22, 2021 •
edited
Loading

@surajkumar-sk,
Yes, I checked with that before commit. and it works fine.

Thanks you for all suggestions!
PR is ready for review.

@joeyparrish joeyparrish removed the gsoc label Sep 27, 2021
@avelad avelad added type: enhancement New feature or request type: accessibility An accessibility issue labels May 2, 2022
@avelad
Copy link
Member

avelad commented May 10, 2022

@Ketan2010 I see that there are conflicts to be able to merge, can you rebase?

@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label May 10, 2022
@avelad
Copy link
Member

avelad commented May 18, 2022

Closing due to inactivity.

@avelad avelad closed this May 18, 2022
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
@github-actions github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

status: archived Archived and locked; will not be updated type: accessibility An accessibility issue type: enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Ketan2010 @surajkumar-sk @avelad @joeyparrish @michellezhuogg