[A11y] Add semantics tree UI - #9982
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the accessibility semantics tree in DevTools, adding the SemanticsNodeModel to represent nodes, updating AccessibilityController to load and parse the tree via service extensions, and introducing the AccessibilitySemanticsTreePane UI along with comprehensive tests. The review feedback highlights several critical issues: a recursive parsing bug in _parseSemanticsNode that leads to duplicate child nodes, the use of a non-existent disposeSemantics service extension (which should be replaced with disabling enableSemantics), and the use of an invalid isCheckable flag which should be updated to hasCheckedState.
srawlins
left a comment
srawlins
left a comment
There was a problem hiding this comment.
Nice tests; I haven't quite looked at everything yet, but do you want to look at gemini's feedback and give each a thumbs up or down?
| /// The current value of this node (e.g. the text in a text field). | ||
| final String value; | ||
|
|
||
| /// Additional hint text spoken after a delay (maps to `SemanticsData.hint`). |
There was a problem hiding this comment.
Can we link with [SemanticsData.hint]? If it is not imported, we can /// @docImport it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the accessibility semantics tree view in DevTools. It introduces the SemanticsNodeModel to represent nodes in the semantics tree, adds logic to AccessibilityController to load, parse, and dispose of the semantics tree via service extensions, and replaces the placeholder AccessibilitySemanticsTreePane with a fully functional tree view UI (including empty, loading, and error states). Comprehensive unit and widget tests are also added. Feedback on the pull request suggests ensuring that stale semantics tree data and error states are cleared when the main isolate becomes null (e.g., when the application is disconnected).
| addAutoDisposeListener( | ||
| serviceConnection.serviceManager.isolateManager.mainIsolate, | ||
| () { | ||
| if (serviceConnection.serviceManager.isolateManager.mainIsolate.value != | ||
| null) { | ||
| // Clear stale data from a previous isolate so the guard in | ||
| // _autoLoadSemanticsTreeIfNeeded doesn't skip the new load. | ||
| semanticsRoots.value = []; | ||
| semanticsTreeError.value = null; | ||
| unawaited(_autoLoadSemanticsTreeIfNeeded()); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
[MUST-FIX] When the main isolate becomes null (e.g., if the application is disconnected or stopped), the listener does not clear the semantics tree or error state. This can lead to stale data from the previous session being displayed in the UI.
Please add an else block to clear semanticsRoots and semanticsTreeError when mainIsolate.value is null.
addAutoDisposeListener(
serviceConnection.serviceManager.isolateManager.mainIsolate,
() {
if (serviceConnection.serviceManager.isolateManager.mainIsolate.value !=
null) {
// Clear stale data from a previous isolate so the guard in
// _autoLoadSemanticsTreeIfNeeded doesn't skip the new load.
semanticsRoots.value = [];
semanticsTreeError.value = null;
unawaited(_autoLoadSemanticsTreeIfNeeded());
} else {
semanticsRoots.value = [];
semanticsTreeError.value = null;
}
},
);
9e12ba6 to
b4da3ce
Compare
2 participants
tracking issue: #9893
Added the tree UI
Pre-launch Checklist
General checklist
///).Issues checklist
contributions-welcomeorgood-first-issuelabel.contributions-welcomeorgood-first-issuelabel. I understand this means my PR might take longer to be reviewed.Tests checklist
AI-tooling checklist
Feature-change checklist
release-notes-not-requiredlabel or left a comment requesting the label be added.packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.If you need help, consider asking for help on Discord.