pointfreeco / swift-navigation

Bringing simple and powerful navigation tools to all Swift platforms, inspired by SwiftUI.
http://pointfree.co
MIT License
1.96k stars 123 forks source link

Add accessibilityLabel property to ButtonState #171

Closed acosmicflamingo closed 3 months ago

acosmicflamingo commented 3 months ago

Attempting to address the following discussion: https://github.com/pointfreeco/swiftui-navigation/discussions/164.

Interfacing with models like AlertState can greatly benefit from adding an accessibilityLabel property in the ButtonState model for VoiceOver users.

stephencelis commented 3 months ago

@acosmicflamingo Thanks for taking the time to PR! TextState actually already supports accessibility labels, but I assume that you're looking to expose this info to UIKit? Can you sketch out some code you'd like to work in UIKit and we can consider how to attack it?

acosmicflamingo commented 3 months ago

@stephencelis yeah, it just seems like this information gets lost in UIKit. Although TextState already supports accessibility labels, doing something like this does not the effect you'd think:

extension AlertState where Action == Never {
  @MainActor
  fileprivate static let emailAddressInvalid: Self = {
    let data = AlertState(
      title: TextState(
        WishKitExtensionsStrings.RequestAFeatureViewController
        .Alert.EmailAddressInvalid.title
      ),
      buttons: [
        .default(
          TextState("Hello, world!")
            .accessibilityLabel("LOL")
        )
      ]
    )
    return data
  }()

If I were to do something like let label = SwiftUI.Text("Hello, world").accessibilityLabel("LOL"), I can't just extract it with let accessibilityLabel = label.accessibilityLabel, which of course is never needed because SwiftUI lazily loads them; SwiftUI just "knows" what to do. Plus, accessibilityLabel behavior is treated as a modifier, so I don't even know what would happen if you were to do something like TextState("Hello, world!").accessibilityLabel("LOL1").accessibilityLabel("LOL2")

With respect to TextState supporting accessibility labels, this is exactly where the information is lost:

extension UIAlertAction {
  convenience init<Action>(
    _ button: ButtonState<Action>,
    action handler: @escaping (_ action: Action?) -> Void
  ) {
    self.init(
      title: String(state: button.label),   // where accessibilityLabel does not make its way to UIAlertAction
      style: button.role.map(UIAlertAction.Style.init) ?? .default
    ) { _ in
      button.withAction(handler)
    }
  }
}
acosmicflamingo commented 3 months ago

With that being said, keep in mind that there may be limitations that come from Apple themselves. For example, our own @ZevEisenberg created this years ago, and it doesn't look like it's been addressed: http://www.openradar.me/27286870.

Apple likes to preach a lot about accessibility, but even they have a bunch of blind-spots in their codebase and apps. I had to do this hack to support accessibility labels in my UISegmentedControl instances:

// Based on this answer: 
// https://stackoverflow.com/questions/8833967/how-do-i-set-the-accessibility-label-for-a-particular-segment-of-a-uisegmentedco/59898440#59898440

import Foundation
import UIKit

extension UISegmentedControl {
  public func insertSegmentA11y(action: UIAction, at segment: Int, animated: Bool) {
    insertSegment(action: action, at: segment, animated: animated)
    // Horrible that Apple's UISegmentedControl does not seem to be
    // capable of setting its segments' accessibility labels or
    // its accessibilityUserInputLabels if it's been set in UIAction
    // instance
    if let segmentView = accessibilityElement(at: segment) as? UIView {
      segmentView.accessibilityLabel = action.accessibilityLabel
      segmentView.accessibilityUserInputLabels = action.accessibilityUserInputLabels
    }
  }
}
stephencelis commented 3 months ago
    self.init(
      title: String(state: button.label),   // where accessibilityLabel does not make its way to UIAlertAction
      style: button.role.map(UIAlertAction.Style.init) ?? .default
    ) { _ in
      button.withAction(handler)
    }

How does UIAlertController accessibility work in general? Would you assign the string to UIAlertAction.accessibilityLabel, or does that not work?

stephencelis commented 3 months ago

@acosmicflamingo Just wondering if these changes would be enough to do what you want, or if there's more:

diff --git a/Sources/SwiftUINavigationCore/TextState.swift b/Sources/SwiftUINavigationCore/TextState.swift
index 09ac2ee98..c90619832 100644
--- a/Sources/SwiftUINavigationCore/TextState.swift
+++ b/Sources/SwiftUINavigationCore/TextState.swift
@@ -380,6 +380,15 @@
       return `self`
     }

+    public var accessibilityLabel: TextState? {
+      for modifier in self.modifiers.reversed() {
+        if case let .accessibilityLabel(accessibilityLabel) = modifier {
+          return accessibilityLabel
+        }
+      }
+      return nil
+    }
+
     public func accessibilityTextContentType(_ type: AccessibilityTextContentType) -> Self {
       var `self` = self
       `self`.modifiers.append(.accessibilityTextContentType(type))
diff --git a/Sources/UIKitNavigation/Navigation/UIAlertController.swift b/Sources/UIKitNavigation/Navigation/UIAlertController.swift
index c70a70aec..dd92dec5a 100644
--- a/Sources/UIKitNavigation/Navigation/UIAlertController.swift
+++ b/Sources/UIKitNavigation/Navigation/UIAlertController.swift
@@ -79,6 +79,9 @@
       ) { _ in
         button.withAction(handler)
       }
+      if #available(iOS 15, *) {
+        self.accessibilityLabel = button.label.accessibilityLabel.map { String(state: $0) }
+      }
     }
   }
 #endif
acosmicflamingo commented 3 months ago

@stephencelis that is correct. If you set a UIAlertAction.accessibilityLabel property and then pass it to UIAlertController via addAction, that is enough for it to just work.

As to the diffs you've provided, that looks perfect and exactly what I need. To be completely sure, I will add those changes and run some tests.

acosmicflamingo commented 3 months ago

@stephencelis works like a charm! With that being said, took a bit for me to get it to work because there is duplicate logic here that should be removed in swift-composable-architecture repo: https://github.com/pointfreeco/swift-composable-architecture/blob/35556a66f28b579625ecf50da2d4bc0d6ad3fa4e/Sources/ComposableArchitecture/UIKit/AlertStateUIKit.swift#L219

stephencelis commented 3 months ago

@acosmicflamingo Wanna update this PR and we can get it merged? We should also remove any duplicate logic in the corresponding TCA branch here: https://github.com/pointfreeco/swift-composable-architecture/pull/3180

acosmicflamingo commented 3 months ago

@stephencelis sure! I'm trying to figure out why deleting the duplicate code breaks the logic in AlertStateUIKit. Once I do, I'll update this PR and create another one for swift-composable-architecture. Thanks a bunch for all your help!

acosmicflamingo commented 3 months ago

@stephencelis want me to squash it all in one commit?

acosmicflamingo commented 3 months ago

Thanks for your help Stephen! Couldn't have done it without your suggestions :D