Add accessibilityAnnouncement matcher #180058
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new accessibilityAnnouncement matcher to simplify testing of accessibility announcements. The implementation is clean and well-documented. The new matcher is used to refactor several existing tests, making them more concise and readable. I have one suggestion to improve the description of the matcher for better test failure messages.
bcf00ec to
25af693
Compare
25af693 to
64c48a3
Compare
| /// | ||
| /// * [WidgetTester.takeAnnouncements], which retrieves the announcements. | ||
| /// * [SemanticsService.sendAnnouncement], which sends an announcement. | ||
| Matcher accessibilityAnnouncement( |
There was a problem hiding this comment.
The name can be more descriptive, something like containsAccessibilityAnnouncement. Our current nameing convenient is that keyword contains means it can have optional argument that only check against when set, otherwise, it should be matches
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [WidgetTester.takeAnnouncements], which retrieves the announcements. |
There was a problem hiding this comment.
| /// * [WidgetTester.takeAnnouncements], which retrieves the announcements. | |
| /// * [WidgetTester.takeAnnouncements], which retrieves the announcements in unit tests. |
| /// | ||
| /// ```dart | ||
| /// await SemanticsService.sendAnnouncement(tester.view, 'Hello', TextDirection.ltr); | ||
| /// expect(tester.takeAnnouncements(), contains(containsAccessibilityAnnouncement('Hello'))); |
There was a problem hiding this comment.
contains(containsAccessibilityAnnouncement(...)) seems a little funky. What do you think of renaming containsAccessibilityAnnouncement to isAccessibilityAnnouncement?
The isFoo naming convention seems to better match other matchers like isOffstage, isSystemTextScaler, etc.
There was a problem hiding this comment.
Ah I see contains was suggested above: #180058 (comment)
Let me discuss offline.
There was a problem hiding this comment.
@loic-sharma
I considered that as well. However, since containsSemantics already exists nearby, I used containsAccessibilityAnnouncement for consistency. Maybe we should just update the example in the comment?
There was a problem hiding this comment.
@chunhtai and I discussed this. TLDR: Let's rename this to isAccessibilityAnnouncement.
Longer version:
We don't have a consistent naming convention. We'd like to support several kinds of matchers:
-
Partial matchers. These only match the properties you provide. Example:
isFoo(property1: true).The matcher's arguments are
nullby default. The matcher only matches on non-nullvalues you provide.Unfortunately, you cannot use this partial matcher to assert a value is
null. For that you'd use... -
Exact matchers. These match all the properties, including those you do not provide. Example:
matchesFoo(property1: true, nullablePropery: null)The matcher's arguments have the same default value as the object's property's default value.
To adopt this naming convention, we'll want to:
- Add this to our style guide
- Add a new
isSemanticsmatcher, and deprecatecontainsSemantics.
I'll file an issue to track this work.
04c6793 to
913def7
Compare
loic-sharma
left a comment
There was a problem hiding this comment.
Thanks for the wonderful patch!
Fix #180057
Pre-launch Checklist
///).