-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add lyrics auto scroll #5585
Add lyrics auto scroll #5585
Conversation
Maybe worth a PR against 10.9? IMHO it's a bit weird behaviour to not scroll on timed lyrics |
f0dab2b
to
904b8bf
Compare
Yeah, I agree. Changed the base to 10.9.z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work.
There's a conflict between focus scroll and lyrics scroll: when focused element leaves the view when some distant lyrics is active. Tbf, I don't think we should fix it right now (because I don't know yet how 😅 ).
👆 in TV mode
7fed6fe
to
fa9b0e6
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desktop and Mobile display modes work fine.
In TV display mode, this autoFocuser
call breaks the instant scroll to the current lyric.
jellyfin-web/src/controllers/lyrics.js
Line 142 in 15e35fa
autoFocuser.autoFocus(view); |
Called after
scrollToElement
, selecting the first lyric.
We could try replacing it with focusing the current lyrics (using focusManager
).
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look fine to me but this could probably use another round of testing. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
webOS 1, 5 - OK
Chrome 124 - OK
Firefox 126 - OK
Changes
Adds autoscrolling to lyrics. Will stop autoscrolling if wheel or up key or down key is used. Can continue autoscrolling by selecting the lyrics line.
Issues
Fixes #5487