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.

Sign up for GitHub

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
Open

Prevent Focus Loss When Skip Button is Pressed #6413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9e6ee42
Prevent Focus Loss When Skip Button is Pressed
rlauuzo Dec 29, 2024
c9f6ac5
Implement the suggested changes
rlauuzo Dec 30, 2024
dcde7c3
Merge branch 'release-10.10.z' into skip-button-focus
rlauuzo Dec 30, 2024
b7974c2
Update index.js
rlauuzo Dec 31, 2024
2a6c7eb
Ensures focus shifts to the pause button
rlauuzo Jan 1, 2025
754d14f
Apply suggested changes
rlauuzo Jan 5, 2025
cf72fbb
Merge branch 'release-10.10.z' into skip-button-focus
rlauuzo Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions src/components/focusManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,22 +93,24 @@ function isCurrentlyFocusableInternal(elem) {

// Determines if a focusable element can be focused at a given point in time
function isCurrentlyFocusable(elem) {
if (elem.disabled) {
return false;
}

if (elem.getAttribute('tabindex') === '-1') {
return false;
}

if (elem.tagName === 'INPUT') {
const type = elem.type;
if (type === 'range') {
if (!elem.classList?.contains('focusable')) {
if (elem.disabled) {
return false;
}
if (type === 'file') {

if (elem.getAttribute('tabindex') === '-1') {
return false;
}

if (elem.tagName === 'INPUT') {
const type = elem.type;
if (type === 'range') {
return false;
}
if (type === 'file') {
return false;
}
}
}

return isCurrentlyFocusableInternal(elem);
Expand Down
9 changes: 9 additions & 0 deletions src/components/playback/skipsegment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ interface ShowOptions {

function onHideComplete(this: HTMLButtonElement) {
if (this) {
// 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);
}
}
Comment on lines +23 to +30
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).


this.classList.add('hide');
}
}
Expand Down
22 changes: 18 additions & 4 deletions src/controllers/playback/video/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,11 +304,15 @@ export default function (view) {
}

function slideDownToShow(elem) {
clearHideAnimationEventListeners(elem);
elem.classList.remove('hide');
dmitrylyzo marked this conversation as resolved.
Show resolved Hide resolved
elem.classList.remove('osdHeader-hidden');
}

function slideUpToHide(elem) {
clearHideAnimationEventListeners(elem);
elem.classList.add('osdHeader-hidden');
dmitrylyzo marked this conversation as resolved.
Show resolved Hide resolved
elem.addEventListener(transitionEndEventName, onHideAnimationComplete);
}

function clearHideAnimationEventListeners(elem) {
Expand All @@ -317,7 +321,7 @@ export default function (view) {

function onHideAnimationComplete(e) {
const elem = e.target;
if (elem != osdBottomElement) return;
if (elem !== osdBottomElement && elem !== headerElement) return;
elem.classList.add('hide');
elem.removeEventListener(transitionEndEventName, onHideAnimationComplete);
}
Expand All @@ -338,8 +342,17 @@ export default function (view) {
_focus(focusElement);
}
toggleSubtitleSync();
} else if (currentVisibleMenu === 'osd' && focusElement && !layoutManager.mobile) {
_focus(focusElement);
} else if (currentVisibleMenu === 'osd' && !layoutManager.mobile) {
// If no focus element is provided, try to keep current focus if it's valid,
// otherwise default to pause button
if (!focusElement) {
const currentFocus = document.activeElement;
if (!currentFocus || !focusManager.isCurrentlyFocusable(currentFocus)) {
focusElement = osdBottomElement.querySelector('.btnPause');
}
}

if (focusElement) _focus(focusElement);
}
}

Expand All @@ -354,7 +367,8 @@ export default function (view) {
toggleSubtitleSync('hide');

// Firefox does not blur by itself
if (document.activeElement) {
if (osdBottomElement.contains(document.activeElement)
|| headerElement.contains(document.activeElement)) {
Comment on lines +370 to +371
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:

jellyfin-web/src/components/subtitlesync/subtitlesync.js

Line 151 in 89867d2

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.

document.activeElement.blur();
}
}
Expand Down
Loading