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 AccessibilityDependency (UIAccessibility abstraction) #26

Merged

Conversation

acosmicflamingo
Copy link
Contributor

@acosmicflamingo acosmicflamingo commented Jan 20, 2023

  • Created dependency module for UIAccessibility framework

  • Supported features/modes

    • VoiceOver
    • Mono audio
    • Closed captioning
    • Invert colors
    • Guided access
    • Bold text
    • Button shapes
    • Grayscale
    • Reduce transparency
    • Reduce motion
    • Reduce motion: prefer cross-fade transitions
    • Video autoplay
    • Darker system colors (increase contrast)
    • Switch control
    • Speak selection
    • Speak screen
    • Shake to undo
    • Assistive touch
    • On/Off switch label
    • Differentiate without color
    • Hearing device paired ear
  • Supported UIAccessibility functions

    • UIAccessibility.convertToScreenCoordinates
    • UIAccessibility.focusedElement
    • UIAccessibility.post
    • UIAccessibility.requestGuidedAccessSession
  • Accessibility Dependency ddocumentation mirrors UIKit's UIAccessibility documentation

  * Created dependency module for UIAccessibility framework

  * Supported features/modes
    - VoiceOver
    - Mono audio
    - Closed captioning
    - Invert colors
    - Guided access
    - Bold text
    - Button shapes
    - Grayscale
    - Reduce transparency
    - Reduce motion
    - Reduce motion: prefer cross-fade transitions
    - Video autoplay
    - Darker system colors (increase contrast)
    - Switch control
    - Speak selection
    - Speak screen
    - Shake to undo
    - Assistive touch
    - On/Off switch label
    - Differentiate without color
    - Hearing device paired ear

  * Supported UIAccessibility functions
    - UIAccessibility.convertToScreenCoordinates
    - UIAccessibility.focusedElement
    - UIAccessibility.post
    - UIAccessibility.requestGuidedAccessSession

  * Accessibility Dependency ddocumentation mirrors UIKit's UIAccessibility
    documentation
@acosmicflamingo
Copy link
Contributor Author

acosmicflamingo commented Jan 20, 2023

@tgrapperon I'd like to think that I can just slap on some pragmas for iOS and tvOS since I didn't see any UIAccessibility functionality that is platform-specific, but I imagine there's a few functions or variables that are only available in one platform. When you were creating the UIApplication or UIDevice abstractions, what was your process for finding the functionality that was available in one platform but not the other?

@tgrapperon
Copy link
Owner

tgrapperon commented Jan 20, 2023

Hey @acosmicflamingo! Wow, thanks, very impressive, and something that could definitely be useful. Congrats for getting the ConfigurableProxy and property wrapper stuff that I had not properly documented yet!

I didn't had the time to go through properly, and I'm heading to bed right now, but a few comments:

  • The library requires iOS 13, etc., so I decided to generally not annotate the availability if <=13, as the symbols are available anyway. The only exception is when we need to specify the availability for a platform of something that was already present for a long time on the other ones. For example, something that was available on iOS 9, but only on watchOS from v8. Nothing wrong with your implementation, but for homogeneity, we would probably remove these annotations if they're not effective.
  • I don't think there's a lot of value into turning the Notification.Name constant as dependencies. But we can implement the corresponding notifications in the _Notification dependency and get streams of these values.
  • I don't think that these values should be MainActor, so this can be simplified too.
  • We should probably expose macOS variants too, and properly install this dependency, as it is currently not built. This is a huge task, so I'll check how to make room for it for a another PR.

There are many things in the library that I didn't had the time to explain yet, so if that's OK with you, I'll take over from now and I'll update this PR with the changes I have in mind, and I'll let you know once I'm done, so you can check if it still fits what you had in mind.

But this is a great idea and a great working base, and surely something that will be very useful when it'll land. Thanks again!

@tgrapperon
Copy link
Owner

@acosmicflamingo

When you were creating the UIApplication or UIDevice abstractions, what was your process for finding the functionality that was available in one platform but not the other?

I looked at the documentation and interface files, and the unavailable ones that I missed were caught when trying to build for these platforms.

@acosmicflamingo
Copy link
Contributor Author

acosmicflamingo commented Jan 20, 2023

Hey @acosmicflamingo! Wow, thanks, very impressive, and something that could definitely be useful. Congrats for getting the ConfigurableProxy and property wrapper stuff that I had not properly documented yet!

I didn't had the time to go through properly, and I'm heading to bed right now, but a few comments:

  • The library requires iOS 13, etc., so I decided to generally not annotate the availability if <=13, as the symbols are available anyway. The only exception is when we need to specify the availability for a platform of something that was already present for a long time on the other ones. For example, something that was available on iOS 9, but only on watchOS from v8. Nothing wrong with your implementation, but for homogeneity, we would probably remove these annotations if they're not effective.
  • I don't think there's a lot of value into turning the Notification.Name constant as dependencies. But we can implement the corresponding notifications in the _Notification dependency and get streams of these values.
  • I don't think that these values should be MainActor, so this can be simplified too.
  • We should probably expose macOS variants too, and properly install this dependency, as it is currently not built. This is a huge task, so I'll check how to make room for it for a another PR.

There are many things in the library that I didn't had the time to explain yet, so if that's OK with you, I'll take over from now and I'll update this PR with the changes I have in mind, and I'll let you know once I'm done, so you can check if it still fits what you had in mind.

But this is a great idea and a great working base, and surely something that will be very useful when it'll land. Thanks again!

@tgrapperon Woohoo! Glad to hear this :) Hahah like isowords, a lot can be learned just by jumping into the deep end so ConfigurableProxy wasn't hard to grasp after looking at how you designed UIDevice & UIApplication. If anything, it felt like plagiarism since UIApplication and UIAccessibility are very similar in structure.

As for your points:

  • I copied and pasted whatever was in UIKit.UIApplication and thought it'd be better to keep the availability and delete it if requested, than to not include it, and be told that it should be included. Because this library requires iOS 13, then yes we can definitely get rid of those superfluous accessibility decorators
  • Yeah I can see that. In the same spirit of being more happy to delete functionality than to add it, I created a lot of the Notification.Name constants as dependencies although it does seem a bit silly since they are constants after all. I do see that in the current state of DependenciesAdditions, the right place for these NSNotification.Name values should be in _Notification. With that being said, my only concern is the instance where users import AccessibilityDependency but not _Notification. If Accessibility is really a UIAccessibility abstraction, then I think users would expect these notification names to be available. A great solution to this may be something like this in the AccessibilityGetters, so we don't leave those users behind (and make accessibility as easy as possible for developers to use):
  /// A Boolean value that indicates whether VoiceOver is in an enabled state.
  ///
  /// You can use this function to customize your app’s UI specifically for VoiceOver users.
  /// For example, you might want UI elements that usually disappear quickly to persist
  /// onscreen for VoiceOver users.
  ///
  /// Note that you can also listen for the
  /// ``voiceOverStatusDidChangeNotification`` notification to determine
  /// when VoiceOver starts and stops. In the spirit of utilizing the Dependencies library,
  /// the voiceOverStatusDidChangeNotification property is located within
  /// `@Dependency(\.notification) var notification`, which can be utilized after
  /// importing `_Notification` in your codebase
  @MainActor
  @available(iOS 4.0, *)
  public var isVoiceOverRunning: Bool {
    _implementation.isVoiceOverRunning
  }
  • I made all those properties MainActor as a result of not trusting the values I get reflecting the app's state if they are retrieved in a background task, but you can thank UIDevice for that ;) looking through UIAccessibility, I think you're right (and I imagine that UIAccessibility needs to be accurate if executed in a background task) so MainActor most likely seems useless here.
  • Yeah macOS does have its own variants (e.g. iOS's darkColorsEnabled == macOS's increaseContrast) so if we want to support AppKit users, then this PR is certainly incomplete

Yeah I don't have a problem with you taking over from here :) I wanted to do this myself since I imagine you have tons in your backlog already and don't want to be that person who creates a Github issue asking "WHERE IS ACCESSIBILITY DEPENDENCY??? MAKE IT NOW" when I could just write up my implementation and create a PR for it. But yeah, DependencyAdditions is your project so as the maintainer, you do what you want! If you need help with anything though, you can certainly ask and I can do what I can! Thanks @tgrapperon for all your effort in TCA, Dependencies, and open source in general :D

@tgrapperon
Copy link
Owner

Hey @acosmicflamingo!
I've reworked the PR into something more in line with the other dependencies.

  • Notifications have been relocated into the _Notification module, so they can be observed directly.
  • It is available on iOS and tvOS for now. Other platforms will be supported later.
  • Removed MainActor annotations (I think that is better to only enforce it when required upstream, which was not the case here.
  • I've also trimmed the documentation to only include the slug line, which is the convention I implicitly followed in other dependencies, as the purpose is mostly to help disambiguating the APIs.
  • I simplified and moved a little the example from the README, as I feel that Application have a better pedagogical value in this position.

If this is fine with you, we can merge.

@acosmicflamingo
Copy link
Contributor Author

@tgrapperon WHOA! You really took it to the next level :D I am all for the changes you had made, and even find it funny that the snippet I wrote in the README in hindsight is pretty useless compared to your more practical one. I also made Accessibility first because I thought the intent was to order it alphabetically but I agree with your decision to move it after Application.

Go ahead and merge this as-is :) thanks for your really constructive feedback and your time to get this to tip-top shape 🥇 Woohoo!!!

@tgrapperon
Copy link
Owner

Thank you for your PR, it will help many folks for sure!
I'll cut a release in a few days, in any case something comes up in the meantime!

@tgrapperon tgrapperon merged commit 023195e into tgrapperon:main Jan 21, 2023
@acosmicflamingo acosmicflamingo deleted the feature/accessibility-dependency branch January 21, 2023 03:52
@acosmicflamingo
Copy link
Contributor Author

Anytime! Sounds good 😃

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.

None yet

2 participants