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

Add support for ListItem on visionOS #2000

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

joannaquu
Copy link
Contributor

@joannaquu joannaquu commented Apr 22, 2024

Platforms Impacted

  • iOS
  • visionOS
  • macOS

Description of changes

Adding support for ListItem on visionOS.

Currently, ListItem internally is a View, which means on visionOS, there is no hover/gaze effect. When there are no tap gestures added to the view, this is ok. However, when the ListItem is tappable, this presents a usability issue (users can't tell where they are and may not be aware the view is tappable). To fix this, update ListItem to be a Button when necessary.

List of changes

  • New parameter: action is an optional closure that executes when the ListItem is tapped.
  • ListItemButtonStyle: a custom SwiftUI button style that is applied when ListItem is a Button
    • The default cornerRadius makes the button a capsule, update it to match TableViewCell
    • Use cellBackgroundSelectedColor as the background color when selected
    • The resting color is already set on the ListItem itself
  • Setting a buttonStyle on the ListItem will trickle down to the detail button. Explicitly set the detail button's style. On iOS, it should be .plain and on visionOS it should be .bordered
    • When ListItem was first implemented, it was done so to mimic TableViewCell as closely as possible. This meant a larger padded detail button. To achieve this, the accessory view's padding was applied to the image and an extra background was added to address VoiceOver. Both of these changes do not work on visionOS (bloated button and darker background).
      • To fix the padding, apply it to the accessory view/innerContent instead. Changes made:
        • Move the padding values around so paddingLeading/paddingTrailing/paddingVertical are always be on innerContent. No matter what's inside the list item, these padding values always apply (this allows us to remove all the conditional code depending on if there's a trailingContentView/accessory view.
        • horizontalSpacing should then apply to the leading edge of the trailingContentView and accessory view. Since it's on the leading edge, we don't run into the issue of double padding.
      • Remove the VoiceOver change, this is actually a bug in TableViewCell, not ListItem (see screenshot of UITableViewCell)
  • To preserve existing behavior, when action is nil, ListItem should still be a View
  • Update ListItemDemoController_SwiftUI with an option to add/remove an action
  • Add option to display ListItem outside List

Binary change

Total increase: 56,264 bytes
Total decrease: -1,568 bytes

File Before After Delta
Total 31,359,184 bytes 31,413,880 bytes 🛑 54,696 bytes
Full breakdown
File Before After Delta
ListItem.o 268,680 bytes 315,992 bytes ⚠️ 47,312 bytes
__.SYMDEF 4,830,512 bytes 4,839,192 bytes ⚠️ 8,680 bytes
FocusRingView.o 837,952 bytes 838,224 bytes ⚠️ 272 bytes
SwiftUI+ViewModifiers.o 128,464 bytes 126,896 bytes 🎉 -1,568 bytes

Verification

Visual Verification
Before After iOS After visionOS
none before none after none
detail before detail after detail

Standalone ListItem

iOS visionOS
standalone ipad standalone vision

VoiceOver change

UITableViewCell Before After
vo ios before ios after

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)
Microsoft Reviewers: Open in CodeFlow

@joannaquu joannaquu requested a review from a team as a code owner April 22, 2024 22:21
@alexanderboswell
Copy link
Contributor

alexanderboswell commented Apr 22, 2024

What happens if the ListItem is part of a navigationLink? I believe that is going to have it as a button already. Is that going to prevent that from working?

Are we just assuming that the implementor makes the right decision? I guess I am a bit worried about the implications as there is a lot of bad implementations of MSFTTableViewCell because of how much it allows.

Should we be adding in something that helps to identify what the action should be so we can handle it correctly and have it not be a button when used in a navigation link? For example we have toggle, push on navigation stack (navigation link), or just a simple action.

@alexanderboswell
Copy link
Contributor

Can you add a screenshot of what it looks like in VoiceOver when the whole listItem is selected?

@joannaquu
Copy link
Contributor Author

joannaquu commented Apr 30, 2024

Can you add a screenshot of what it looks like in VoiceOver when the whole listItem is selected?

yup! they all look good except when the listitem is standalone and in a vstack (we run into the same issue where voiceover does not wrap the padded view). looking into if we can shift our padding values around updated

List Standalone
View list view vstack view
Button list button vstack button

@joannaquu joannaquu merged commit b663754 into microsoft:main Apr 30, 2024
7 checks passed
@joannaquu joannaquu deleted the joannaqu/list-item-vision branch April 30, 2024 22:34
@joannaquu joannaquu mentioned this pull request May 23, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants