Fix incorrect sort order of NavigationStack #169
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // | ||
| // Copyright 2021 Square Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
| #if swift(>=5.1) && canImport(SwiftUI) | ||
|
|
||
| import SwiftUI | ||
| import UIKit | ||
|
|
||
| extension View { | ||
|
|
||
| // This method should be removed once we support snapshotting native SwiftUI views. | ||
| // See cashapp/AccessibilitySnapshot#37. | ||
| func embedInHostingController() -> UIViewController { | ||
| return UIHostingController(rootView: self) | ||
| } | ||
| } | ||
|
|
||
| #endif | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // | ||
| // Copyright 2023 Square Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
|
|
||
|
|
||
| #if swift(>=5.1) && canImport(SwiftUI) | ||
|
|
||
| import SwiftUI | ||
|
|
||
| @available(iOS 15.0, *) | ||
| struct SwiftUIViewWithListWithSections: View { | ||
| @State var groupedItems: [(String, [String])] = [ | ||
| ("Section 1", ["Item 1", "Item 2", "Item 3"]), | ||
| ("Section 2", ["Item 1", "Item 2", "Item 3"]), | ||
| ("Section 3", ["Item 1", "Item 2", "Item 3"]), | ||
| ] | ||
|
|
||
| var body: some View { | ||
| List { | ||
| ForEach(groupedItems, id: \.0) { groupedItem in | ||
| Section(groupedItem.0) { | ||
| ForEach(groupedItem.1, id: \.self) { item in | ||
| Text(item) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @available(iOS 15.0, *) | ||
| struct SwiftUIViewWithListWithSections_Previews: PreviewProvider { | ||
| static var previews: some View { | ||
| SwiftUIViewWithListWithSections() | ||
| .previewLayout(.sizeThatFits) | ||
| } | ||
| } | ||
|
|
||
| #endif |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // | ||
| // Copyright 2023 Square Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
|
|
||
|
|
||
| #if swift(>=5.1) && canImport(SwiftUI) | ||
|
|
||
| import SwiftUI | ||
|
|
||
| @available(iOS 16.0, *) | ||
| struct SwiftUIViewWithNavigationStack: View { | ||
| var body: some View { | ||
| NavigationStack { | ||
| Text("Text inside a NavigationStack") | ||
| .navigationTitle("Navigation Stack") | ||
| .toolbar { | ||
| ToolbarItem { | ||
| Button { | ||
| // no-op | ||
| } label: { | ||
| Text("Add") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @available(iOS 16.0, *) | ||
| struct SwiftUIViewWithNavigationStack_Previews: PreviewProvider { | ||
| static var previews: some View { | ||
| SwiftUIViewWithNavigationStack() | ||
| .previewLayout(.sizeThatFits) | ||
| } | ||
| } | ||
|
|
||
| #endif |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // | ||
| // Copyright 2023 Square Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
|
|
||
|
|
||
| #if swift(>=5.1) && canImport(SwiftUI) | ||
|
|
||
| import SwiftUI | ||
|
|
||
| @available(iOS 14.0, *) | ||
| struct SwiftUIViewWithNavigationView: View { | ||
| var body: some View { | ||
| NavigationView { | ||
| Text("Text inside a NavigationView") | ||
| .navigationTitle("Navigation View") | ||
| .toolbar { | ||
| ToolbarItem { | ||
| Button { | ||
| // no-op | ||
| } label: { | ||
| Text("Add") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @available(iOS 16.0, *) | ||
| struct SwiftUIViewWithNavigationView_Previews: PreviewProvider { | ||
| static var previews: some View { | ||
| SwiftUIViewWithNavigationView() | ||
| .previewLayout(.sizeThatFits) | ||
| } | ||
| } | ||
|
|
||
| #endif |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -604,9 +604,28 @@ private extension NSObject { | |
| ) | ||
| ) | ||
| } | ||
|
|
||
| let explicitOrderingExceptionList = [ | ||
| "UILayoutContainerView", | ||
| "UpdateCoalescingCollectionView", | ||
| ] | ||
|
Comment on lines
+608
to
+611
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 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 This is key to the problem because the 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 |
||
|
|
||
| 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 There was a problem hiding this comment. @NickEntin could I get your thoughts on this approach? |
||
| switch node { | ||
| case .element: | ||
| // Only set explicitly ordered if an element node is a direct descendent of this group. | ||
| return true | ||
| case .group: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| recursiveAccessibilityHierarchy.append(.group( | ||
| accessibilityHierarchyOfElements, | ||
| explicitlyOrdered: shouldBeExplicitlyOrdered, | ||
| frameOverrideProvider: (overridesElementFrame(with: contextProvider) ? self : nil) | ||
| )) | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
framevs.bounds). Might affect the safe area insets, but everything should still be rendered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I misspoke. The button is getting rendered but it is not included as an accessibility element
Changing this code from
boundstoframeresolves the issue: https://github.com/cashapp/AccessibilitySnapshot/blob/master/Sources/AccessibilitySnapshot/SnapshotTesting/SnapshotTesting%2BSwiftUI.swift#L68