-
Notifications
You must be signed in to change notification settings - Fork 33
[iOS] Add Support for List Item Focus Area #602
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
[iOS] Add Support for List Item Focus Area #602
Conversation
8c87d32 to
c9f8575
Compare
ListableUI/Sources/Behavior.swift
Outdated
| } | ||
| } | ||
|
|
||
| public static func == (lhs: FocusConfiguration, rhs: FocusConfiguration) -> Bool { |
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.
Can this implementation be automatically synthesized?
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.
Yep, I removed this!
| switch self.behavior.focus { | ||
| case nil, .some(.none): |
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.
Does this .some(.none) mean that there is a double optional thing going on here?
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.
Updated so the .some(.none) case is no longer relevant.
a0fc0a2 to
dfa161f
Compare
johnnewman-square
left a comment
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.
This is looking great, including the demo. Thanks for making these updates. I left just a few notes that I'd like to iron out.
ListableUI/Sources/Behavior.swift
Outdated
|
|
||
| /// Configuration for keyboard focus behavior in the list view. | ||
| /// | ||
| /// - `nil`: No focus support - keyboard navigation is disabled | ||
| /// - `.none`: Explicitly disabled focus support | ||
| /// - `.allowsFocus`: Basic focus support with keyboard navigation, but selection doesn't follow focus | ||
| /// - `.selectionFollowsFocus`: Focus support where selection automatically follows focus changes | ||
| /// | ||
| /// When focus is enabled, items that support selection can receive focus for keyboard navigation. | ||
| /// The focus ring will be applied to focused items automatically. | ||
| public var focus: FocusConfiguration? |
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.
If nil and none are identical in behavior, it could be a nice change to consolidate them into none and make focus a non-optional variable. I think that would simplify the overall mental model of these two variants and could remove the need for the nil guards in ListView.Delegate.
This is similar to how PageScrollingBehavior was built, where the Behavior's pageScrollingBehavior init parameter defaults to none instead of nil. Curious on your thoughts!
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.
Great idea
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.
Great suggestion! I went ahead and made this change. The focus property is now non-optional and defaults to .none. Also removed all the optional guards in the delegate methods and simplified switch statements.
| Item( | ||
| DemoItem(text: "Toggle Item (Focus + Toggle)"), | ||
| selectionStyle: .toggles(), | ||
| onSelect: { _ in | ||
| print("Toggled Toggle Item") | ||
| } | ||
| ) |
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.
I noticed that after toggling this item, the focus selection is reset the next time I use the arrow keys for navigation. If we wanted the focused item to be preserved, is this behavior that needs to be configured by the client, or could that be something internal to our UICollectionView implementation within Listable?
(I did not have full keyboard access enabled during this flow.)
ScreenRecording_10-27-2025.10-16-24_1.mov
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.
This is interesting! It's hard to tell from the documentation but I wonder if this is expected UIKit behavior. When the toggle item deselects, UICollectionView loses the current focus anchor and re-evaluates from the beginning of the focus group. I think one potential solution is leveraging focusgroupidentifier and focusgrouppriority and updating a cell's focusgrouppriority if it's deselected.
Curious if you're ok with keeping the current changes for now and we could add focus preservation logic for toggles in a follow-up? I can create a ticket to track it.
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.
Got it! Increasing the priority when deselected sounds like a good idea to try out. Interestingly, when using full keyboard access, the toggled item stays focused between toggle states (video below). So this issue seems to be isolated to standard focus and will impact a smaller subset of users. I'm on board with merging if you'd like to move toggle focus preservation to a followup ticket!
ScreenRecording_10-31-2025.14-11-25_1.mov
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.
I've made a ticket here: https://block.atlassian.net/browse/UI-9566
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.
@johnnewman-square Would you be able to merge on your end? I don't have the option to do so.
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.
Sure thing! I'll kick off one more set of test actions and merge.
| list.add { | ||
| Section("instructions") { | ||
| Item( | ||
| DemoItem(text: "Instructions:\n• Use Arrow keys to navigate within the list\n• Press Return/Space to select items\n• Toggle settings with navigation buttons"), |
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.
In my testing flows, the space key will only function as return when full keyboard access is enabled. If that's right, could we add that detail to the demo text? Thanks!
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.
That's correct. I've added updated demo text.
Package.resolved
Outdated
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.
With the Tuist setup storing Package.resolved under Development/Tuist/, we should be able to remove this file.
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.
Removed
5ad8b50 to
b84698c
Compare
johnnewman-square
left a comment
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.
These changes look good. Thank you for addressing the feedback!
soroushsq
left a comment
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.
Just a quick code golfy thing
| if view.behavior.focus.allowsFocus { | ||
| cell.focusEffect = view.behavior.focus.showFocusRing ? UIFocusHaloEffect() : nil | ||
| } else { | ||
| cell.focusEffect = nil | ||
| } |
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.
| if view.behavior.focus.allowsFocus { | |
| cell.focusEffect = view.behavior.focus.showFocusRing ? UIFocusHaloEffect() : nil | |
| } else { | |
| cell.focusEffect = nil | |
| } | |
| cell.focusEffect = view.behavior.focus.allowsFocus && view.behavior.focus.showFocusRing ? UIFocusHaloEffect() : nil |
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.
@soroushsq Updated!
b84698c to
612e1c8
Compare
Describe your changes here. Please include screenshots if they're visual!
This PR adds support for keyboard focus navigation on iOS, enabling users to navigate list items using keyboard input (Tab, Arrow keys, Return/Space) for improved accessibility and external keyboard support.
Key Changes
1. New Focus Configuration API
Added
Behavior.FocusConfigurationenum with three modes:.none- No focus support (default).allowsFocus- Basic keyboard navigation with always-visible focus ring.selectionFollowsFocus(showFocusRing: Bool)- Navigation where selection follows focus3. Demo Integration
Added
KeyboardNavigationDemoViewControllershowcasing:Usage Example
Demo Videos
Demo 1: Selection Follows Focus:
Demo 2: Allows Focus (pressing enter triggers a selection)
Screen.Recording.2025-10-20.at.11.01.09.AM.mov
Screen.Recording.2025-10-20.at.11.04.38.AM.mov
Checklist
Please do the following before merging:
Mainsection.