Skip to content

Add support for keyboard navigation in theme switcher#8167

Closed
Inverle wants to merge 2 commits intoFreshRSS:edgefrom
Inverle:theme-switcher-keyboard-nav
Closed

Add support for keyboard navigation in theme switcher#8167
Inverle wants to merge 2 commits intoFreshRSS:edgefrom
Inverle:theme-switcher-keyboard-nav

Conversation

@Inverle
Copy link
Member

@Inverle Inverle commented Oct 30, 2025

Partially solves issue (navigating with keyboard) described in #8152 (comment)

The JS part doesn't work in SeaMonkey, I have not investigated why.

@Alkarex Alkarex added this to the 1.28.0 milestone Oct 30, 2025
@Alkarex Alkarex added Shortcuts ⌨ UI 🎨 User Interfaces labels Oct 30, 2025
@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2025

The JS part doesn't work in SeaMonkey, I have not investigated why.

I am more surprised that it works in other browsers. If I am not mistaken, <label> elements are not supposed to be directly focusable but rather depend on their associated <input> to be focussed, which happens to be hidden in our case here.

I think the underlying method to change style should be reworked to be simpler and more semantically valid.

We can merge here in the meantime, but it adds to something that is not so nice.

Alkarex
Alkarex reviewed Nov 5, 2025
View reviewed changes
p/scripts/extra.js
// Keyboard support for theme switcher navigation
const wrapper = parent.querySelector('div.theme-preview-list-wrapper');
if (wrapper) {
wrapper.addEventListener('keypress', function (e) {
Copy link
Member

@Alkarex Alkarex Nov 5, 2025

Choose a reason for hiding this comment

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

keypress is deprecated: https://developer.mozilla.org/en-US/docs/Web/API/Element/keypress_event

Copy link
Member Author

@Inverle Inverle Nov 5, 2025

Choose a reason for hiding this comment

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

Interesting

Copy link
Member

@Frenzie Frenzie Nov 5, 2025

Choose a reason for hiding this comment

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

Keydown is preferable for most things anyway. Keyup/press just adds a bit of extra delay.

@Frenzie
Copy link
Member

Frenzie commented Nov 5, 2025

Yes, I'd rather avoid such complications and just simplify it to a select, since then you also wouldn't need to move left or right ten times to reach a desired theme. All it'd need is an onchange → update image. Though I suppose if you really want you can also use role=radio and the like to construct whatever you want.

@Inverle
Copy link
Member Author

Inverle commented Nov 5, 2025

We can merge here in the meantime, but it adds to something that is not so nice.

I'm gonna make a second PR for the <select> approach and close this one maybe

Inverle added a commit to Inverle/FreshRSS that referenced this pull request Nov 5, 2025
@Inverle
Replace arrow navigation in theme switcher with <select>
c6d3f1d 
Follow-up to FreshRSS#8167
Inverle added a commit to Inverle/FreshRSS that referenced this pull request Nov 5, 2025
@Inverle
Replace arrow navigation in theme switcher with <select>
0da1183 
Follow-up of FreshRSS#8167
@Inverle Inverle mentioned this pull request Nov 5, 2025
Merged
@Inverle
Copy link
Member Author

Inverle commented Nov 5, 2025

#8190

@Inverle Inverle closed this Nov 6, 2025
Alkarex pushed a commit that referenced this pull request Nov 6, 2025
@Inverle
Replace arrow navigation in theme switcher with <select> (#8190)
8cb2158 
Follow-up (or replacement) of #8167

![gif](https://github.com/user-attachments/assets/96cdeaff-36bc-4d0b-9b75-97bd7ff78ba1)
@Inverle Inverle deleted the theme-switcher-keyboard-nav branch November 7, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Frenzie Frenzie Frenzie left review comments

@Alkarex Alkarex Alkarex left review comments

Assignees

No one assigned

Labels

Shortcuts ⌨ UI 🎨 User Interfaces

Projects

None yet

Milestone

1.28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Inverle @Alkarex @Frenzie