-
Notifications
You must be signed in to change notification settings - Fork 6k
Use announce function in live region #38084
Conversation
| _cleanupDom(); | ||
| // Avoid announcing the same message over and over. | ||
| if (_lastAnnouncement != semanticsObject.label) | ||
| { |
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.
nit: leave the open brace on the previous line
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.
Done!
|
|
||
| void _cleanupDom() { | ||
| semanticsObject.element.removeAttribute('aria-live'); | ||
| _lastAnnouncement = null; |
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.
_lastAnnouncement is not a DOM element. It will be garbage-collected automatically. I think we can remove _cleanupDom and the dispose methods, and save a few bytes in payload size.
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.
Good point. I have to keep dispose though because otherwise it throws an error about it not being implemented.
| semanticsObject.element.setAttribute('aria-live', 'polite'); | ||
| } else { | ||
| _cleanupDom(); | ||
| // Avoid announcing the same message over and over. |
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 think the logic that protects us from announcing the same message again is missing a test. It can be tested by clearing the contents of ariaLiveElementFor(Assertiveness.polite) after the first announcement, issuing an update with the same message, and verifying that the element remains empty.
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.
Based on our conversation, I implemented a mock class for AccessibilityAnnouncements and added this test case.
yjbanov
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.
| rect: const ui.Rect.fromLTRB(0, 0, 100, 50), | ||
| ); | ||
| semantics().updateSemantics(builder.build()); | ||
|
|
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.
nit: I'd add expect(mockAccessibilityAnnouncements.announceInvoked, 1); here too to ensure that the count increases after the first update and then stays stable afterwards.
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.
Good point! Done!
|
auto label is removed for flutter/engine, pr: 38084, due to - The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…116633) * 5429243d3 [Impeller Scene] Render imported meshes (flutter/engine#38097) * 67254d6e4 Use announce function in live region (flutter/engine#38084)
…lutter#116633) * 5429243d3 [Impeller Scene] Render imported meshes (flutter/engine#38097) * 67254d6e4 Use announce function in live region (flutter/engine#38084)
…lutter#116633) * 5429243d3 [Impeller Scene] Render imported meshes (flutter/engine#38097) * 67254d6e4 Use announce function in live region (flutter/engine#38084)

Fixes flutter/flutter#108768
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.