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

Prevent Focus Loss When Skip Button is Pressed #6413

Open
wants to merge 7 commits into
base: release-10.10.z
Choose a base branch
from

Conversation

rlauuzo
Copy link
Contributor

@rlauuzo rlauuzo commented Dec 29, 2024

Addresses the focus-related issue when interacting with the skip button during playback.

  1. Ensures that after the skip button is pressed and the OSD is visible, the focus automatically moves to the pause button.
  2. Updates the logic to retain focus on the skip button when the OSD hides.

@rlauuzo rlauuzo requested a review from a team as a code owner December 29, 2024 19:43
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Dec 29, 2024

Cloudflare Pages deployment

Latest commit cf72fbb
Status ✅ Deployed!
Preview URL https://be0b95b8.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@thornbill thornbill added bug Something isn't working stable backport Backport into the next stable release labels Dec 29, 2024
@thornbill thornbill added this to the v10.10.4 milestone Dec 29, 2024
src/components/focusManager.js Outdated Show resolved Hide resolved
Comment on lines +23 to +30
// Handle focus after the hide transition completes
if (document.activeElement === this) {
this.blur();
const pauseButton = document.querySelector('.btnPause');
if (pauseButton && focusManager.isCurrentlyFocusable(pauseButton)) {
focusManager.focus(pauseButton);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be considered as a temporary solution. We need to move the skip button to the video OSD (maybe in 10.11).

src/controllers/playback/video/index.js Show resolved Hide resolved
src/controllers/playback/video/index.js Show resolved Hide resolved
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Jan 5, 2025

@rlauuzo
Copy link
Contributor Author

rlauuzo commented Jan 5, 2025

Hey, one thing I noticed, unrelated to the PR, is that on my TV (WebOS 6; this doesn't happen on PC browsers), when the button is pressed, it briefly reappears after the seek and then hides again.

@dmitrylyzo
Copy link
Contributor

Hey, one thing I noticed, unrelated to the PR, is that on my TV (WebOS 6; this doesn't happen on PC browsers), when the button is pressed, it briefly reappears after the seek and then hides again.

Maybe it seeks on the keyframe that is before the end of the segment?
Large intervals between keyframes make skipping unreliable.

Comment on lines +370 to +371
if (osdBottomElement.contains(document.activeElement)
|| headerElement.contains(document.activeElement)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remembered about SubtitleSync - it was blurred by the OSD. Currently we only blur the elements that belong to the OSD. So we should make SubtitleSync to blur itself near this:

subtitleSyncContainer.classList.add('hide');

We could also just revert the blurring to if (document.activeElement) since we are hiding everything anyway. Then we probably need to fix the refocusing on the pause button when hiding the skip button.

But since, as I said, it (blur) is probably unnecessary now, we can leave it as is (for now) and remove it completely later.

For the record, this is reproducible in Firefox 72. We don't have to support such an old Firefox because it can/should be updated unlike the web engine in TV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing: SubtitleSync can't be accessed using navigation keys if the skip button is visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing: SubtitleSync can't be accessed using navigation keys if the skip button is visible.

Yes, this is a problem with current navigation in general: we don't have a "navmap", the focus manager moves along horizontal and vertical "lanes".

We could probably search for the nearest element within the container crossed by the lane. Obviously not for this PR.

It would also be better to take into account how the navigation will be implemented in React.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stable backport Backport into the next stable release
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants