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

reorder control now proxies accessibility into a seperate element #533

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

RoyalPineapple
Copy link
Collaborator

@RoyalPineapple RoyalPineapple commented Mar 5, 2024

This allows the gesture recognizer to be applied to a view without overriding its accessibility.
Useful in situations where you'd like the gesture to apply to the entire cell for example.

Checklist

Please do the following before merging:

  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@RoyalPineapple RoyalPineapple force-pushed the alex/reodera11y branch 2 times, most recently from 4f98207 to 9946411 Compare March 5, 2024 21:37
@RoyalPineapple RoyalPineapple force-pushed the alex/reodera11y branch 2 times, most recently from 76faf8e to 50ae673 Compare March 13, 2024 11:20
@RoyalPineapple RoyalPineapple marked this pull request as ready for review March 19, 2024 16:13
@@ -5,6 +5,7 @@
// Created by Kyle Van Essen on 11/14/19.
//

import Accessibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

huh, TIL

BlueprintUILists/Sources/ListReorderGesture.swift Outdated Show resolved Hide resolved
BlueprintUILists/Sources/ListReorderGesture.swift Outdated Show resolved Hide resolved
BlueprintUILists/Sources/ListReorderGesture.swift Outdated Show resolved Hide resolved
let recognizer : ItemReordering.GestureRecognizer
private lazy var proxyElement = UIAccessibilityElement(accessibilityContainer: self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, TIL, I had no idea you could do this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, although seems obvious in retrospect.

BlueprintUILists/Sources/ListReorderGesture.swift Outdated Show resolved Hide resolved
BlueprintUILists/Sources/ListReorderGesture.swift Outdated Show resolved Hide resolved
@RoyalPineapple RoyalPineapple requested a review from kyleve March 19, 2024 21:49
@@ -66,6 +67,7 @@ public struct ListReorderGesture : Element
isEnabled : Bool = true,
actions : ReorderingActions,
begins: Begins = .onTap,
reorderItemAccessibilityLabel: String? = nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be helpful to add docs for the parameters and indicate to the caller what a recommended localised reordering label might be.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I noticed below that we're using a string format and plugging in the item name. What do you think about making this an enum instead where the caller can specify whether it's just the item name and they use our localised terminology or whether they themselves want to provide the terminology. For example, in case someone might want to call it Rearrange Foo instead of Reorder Foo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to push this entirely down to the caller, i think you're right that it needs more customizability but i think that too much magic is overcomplicated here.

}

@objc private func updateGesturePressDuration() {
self.recognizer.minimumPressDuration = UIAccessibility.isVoiceOverRunning ? 0.0 : self.minimumPressDuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for this PR, but wondering how the behaviour would be with full keyboard access, Switch Control, and Voice Control.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested this out using full keyboard access. It kind of works, but I observed that Listable reordering is overall broken for full keyboard access. Then again, even the iOS home screen felt broken for full keyboard access, so there's no prior art to even copy from.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wouldnt expect the gesture recognizer itself to work well with keyboard access. do the accessibility actions work with full keyboard access?

Copy link
Contributor

@meherkasam-square meherkasam-square left a comment

Choose a reason for hiding this comment

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

Can you please add an entry to the changelog?

@RoyalPineapple RoyalPineapple enabled auto-merge (squash) March 28, 2024 19:08
@RoyalPineapple RoyalPineapple merged commit c3f8240 into main Mar 28, 2024
4 checks passed
@RoyalPineapple RoyalPineapple deleted the alex/reodera11y branch March 28, 2024 21:33
kyleve added a commit that referenced this pull request Aug 1, 2024
* origin/main:
  Prepare 14.3.0 (#540)
  Custom Update Animations (#539)
  Update version to 14.2.0
  Prepare 14.2.0 release, which contains a Blueprint update
  Bumping versions to 14.1.0 (#535)
  reorder control now proxies accessibility into a seperate element (#533)
  Prepare 14.0.3
  Fix a crash that could occur during cell reuse if a list contained different types of headers. The wrong ObjectIdentifier was being compared and stored.
  Get a repro for the reordering crash reported in #market-ios
  chore: Generated documentation now uses a static copyright notice to avoid noisy diffs (#530)
  Bumping versions to 15.0.2
  fix: Fix tap gesture swallowing touches in swipe actions view
  Bump to 14.0.1
  Fix SPM Blueprint dependency
  Release 14.0.0, update BlueprintUI to 3.0.0 (#525)
  chore: iOS 15 deployment target bump [UI-5187] (#524)
  chore: Bump CI to Xcode 15.1. Bump gems. [UI-5186] (#523)
  fix: don't cancel touches in view for tap gesture recognizer
  Revert weak change
  Revert "Revert "Supplementary Tracking Fixes (#433)""
kyleve added a commit that referenced this pull request Aug 6, 2024
…rovements

* origin/main: (33 commits)
  Prepare 14.3.0 (#540)
  Custom Update Animations (#539)
  Update version to 14.2.0
  Prepare 14.2.0 release, which contains a Blueprint update
  Bumping versions to 14.1.0 (#535)
  reorder control now proxies accessibility into a seperate element (#533)
  Prepare 14.0.3
  Fix a crash that could occur during cell reuse if a list contained different types of headers. The wrong ObjectIdentifier was being compared and stored.
  Get a repro for the reordering crash reported in #market-ios
  chore: Generated documentation now uses a static copyright notice to avoid noisy diffs (#530)
  Bumping versions to 15.0.2
  fix: Fix tap gesture swallowing touches in swipe actions view
  Bump to 14.0.1
  Fix SPM Blueprint dependency
  Release 14.0.0, update BlueprintUI to 3.0.0 (#525)
  chore: iOS 15 deployment target bump [UI-5187] (#524)
  chore: Bump CI to Xcode 15.1. Bump gems. [UI-5186] (#523)
  fix: don't cancel touches in view for tap gesture recognizer
  fix: don't cancel touches in view for tap gesture recognizer
  Revert weak change
  ...
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.

3 participants