[macOS]Support SemanticsService.announce #40585
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
hannah-hyj
changed the title
| NSAccessibilityPostNotificationWithUserInfo( | ||
| [self viewControllerForId:kFlutterDefaultViewId].flutterView, | ||
| NSAccessibilityAnnouncementRequestedNotification, | ||
| @{NSAccessibilityAnnouncementKey : message, NSAccessibilityPriorityKey : @(priority)}); | ||
| } |
There was a problem hiding this comment.
Top-level AppKit API functions are always a bit tricky to test. There are a few tricks you can use for this. A simple one in this case is:
- Extract this to a method like:
- (void)announceAccessibilityMessage:(NSString*)withPriority:(NSAccessibilityPrioirityKey);- Use
CreateMockFlutterEngine()to create a partial mock in your test. - Verify the method got called in the test:
[[engineMock expect] announceAccessibilityMessage:@"Foo" withPriority:...];
// Fire an event here.
[engineMock verify];
|
|
||
| OCMStub([engineMock announceAccessibilityMessage:[OCMArg any] withPriority:[OCMArg any]]) | ||
| .andDo((^(NSInvocation* invocation) { | ||
| announced = true; |
There was a problem hiding this comment.
Can you also double check message and priority?
There was a problem hiding this comment.
+1. You can get these via [invocation getArgument:AtIndex:].
| NSString* type = annotatedEvent[@"type"]; | ||
| if ([type isEqualToString:@"announce"]) { | ||
| NSString* message = annotatedEvent[@"data"][@"message"]; | ||
| NSNumber* assertiveness = annotatedEvent[@"data"][@"assertiveness"]; |
There was a problem hiding this comment.
Does this crash if data, message, or assertiveness properties are missing? A malformed message should be ignored, though an error can be logged.
(Asking as there was a recent crash on Windows embedder due to the framework sending unexpected clipboard messages)
There was a problem hiding this comment.
These will result in nil, so the current code handles the cases where any/all of these are missing.
To add a bit of detail:
- We guard against a nil message below since announcing nil makes no sense.
- In the case of nil
assertivenesswe set the default priority; since messages to nil evaluate to nil, the ternary evaluates toNSAccessibilityPriorityMedium.
Co-authored-by: Chris Bracken <[email protected]>
) (flutter#123745) Roll Flutter Engine from 047fc60dd0a9 to 89c1b23cc6cd (1 revision)
issue:flutter/flutter#99715
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.