[A11y] Add Semantics tree node details - #10018
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 introduces a new SemanticsNodeDetailsPane to display detailed properties (such as label, value, hint, bounding box, and flags) of the selected semantics node in the accessibility screen. It updates the AccessibilityController and SemanticsNodeModel to support node selection, parses additional node details from JSON, and updates the layout and tests accordingly. The review feedback highlights a potential runtime crash in _formatNumber when formatting non-finite coordinates, suggests adding an early-return guard in selectSemanticsNode to avoid redundant state updates, and recommends using safer is num type checks instead of explicit casts in parseRect to prevent runtime type errors.
| title: _valueTitle, | ||
| description: _valueDescription, | ||
| child: _NodeDetailValueBox( | ||
| text: node.value.isNotEmpty ? '"${node.value}"' : null, |
There was a problem hiding this comment.
It looks like there is a check for whether text is empty in _NodeDetailValueBox, so instead of this ternary could we simply pass _NodeDetailValueBox(text: node.value) and have _NodeDetailValueBox handle wrapping the text in quotation?
(Basically is there a way to remove the ternary and empty check from the call site here and below and into the implementation of _NodeDetailValueBox?)
| final textStyle = !hasValue | ||
| ? theme.subtleFixedFontStyle | ||
| : highlightText | ||
| ? theme.fixedFontStyle.copyWith( | ||
| color: colorScheme.primary, | ||
| fontWeight: FontWeight.bold, | ||
| ) | ||
| : theme.fixedFontStyle; |
There was a problem hiding this comment.
[optional] I find nested ternaries hard to read, consider pulling this into method to avoid the nesting
70ad8a8 to
421240b
Compare
421240b to
cea6152
Compare
elliette
left a comment
elliette
left a comment
There was a problem hiding this comment.
LGTM with one suggestion! Sorry for the delay here.
| dataRootsListenable: controller.semanticsRoots, | ||
| scrollController: controller.treeScrollController, | ||
| includeScrollbar: true, | ||
| onItemSelected: controller.selectSemanticsNode, |
There was a problem hiding this comment.
We might want to add onItemExpanded as well - I'm guessing right now every time an item is expanded it is automatically selected as well?
There was a problem hiding this comment.
yeah your guess was correct lol!
updated so expand will not auto select !
|
Thank you for reviewing! :D |
2 participants
tracking issue: #9893
So it will display a selected node details
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.