-
Notifications
You must be signed in to change notification settings - Fork 6k
ios accessibility: started ignoring route changes when presenting modal view controllers #18544
Conversation
|
Nobody moved on this PR so I'm going to change it to incorporate it in my fix for the related issue. |
6ec595d to
59dba26
Compare
59dba26 to
3db91b9
Compare
3db91b9 to
461c2d0
Compare
|
@goderbauer @GaryQian friendly ping on review |
|
@goderbauer @GaryQian less friendly ping for review ;) |
| } else if (layoutChanged) { | ||
| // TODO(goderbauer): figure out which node to focus next. | ||
| UIAccessibilityPostNotification(UIAccessibilityLayoutChangedNotification, nil); | ||
| ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, nil); |
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.
Why run the notification through the delegate as well?
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.
For testing. We can't test that someone has called UIAccessibilityPostNotification so the delegate allows us to do dependency injection.
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.
That makes sense. Thanks for the explanation :)
|
@jmagman @goderbauer has requested someone with more objc experience to do a review |
|
Thee a11y stuff looks good. Can you please have somebody with ObjC expertise take a look at this as well? |
jmagman
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.
Can the model view controller itself be a FlutterViewController? Should it itself handle being presented modally with viewWillAppear/isBeingPresented? Maybe add-to-app?
| namespace flutter { | ||
| namespace { | ||
|
|
||
| FlutterViewController* GetFlutterViewControllerForView(UIView* view) { |
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.
This function name sounds like it would return the view.nextResponder if the viewcontroller is a FlutterViewController, not that it would necessarily walk up the responder chain.
Maybe GetNextFlutterResponder?
FlutterViewController*_Nullable GetNextFlutterResponder(id<UIResponder> view) {
id nextResponder = [view nextResponder];
if ([nextResponder isKindOfClass:[FlutterViewController class]]) {
return nextResponder;
}
if ([nextResponder conformsToProtocol:@protocol(UIResponder)) {
return GetFlutterViewControllerForView(nextResponder);
}
return nil;
}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.
If i named it that, the name would match better with the implementation details, but it would be less clear at the call site what I was doing. If you ignore the implementation details this function does what the function name suggests. It's an artifact of not having a good way to do this in UIKit. If wanted to tear things up more I'd stop tracking a UIView in the bridge and actually trace a FlutterViewController so we wouldn't need this. I'll add the nullable annotation and add a comment to explain the implementation.
| nodes[root_node.id] = root_node; | ||
| bridge->UpdateSemantics(/*nodes=*/nodes, /*actions=*/actions); | ||
|
|
||
| XCTAssertEqual([accessibility_notifications count], 1ul); |
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 don't think the UL is necessary any more (there was a time where you'd get a compilation warning, but they fixed that a few years ago).
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 just removed it to see what happens and it still creates an error.
| std::string label = "some label"; | ||
|
|
||
| NSMutableArray<NSDictionary<NSString*, id>*>* accessibility_notifications = | ||
| [[[NSMutableArray alloc] init] autorelease]; |
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.
Should retain and release this instead of letting the autorelease pool handle it (usually -autorelease if you're returning a value from a method, say, not if you actually are in control of the scope). But yeah it's a test so whatever.
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.
In ObjC++ the rules are a bit different because of RAII. If I didn't put an autorelease here I should have a RAII wrapper around the pointer... I tried to get this file compiling with ARC but there is a hangup supporting that. Hopefully we can migrate it.
| [super presentViewController:viewControllerToPresent | ||
| animated:flag | ||
| completion:^{ | ||
| self.isPresentingViewControllerAnimating = NO; |
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.
This is my own UIViewController ignorance showing, but what happens if you present two in a row? I'm guessing one animation finishes before the next one starts?
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.
Presenting one while the other is animating? No idea. I would actually expect the first animation to be cancelled.
| nodes[root_node.id] = root_node; | ||
| bridge->UpdateSemantics(/*nodes=*/nodes, /*actions=*/actions); | ||
|
|
||
| XCTAssertEqual([accessibility_notifications count], 0ul); |
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.
Same.
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.
ack
There is nothing stopping it from being a FlutterViewController. If it was one, I would expect There may be a difference when using a FlutterViewController with a shared engine versus its own engine. With a shared engine I could see there being some timing issues related to creating the accessibility bridge. |
controllers and added a test that exercises announcing route changes.
33c6031 to
59d9c54
Compare
| namespace { | ||
|
|
||
| FlutterViewController* _Nullable GetFlutterViewControllerForView(UIView* view) { | ||
| // There is no way to get a view's view controller in UIKit directly, this is |
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.
Looks like platformviewios right above it has a FlutterViewController anyway. We can just refactor this private constructor to take a FVC if needed.
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.
Yea, i can look into it. i'll split it into a different PR to keep the bug fix and the refactor separate.
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.
SG
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.
|
landing on purple |
…ting modal view controllers (flutter/engine#18544)
…ting modal view controllers (flutter/engine#18544)
…ting modal view controllers (flutter/engine#18544)
…ting modal view controllers (flutter/engine#18544)
…ting modal view controllers (flutter/engine#18544)
…ting modal view controllers (flutter/engine#18544)
Relevant issue flutter/flutter#56850
This PR ignores route change notifications in cases where a modal view controller is being presented.
Since there doesn't seem to be a reliable way to query if a view controller is animating in a modal view controller we had to add an extra property to FlutterViewController to keep track of that. Now when it is presenting we are mum about route changes since it would only serve to confuse.