-
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.
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.