-
Notifications
You must be signed in to change notification settings - Fork 75
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
[a11y] Pass the id of the node whose layout changed to accessibility controllers #1162
Conversation
…controllers, rather than simply call onSemanticsChange.
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.
Blocking to agree on API change before merging
* Note that the id, rather than the [LayoutNode] itself, is passed here because | ||
* [LayoutNode] is an internal type, so it can't be exposed in a public method. | ||
*/ | ||
fun onLayoutChange(semanticsOwner: SemanticsOwner, semanticsNodeId: Int) |
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.
We already discussed this with @elijah-semyonov and as far as I remember we decided to introduce optional second parameter to onSemanticsChange
instead of adding a new callback
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.
Having it in a separate function adds more information; namely that only the bounds of the node have changed, and not any semantic properties.
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 API changed here isn't public, by the way, so no need for the "changes in API" tag.
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.
it's public but with "internal" annotation. The rule for tag is basically - "changes in desktop/ui.api
file".
I'd apply the rule regarding two reviewers here
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 disagree. @internalapi implies that it's internal, and only technically public because of the limitations of the language. We don't make any compatibility guarantees about such code.
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.
ok, it's not a big deal in this case
* Note that the id, rather than the [LayoutNode] itself, is passed here because | ||
* [LayoutNode] is an internal type, so it can't be exposed in a public method. | ||
*/ | ||
fun onLayoutChange(semanticsOwner: SemanticsOwner, semanticsNodeId: Int) |
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.
ok, it's not a big deal in this case
Currently, when the layout of a single node changes, we call onSemanticsChange on (desktop)
AccessibilityController
and (iOS)AccessibilityMediator
, which causes them to re-sync the entire node tree.Proposed Changes
Pass the semantics id of the node whose layout changed to the accessibility controllers, so they can invalidate the tree more granularly. Note that this PR only passes the information; the actual granular handling will be done in separate PR(s).
Testing
Test: Manually, only on the desktop.