Fix incorrect sort order of NavigationStack #169
Conversation
There was a problem hiding this comment.
I'm on an Apple Silicon Mac so these snapshots might not be generated as needed.
| @@ -0,0 +1,30 @@ | |||
| // | |||
There was a problem hiding this comment.
I know y'all just got rid of this, but I brought it back because the navigation buttons were not being rendered when using only the SwiftUI views.
There was a problem hiding this comment.
Hmm, they weren't being rendered at all? The only thing that should be different between these is how the view is sized (setting the frame vs. bounds). Might affect the safe area insets, but everything should still be rendered.
There was a problem hiding this comment.
I'm sorry, I misspoke. The button is getting rendered but it is not included as an accessibility element
Changing this code from bounds to frame resolves the issue: https://github.com/cashapp/AccessibilitySnapshot/blob/master/Sources/AccessibilitySnapshot/SnapshotTesting/SnapshotTesting%2BSwiftUI.swift#L68
| @@ -601,9 +601,14 @@ private extension NSObject { | |||
| ) | |||
| ) | |||
| } | |||
|
|
|||
| let explicitlyOrdered = accessibilityElements.contains { | |||
| "\(type(of: $0))" != "UILayoutContainerView" | |||
There was a problem hiding this comment.
I don't love this fix but from my very small set of tests it works. Next week I'm going to check against our tests at work to see if anything breaks with this change.
There was a problem hiding this comment.
I've checked our tests at work and see no regression from this change.
| @@ -138,6 +138,32 @@ final class SnapshotTestingTests: XCTestCase { | |||
| assertSnapshot(matching: viewController, as: .imageWithSmartInvert, named: nameForDevice()) | |||
| } | |||
|
|
|||
| @available(iOS 16.0, *) | |||
There was a problem hiding this comment.
If I remember correctly this doesn't do quite enough to make the tests pass and I'll need to add an XCTSkip.
|
@NickEntin any feedback on this pull request? I think it is ready to come out of draft status but I want to make sure y'all are good with it. |
| let explicitOrderingExceptionList = [ | ||
| "UILayoutContainerView", | ||
| "UpdateCoalescingCollectionView", | ||
| ] |
There was a problem hiding this comment.
While this fixes the specific bug we have a repro case for, I suspect it's hiding a larger issue around how we're matching VoiceOver's treatment of accessibility containers. If we recreate the same accessibility hierarchy of a UILayoutContainerView or UpdateCoalescingCollectionView do we get the same incorrect results? Looks like they might be using containers of containers.
There was a problem hiding this comment.
Digging into this to try to understand it better.
There was a problem hiding this comment.
Update with what I've found so far:
Both the NavigationView and NavigationStack have a UILayoutContainerView in their view hierarchy -- in general both view hierarchies look very similar. The key difference between the two is that the UILayoutContainerView in the NavigationView has shouldGroupAccessibilityChildren set to true while in the NavigationStack it is set to false.
This is key to the problem because the shouldGroupAccessiblityChildren property is what is used to determine whether a group of accessibility elements has explicitlyOrdered set to false: https://github.com/cashapp/AccessibilitySnapshot/blob/master/Sources/AccessibilitySnapshot/Core/Swift/Classes/AccessibilityHierarchyParser.swift#L631
To me this points to what you mentioned: a larger issue around how the library is matching VoiceOver's treatment of accessibility containers. In specific I think we need improvements to how explicitlyOrdered is determined, or maybe how it is used to create the sorted elements, but I don't have suggestions at this time.
| let explicitlyOrdered = accessibilityElements.contains { | ||
| explicitOrderingExceptionList.contains("\(type(of: $0))") == false | ||
| } | ||
| let shouldBeExplicitlyOrdered = accessibilityHierarchyOfElements.contains { node in |
There was a problem hiding this comment.
Here is another approach to determining whether a group should be explicitly ordered. I need to think it through a bit more but I think this matches how VoiceOver works since the ordering is determined by the order of the accessibilityElements array.
There was a problem hiding this comment.
@NickEntin could I get your thoughts on this approach?
DavidBrunow
force-pushed
the
bugfix/navigationStackSortOrder
branch
from
010671b
to
4321111
Compare
|
Would love to see this land at some point. Seems it may fix #129 as well? |
|
Just tested and this does indeed fix #129 |
I need to pick this back up again, will try to do so soon. |
Fixes #168.