Skip to content

Updated Sort Order#244

Merged
RoyalPineapple merged 3 commits intomasterfrom
alex/SortUpdate
Jun 24, 2025
Merged

Updated Sort Order#244
RoyalPineapple merged 3 commits intomasterfrom
alex/SortUpdate

Conversation

@RoyalPineapple
Copy link
Copy Markdown
Collaborator

Based on discussions with @meherkasam this PR updates the sort order to take the userinterfaceidiom into account.

@RoyalPineapple RoyalPineapple requested a review from meherkasam May 27, 2025 15:37
@RoyalPineapple RoyalPineapple marked this pull request as ready for review May 27, 2025 15:40
Co-authored-by: Meher Kasam <72885155+meherkasam-square@users.noreply.github.com>
in root: UIView,
userInterfaceLayoutDirectionProvider: UserInterfaceLayoutDirectionProviding = UIApplication.shared
userInterfaceLayoutDirectionProvider: UserInterfaceLayoutDirectionProviding = UIApplication.shared,
userInterfaceIdiomProvider: UserInterfaceIdiomProviding = UIDevice.current
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Noting that in the Market snapshot tests we will want to inject a different UserInterfaceIdiomProviding, since they're kinda magic: They can run on any 3x iOS device and do swizzling and such to simulate all different device sizes, safe areas, etc, that we need.

let minimumVerticalSeparation = 8.0
// Derived via experimentation, these magic numbers are the cutoff for VoiceOver to consider
// an element to be vertically "above" other views.
let minimumVerticalSeparation = userInterfaceIdiom == .phone ? 8.0 : 13.0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not for this PR, but I am wondering if Voiceover is also a bit more clever when seeing a hierarchy like this:

┌──────────────────────────────────┐
│                                  │
│             ┌───────┐            │
│  ┌───────┐  │2      │ ┌───────┐  │
│  │1      │  │       │ │3      │  │
│  └───────┘  │       │ │       │  │
│             └───────┘ │       │  │
│                       └───────┘  │
│                                  │
│                                  │
│  ┌───────┐                       │
│  │4      │  ┌───────┐            │
│  └───────┘  │5      │ ┌───────┐  │
│             │       │ │6      │  │
│             │       │ └───────┘  │
│             └───────┘            │
│                                  │
└──────────────────────────────────┘

Eg does it try to also group content into "rows" (eg, 1, 2, 3 would be in one group, 4, 5, 6) in another, and then orders the items in the groups LTR/RTL based on layout direction, rather than purely by vertical separation? That would make a bit more sense to me.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I thought I responded to this comment, but maybe not? I set up a test project and tested out some scenarios similar to the one you described. In all cases, it seems like VoiceOver is simply doing it based on the leading-to-trailing, top to bottom (within a tolerance) regardless of the layout of the elements.

@yonaskolb
Copy link
Copy Markdown

Will this fix #129 and #168 like #169 does?

@RoyalPineapple
Copy link
Copy Markdown
Collaborator Author

Will this fix #129 and #168 like #169 does?

Probably not unfortunately, this is an unrelated fix.

@RoyalPineapple RoyalPineapple merged commit 29cc8f6 into master Jun 24, 2025
6 checks passed
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.

5 participants