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

Spacebar Pause Doubling Pause #788

Closed
Layendan opened this issue Mar 5, 2023 · 6 comments
Closed

Spacebar Pause Doubling Pause #788

Layendan opened this issue Mar 5, 2023 · 6 comments
Assignees
Labels

Comments

@Layendan
Copy link

Layendan commented Mar 5, 2023

Current Behavior:

When using safari an chromium (Edge and Chrome, not tested on others) fullscreen api pausing a video while in fullscreen pauses the video by default. The new keybinds calls pause = !pause again making the video play again.

Expected Behavior:

Either e.preventDefault() or e.stopPropagation() (Not Tested),
or check for

// Checks if fullscreen since spacebar pauses automatically
// Checks to see if using chromium since spacebar pauses automatically
if ( // @ts-ignore
    !document.webkitCurrentFullScreenElement &&
    !document.exitFullscreen ) {
        paused = !paused;
}

to pause only when not in fullscreen when pressing spacebar.

Steps To Reproduce:

(Prereqs: Rust + Node + Yarn)

  1. Clone https://github.com/Layendan/NineAnimator-Tauri
  2. Run yarn install
  3. Run yarn dev (Opens up a new window that uses Edge on Windows and Safari on MacOS)
  4. Click on any card
  5. Click on an episode
  6. Select fullscreen (Optional for chrome, maybe edge)
  7. Pause using spacebar

This runs on http://localhost:5173/ which allows you to follow steps 4-7 on any browser you wish.

Environment:

  • Framework: Svelte
  • Meta Framework: SvelteKit
  • Node: 19.6.0
  • Device: Mac M1 2020, Windows Intel CPU
  • OS: MacOS@13.1, Windows@(Replace when access to PC)
  • Browser: Safari@16.2, Chrome@109.0.5414.119, Edge@(Replace when access to PC)

Anything Else?

2023-03-05.00-06-02.mp4

After hitting play with the mouse, I press spacebar multiple times to pause, but the video pauses and unpauses.

@Layendan Layendan added the bug Something isn't working label Mar 5, 2023
@Layendan Layendan changed the title Fullscreen Pause Double Event Spacebar Pause Doubling Pause Mar 5, 2023
@Layendan
Copy link
Author

Layendan commented Mar 5, 2023

I added more info about this in this discord thread: https://discord.com/channels/742612686679965696/1082046652992467024

@mihar-22 mihar-22 added this to the Release 0.4.0 milestone Mar 6, 2023
@mihar-22 mihar-22 self-assigned this Mar 6, 2023
@Layendan
Copy link
Author

Layendan commented Mar 6, 2023

Fixed on chrome, cannot try on edge, but the issue still persists on macos safari fullscreen.

I know macos safari do not provide any default keyboard shortcuts for inline videos, but they do provide them for fullscreen.

I marked this pull request #789 as closed, but I know that it fixes it on fullscreen safari and does not break chrome, although I do not know if it breaks anything on other browsers.

@mihar-22 mihar-22 reopened this Mar 6, 2023
@mihar-22
Copy link
Member

mihar-22 commented Mar 6, 2023

For now I'd recommend disabling keyboard shortcuts if you're using native controls like so:

<media-player key-disabled>

I think we'll just automatically disable it if controls is set.

@Layendan
Copy link
Author

Layendan commented Mar 7, 2023

The problem for that is if the user is in inline video mode, they will have no controls and will be missing a few controls in fullscreen mode since WebKit does not inject that many shortcuts.

As mentioned previously, this pr #789 fixes the aforementioned problem. Allowing for vidstack's custome keyboard shortcuts without interfering with the default shortcuts.

@mihar-22
Copy link
Member

mihar-22 commented Mar 7, 2023

We can't blanket stop propagation of all trigger events because they don't belong to us, they could be provided by the user and we shouldn't stop UI events either.

I see what you're saying. The keyboard shortcuts weren't intended for use with native controls but I've also noticied native shortcuts are very limited. I'll have another review of this and see what we can do, tricky part is we can't tell which shortcuts are supported by which browser without manual testing and that could change any day.

@Layendan
Copy link
Author

Layendan commented Mar 7, 2023

We can't blanket stop propagation of all trigger events because they don't belong to us, they could be provided by the user and we shouldn't stop UI events either.

I see what you mean. That's something I haven't thought of. Sorry for pushing a faulty fix so much.

we can't tell which shortcuts are supported by which browser without manual testing and that could change any day.

If you need any help on this, you can contact me on discord and I'll try and help

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 a pull request may close this issue.

2 participants