-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix for PR #174374 - Fix - TalkBack does not announce list information #177622
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
Fix for PR #174374 - Fix - TalkBack does not announce list information #177622
Conversation
…action was present - Limiting the CollectionInfo to only be apply if there is more than 1 scrollChild
Signed-off-by: jwlilly <[email protected]>
Fixing logic bug with column/row index
Moving comment for setScrollable
…oes-not-announce-list-information
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.
Code Review
This pull request fixes an issue where TalkBack would not announce list information by correctly creating CollectionInfo and CollectionItemInfo for different Android API levels. The changes introduce version checks to handle deprecated methods and improve the logic for identifying lists and list items. My review focuses on improving code maintainability by refactoring duplicated code blocks and enhancing the new test cases with more specific assertions for better debuggability.
| // children. | ||
| if (shouldSetCollectionInfo(semanticsNode)) { | ||
| if (semanticsNode.hasAction(Action.SCROLL_LEFT) | ||
| || semanticsNode.hasAction(Action.SCROLL_RIGHT)) { | ||
| // This code will only run on devices with API level 32 or lower. | ||
| // The obtain method was deprecated in API 33. | ||
| if (Build.VERSION.SDK_INT < 33) { | ||
| result.setCollectionInfo( | ||
| AccessibilityNodeInfo.CollectionInfo.obtain( | ||
| 1, // row count | ||
| semanticsNode.scrollChildren, // column count | ||
| false // hierarchical | ||
| )); | ||
|
|
||
| } else { | ||
| if (shouldSetCollectionInfo(semanticsNode)) { | ||
| result.setCollectionInfo( | ||
| AccessibilityNodeInfo.CollectionInfo.obtain( | ||
| semanticsNode.scrollChildren, // rows | ||
| 0, // columns | ||
| false // hierarchical | ||
| )); | ||
| } else { | ||
| result.setClassName("android.widget.ScrollView"); | ||
| } | ||
| result.setCollectionInfo( | ||
| new AccessibilityNodeInfo.CollectionInfo( | ||
| 1, // row count | ||
| semanticsNode.scrollChildren, // column count | ||
| false // hierarchical | ||
| )); | ||
| } | ||
| } else { | ||
| // This code will only run on devices with API level 32 or lower. | ||
| // The obtain method was deprecated in API 33. | ||
| if (Build.VERSION.SDK_INT < 33) { | ||
| result.setCollectionInfo( | ||
| AccessibilityNodeInfo.CollectionInfo.obtain( | ||
| semanticsNode.scrollChildren, // row count | ||
| 1, // column count | ||
| false // hierarchical | ||
| )); | ||
| } else { | ||
| result.setCollectionInfo( | ||
| new AccessibilityNodeInfo.CollectionInfo( | ||
| semanticsNode.scrollChildren, // row count | ||
| 1, // column count | ||
| false // hierarchical | ||
| )); |
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.
The logic for creating CollectionInfo for horizontal and vertical lists is duplicated and can be simplified. You can determine the row and column counts first, and then use a single block for the version check to create the CollectionInfo object. This will make the code more concise and easier to maintain.
Also, it's better to use Build.VERSION_CODES.TIRAMISU instead of the magic number 33 for better readability.
final boolean isHorizontal =
semanticsNode.hasAction(Action.SCROLL_LEFT)
|| semanticsNode.hasAction(Action.SCROLL_RIGHT);
final int rowCount = isHorizontal ? 1 : semanticsNode.scrollChildren;
final int colCount = isHorizontal ? semanticsNode.scrollChildren : 1;
// The obtain method was deprecated in API 33.
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
result.setCollectionInfo(
AccessibilityNodeInfo.CollectionInfo.obtain(
rowCount, colCount, false /* hierarchical */));
} else {
result.setCollectionInfo(
new AccessibilityNodeInfo.CollectionInfo(rowCount, colCount, false /* hierarchical */));
}| !(parent.hasAction(Action.SCROLL_LEFT) || parent.hasAction(Action.SCROLL_RIGHT)); | ||
| int nodeIndex = scrollChildren.indexOf(semanticsNode); | ||
| if (verticalScroll) { | ||
| // This code will only run on devices with API level 32 or lower. | ||
| // The obtain method was deprecated in API 33. | ||
| if (Build.VERSION.SDK_INT < 33) { | ||
| result.setCollectionItemInfo( | ||
| AccessibilityNodeInfo.CollectionItemInfo.obtain( | ||
| nodeIndex, // row index | ||
| 1, // row span | ||
| 0, // column index | ||
| 1, // column span | ||
| semanticsNode.hasFlag(Flag.IS_HEADER) // is heading | ||
| )); | ||
| } else { | ||
| result.setCollectionItemInfo( | ||
| new AccessibilityNodeInfo.CollectionItemInfo( | ||
| nodeIndex, // row index | ||
| 1, // row span | ||
| 0, // column index | ||
| 1, // column span | ||
| semanticsNode.hasFlag(Flag.IS_HEADER) // is heading | ||
| )); | ||
| } | ||
| } else { | ||
| // This code will only run on devices with API level 32 or lower. | ||
| // The obtain method was deprecated in API 33. | ||
| if (Build.VERSION.SDK_INT < 33) { | ||
| result.setCollectionItemInfo( | ||
| AccessibilityNodeInfo.CollectionItemInfo.obtain( | ||
| 0, // row index | ||
| 1, // row span | ||
| nodeIndex, // column index | ||
| 1, // column span | ||
| semanticsNode.hasFlag(Flag.IS_HEADER) // is heading | ||
| )); | ||
| } else { | ||
| result.setCollectionItemInfo( | ||
| new AccessibilityNodeInfo.CollectionItemInfo( | ||
| 0, // row index | ||
| 1, // row span | ||
| nodeIndex, // column index | ||
| 1, // column span | ||
| semanticsNode.hasFlag(Flag.IS_HEADER) // is heading | ||
| )); |
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.
Similar to CollectionInfo, the logic for creating CollectionItemInfo is duplicated for different SDK versions and scroll directions. This can be refactored to be more concise. Determine the row and column indices first, then use a single version check to create the CollectionItemInfo.
Using Build.VERSION_CODES.TIRAMISU instead of 33 is also recommended here for readability.
final int rowIndex = verticalScroll ? nodeIndex : 0;
final int colIndex = verticalScroll ? 0 : nodeIndex;
// The obtain method was deprecated in API 33.
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
result.setCollectionItemInfo(
AccessibilityNodeInfo.CollectionItemInfo.obtain(
rowIndex,
1, // row span
colIndex,
1, // column span
semanticsNode.hasFlag(Flag.IS_HEADER) // is heading
));
} else {
result.setCollectionItemInfo(
new AccessibilityNodeInfo.CollectionItemInfo(
rowIndex,
1, // row span
colIndex,
1, // column span
semanticsNode.hasFlag(Flag.IS_HEADER) // is heading
));
}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.
This comment is an improvement in readability.
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 used API_LEVELS.API_33 instead of Build.VERSION_CODES.TIRAMISU. We are using API_LEVELS.API_33 in other places so I thought that might more sense
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.
That was the correct thing to sorry I was focused on reducing the duplication and cared less about the raw api number. Using API_LEVELS.API_33 is an improvement thank you.
| testSemanticsNode.scrollChildren = 1; | ||
| TestSemanticsUpdate testSemanticsUpdate = testSemanticsNode.toUpdate(); | ||
| testSemanticsUpdate.sendUpdateToBridge(accessibilityBridge); | ||
| AccessibilityNodeInfo nodeInfo = accessibilityBridge.createAccessibilityNodeInfo(0); | ||
| assertNull(nodeInfo.getCollectionInfo()); |
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.
Combining multiple checks into a single assertTrue can make it harder to debug test failures. It's better to use separate assertions for each condition. This provides clearer failure messages, indicating exactly which assertion failed.
assertEquals(testSemanticsNode.scrollChildren, collectionInfo.getRowCount());
assertEquals(1, collectionInfo.getColumnCount()); // 1 column for a list
assertFalse(collectionInfo.isHierarchical()); // this should currently always be false| childNode1.id = 1; | ||
| childNode1.label = "Test 1"; | ||
| TestSemanticsNode childNode2 = new TestSemanticsNode(); | ||
| childNode2.id = 2; | ||
| childNode2.label = "Test 2"; | ||
| parentTestSemanticsNode.addChild(childNode1); | ||
| parentTestSemanticsNode.addChild(childNode2); | ||
| TestSemanticsUpdate testSemanticsUpdate = parentTestSemanticsNode.toUpdate(); |
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.
For better test readability and easier debugging, it's recommended to use separate assertions for each check instead of combining them into a single assertTrue. This will provide more specific feedback if the test fails.
assertEquals(0, itemInfo.getRowIndex()); // first item in the list
assertEquals(1, itemInfo.getRowSpan());
assertEquals(0, itemInfo.getColumnIndex()); // only a single column
assertEquals(1, itemInfo.getColumnSpan());
// Note: isHeading() is deprecated, and since this test node doesn't have IS_HEADER flag,
// we expect it to not be a heading. The heading state is set during CollectionItemInfo
// construction.
assertFalse(itemInfo.isHeading());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.
Agree with this recommendation.
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.
Added in most recent commit. I changed the assertFalse to assertFalse(nodeInfo.isHeading() since CollectionItemInfo.isHeading() was deprecated in API 28
camsim99
left a comment
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.
LGTM! Thanks for getting this re-landed :)
…oes-not-announce-list-information
|
autosubmit label was removed for flutter/flutter/177622, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…oes-not-announce-list-information
…oes-not-announce-list-information
…oes-not-announce-list-information
…oes-not-announce-list-information
…oes-not-announce-list-information
|
autosubmit label was removed for flutter/flutter/177622, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
…oes-not-announce-list-information
…oes-not-announce-list-information
flutter/flutter@05d6005...5545bb3 2025-12-02 [email protected] Roll Skia from 4371ed0ce49e to 45337c4e919d (1 revision) (flutter/flutter#179342) 2025-12-02 [email protected] Roll Fuchsia Linux SDK from sTk6OB7a4yudbfdZg... to l0DvmZrMHlF12frrX... (flutter/flutter#179338) 2025-12-02 [email protected] Unfocus search anchor bar when the view is closed (flutter/flutter#178910) 2025-12-02 [email protected] Directly generate a Mach-O dynamic library using gen_snapshot. [reland] (flutter/flutter#174870) 2025-12-02 [email protected] Roll Skia from 1fc59bf5cbb1 to 4371ed0ce49e (3 revisions) (flutter/flutter#179326) 2025-12-02 [email protected] [win32] Replace threadpool timer with custom background thread timer (flutter/flutter#179249) 2025-12-02 [email protected] Roll Skia from 61257a1036fb to 1fc59bf5cbb1 (1 revision) (flutter/flutter#179321) 2025-12-02 [email protected] Roll Skia from ef52cf952211 to 61257a1036fb (2 revisions) (flutter/flutter#179319) 2025-12-02 [email protected] Roll Skia from 8887653a773e to ef52cf952211 (1 revision) (flutter/flutter#179316) 2025-12-02 [email protected] Roll pub packages (flutter/flutter#179313) 2025-12-02 [email protected] Update customer tests (flutter/flutter#179309) 2025-12-01 [email protected] Marks Linux_pixel_7pro new_gallery__transition_perf to be unflaky (flutter/flutter#176339) 2025-12-01 [email protected] Fix typo (flutter/flutter#179200) 2025-12-01 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 5.0.1 to 6.0.0 in the all-github-actions group (flutter/flutter#179308) 2025-12-01 [email protected] Roll Dart SDK from c54108eeb2c1 to eb743a1d4ade (1 revision) (flutter/flutter#179304) 2025-12-01 [email protected] Roll pub packages (flutter/flutter#179280) 2025-12-01 [email protected] Roll Skia from 68cc3257e734 to 8887653a773e (4 revisions) (flutter/flutter#179302) 2025-12-01 [email protected] Support round caps for the fast arc stroke generator (flutter/flutter#178269) 2025-12-01 [email protected] Fix for PR #174374 - Fix - TalkBack does not announce list information (flutter/flutter#177622) 2025-12-01 [email protected] Small cleanup in `AccessibilityBridge.java` (flutter/flutter#179226) 2025-12-01 [email protected] Roll Skia from 925c311f4b37 to 68cc3257e734 (44 revisions) (flutter/flutter#179294) 2025-12-01 [email protected] [ Widget Preview ] Ignore changes under `ios/.symlinks` (flutter/flutter#179290) 2025-12-01 [email protected] Delete unecessary lockfile (flutter/flutter#179052) 2025-12-01 [email protected] Resolving and piping the view ID through the WidgetController and the TestPointer so that clicks wind up on the right view (flutter/flutter#178941) 2025-12-01 [email protected] Fix link specified as plain text `FlutterApplication.java` (flutter/flutter#178573) 2025-12-01 [email protected] Update some comments to reflect theme normalization (flutter/flutter#179013) 2025-12-01 [email protected] Roll Dart SDK from 51fe8cd01fbe to c54108eeb2c1 (1 revision) (flutter/flutter#179267) 2025-12-01 [email protected] Explicitly use FreeType font scanner with Fuchsia (flutter/flutter#179055) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ormation (flutter#177622) Fix for PR flutter#174374. It was reverted due to failing tests for creating CollectionInfo and CollectionItemInfo using constructors added in API 30. I used the `obtain` method for creating both CollectionInfo and CollectionItemInfo if the API version is lower than 33. The `obtain` method was deprecated in API 33. > > Updating AccessibilityBridge.java and AccessibilityBridgeTest.java to include AccessibilityNodeInfo.CollectionItemInfo to get TalkBack to announce item indexes for ListViews. > > Updated the logic for adding CollectionInfo to add when the scrollChildren count is greater than 1. This is an attempt to exclude something like a SingleChildScrollView from getting CollectionInfo added. The only semantics info we can rely on from Android is the hasImplicitScrolling and the number of scrollChildren. > Added logic for adding the CollectionItemInfo to ListView children. It first checks to see if the parent node qualifies for CollectionInfo and then adds the CollectionItemInfo to the child view. The index values come from the childrenInTraversalOrder. We aren't currently passing the indexedSemantics into the SemanticsNode. Once those values are available, this can be made more robust. > > The following video includes the new changes with a ListView > > https://github.com/user-attachments/assets/b9bf1b80-9f29-40e2-8f33-a08919554b7e > > > This video includes the logic with a SingleChildScrollView (no list announcements as expected) > > https://github.com/user-attachments/assets/8fe4dd24-cc5a-4a6b-b4b0-0f371fea20df Fixes flutter#168651 - This is a minimal fix. A full fix would be to pass the indexedSemantics to the SemanticsNode in Android and build the list with those values but that is a larger effort. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Signed-off-by: jwlilly <[email protected]> Co-authored-by: Reid Baker <[email protected]> Co-authored-by: Camille Simon <[email protected]>
…ormation (flutter#177622) Fix for PR flutter#174374. It was reverted due to failing tests for creating CollectionInfo and CollectionItemInfo using constructors added in API 30. I used the `obtain` method for creating both CollectionInfo and CollectionItemInfo if the API version is lower than 33. The `obtain` method was deprecated in API 33. > > Updating AccessibilityBridge.java and AccessibilityBridgeTest.java to include AccessibilityNodeInfo.CollectionItemInfo to get TalkBack to announce item indexes for ListViews. > > Updated the logic for adding CollectionInfo to add when the scrollChildren count is greater than 1. This is an attempt to exclude something like a SingleChildScrollView from getting CollectionInfo added. The only semantics info we can rely on from Android is the hasImplicitScrolling and the number of scrollChildren. > Added logic for adding the CollectionItemInfo to ListView children. It first checks to see if the parent node qualifies for CollectionInfo and then adds the CollectionItemInfo to the child view. The index values come from the childrenInTraversalOrder. We aren't currently passing the indexedSemantics into the SemanticsNode. Once those values are available, this can be made more robust. > > The following video includes the new changes with a ListView > > https://github.com/user-attachments/assets/b9bf1b80-9f29-40e2-8f33-a08919554b7e > > > This video includes the logic with a SingleChildScrollView (no list announcements as expected) > > https://github.com/user-attachments/assets/8fe4dd24-cc5a-4a6b-b4b0-0f371fea20df Fixes flutter#168651 - This is a minimal fix. A full fix would be to pass the indexedSemantics to the SemanticsNode in Android and build the list with those values but that is a larger effort. - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Signed-off-by: jwlilly <[email protected]> Co-authored-by: Reid Baker <[email protected]> Co-authored-by: Camille Simon <[email protected]>
Fix for PR #174374. It was reverted due to failing tests for creating CollectionInfo and CollectionItemInfo using constructors added in API 30. I used the
obtainmethod for creating both CollectionInfo and CollectionItemInfo if the API version is lower than 33. Theobtainmethod was deprecated in API 33.Fixes #168651 - This is a minimal fix. A full fix would be to pass the indexedSemantics to the SemanticsNode in Android and build the list with those values but that is a larger effort.
Pre-launch Checklist
///).