-
Notifications
You must be signed in to change notification settings - Fork 6k
Fixed the ability to scroll to the top on iOS 13 #16820
Changes from 5 commits
eceab41
c2a7861
b861aff
1db613a
f182e10
663b804
7cc5adb
944c0ed
3eeeeb9
e520d30
0c88a32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ | |
| #import "flutter/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.h" | ||
| #import "flutter/shell/platform/darwin/ios/platform_view_ios.h" | ||
|
|
||
| constexpr int kMicrosecondsPerSecond = 1000 * 1000; | ||
|
|
||
| NSNotificationName const FlutterSemanticsUpdateNotification = @"FlutterSemanticsUpdate"; | ||
|
|
||
| NSNotificationName const FlutterViewControllerWillDealloc = @"FlutterViewControllerWillDealloc"; | ||
|
|
@@ -86,7 +88,7 @@ - (void)invalidate { | |
| // This is left a FlutterBinaryMessenger privately for now to give people a chance to notice the | ||
| // change. Unfortunately unless you have Werror turned on, incompatible pointers as arguments are | ||
| // just a warning. | ||
| @interface FlutterViewController () <FlutterBinaryMessenger> | ||
| @interface FlutterViewController () <FlutterBinaryMessenger, UIScrollViewDelegate> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment for the interface like binary messenger
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't document every time we implement a protocol in the codebase. FlutterBinaryMessenger was a special case since it was a special migration step. |
||
| @property(nonatomic, readwrite, getter=isDisplayingFlutterUI) BOOL displayingFlutterUI; | ||
| @end | ||
|
|
||
|
|
@@ -123,6 +125,7 @@ @implementation FlutterViewController { | |
| // Coalescer that filters out superfluous keyboard notifications when the app | ||
| // is being foregrounded. | ||
| fml::scoped_nsobject<FlutterCoalescer> _updateViewportMetrics; | ||
| fml::scoped_nsobject<UIScrollView> _scrollView; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment for why we own a uiscrollview
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of that information is documented at the site of instantiation. We don't have a practice of documenting every instance variable, it would just be a duplication of what is said at the instantiation site.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that documentation is better served here than down below. If you see a _scrollView and don't know where it is, you can go-to-definition to see the reasoning. It's less direct to see where the scoped_nsobject is set and then find the local variable's construction which populates it. You can also add one more line to say that UIScrollView specifically has a https://developer.apple.com/documentation/uikit/uiscrollviewdelegate/1619378-scrollviewshouldscrolltotop callback which we game here as a hint for future maintainers.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| } | ||
|
|
||
| @synthesize displayingFlutterUI = _displayingFlutterUI; | ||
|
|
@@ -329,6 +332,46 @@ - (void)loadView { | |
| self.view.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; | ||
|
|
||
| [self installSplashScreenViewIfNecessary]; | ||
| // This is a workaround to accomodate iOS 13 and higher. There isn't a way to get touches on the | ||
| // status bar to trigger scrolling to the top of a scroll view. We place a UIScrollView offscreen | ||
| // with a content offset so we can get those events. See also: | ||
| // https://github.com/flutter/flutter/issues/35050 | ||
| UIScrollView* scrollView = [[UIScrollView alloc] init]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment for why in needs to be a scrollview rather than any uiview
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That information is already included in the current comment. |
||
| scrollView.autoresizingMask = UIViewAutoresizingFlexibleWidth; | ||
| // The color shouldn't matter since it is offscreen. | ||
| scrollView.backgroundColor = UIColor.whiteColor; | ||
| scrollView.delegate = self; | ||
| // This is an arbitrary small size. | ||
| scrollView.contentSize = CGSizeMake(10, 10); | ||
| // This is an arbitrary offset that is not CGPointZero. | ||
| scrollView.contentOffset = CGPointMake(10, 10); | ||
| // This is some aribrary place offscreen. | ||
| scrollView.bounds = CGRectMake(0, -10, 0, 10); | ||
| [self.view addSubview:scrollView]; | ||
| _scrollView.reset(scrollView); | ||
| } | ||
|
|
||
| static void sendFakeTouchEvent(FlutterEngine* engine, | ||
| CGPoint location, | ||
| flutter::PointerData::Change change) { | ||
| const CGFloat scale = [UIScreen mainScreen].scale; | ||
| flutter::PointerData pointer_data; | ||
| pointer_data.Clear(); | ||
| pointer_data.physical_x = location.x * scale; | ||
| pointer_data.physical_y = location.y * scale; | ||
| pointer_data.kind = flutter::PointerData::DeviceKind::kTouch; | ||
| pointer_data.time_stamp = [[NSDate date] timeIntervalSince1970] * kMicrosecondsPerSecond; | ||
| auto packet = std::make_unique<flutter::PointerDataPacket>(/*count=*/1); | ||
| pointer_data.change = change; | ||
| packet->SetPointerData(0, pointer_data); | ||
| [engine dispatchPointerDataPacket:std::move(packet)]; | ||
| } | ||
|
|
||
| - (BOOL)scrollViewShouldScrollToTop:(UIScrollView*)scrollView { | ||
| CGPoint statusBarPoint = CGPointZero; | ||
| sendFakeTouchEvent(_engine.get(), statusBarPoint, flutter::PointerData::Change::kDown); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldnt it make more sense to send a message to flutter instead of sending fake touch events? @xster
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I did it this way since thats how the code was previously working. I tried to change as little as possible.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I asked heaving in mind that flutter/flutter#42560 will be implemented somewhere in the future. @xster should I make a separate issue for this or add a comment to the one linked?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a bit pre-emptive but it would be nice to check whether the flutter vc is top aligned before doing it (so we don't scroll if it's nested in the middle of the page).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unnecessary because the statusbar is guaranteed to be top aligned if visible. If it isn't visible, the call to scrollViewShouldScrollToTop won't be called.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than being invisible, I meant if Flutter was embedded as a partial view in another parent native view. Would Flutter get this event even though it's not at the top of the screen? Maybe it already does the right thing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scrollview doesn't need to be at the top of the screen, but it has to be on screen in order to get the event.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant rather that if Flutter was embedded as an entry in a native table, and you tap the top of the screen, you wouldn't want to send this synthesized touch to Flutter |
||
| sendFakeTouchEvent(_engine.get(), statusBarPoint, flutter::PointerData::Change::kUp); | ||
| return NO; | ||
| } | ||
|
|
||
| #pragma mark - Managing launch views | ||
|
|
@@ -569,7 +612,6 @@ - (void)flushOngoingTouches { | |
| flutter::PointerData pointer_data; | ||
| pointer_data.Clear(); | ||
|
|
||
| constexpr int kMicrosecondsPerSecond = 1000 * 1000; | ||
| // Use current time. | ||
| pointer_data.time_stamp = [[NSDate date] timeIntervalSince1970] * kMicrosecondsPerSecond; | ||
|
|
||
|
|
@@ -835,6 +877,9 @@ - (void)viewDidLayoutSubviews { | |
| CGSize viewSize = self.view.bounds.size; | ||
| CGFloat scale = [UIScreen mainScreen].scale; | ||
|
|
||
| // Purposefully place this offscreen. | ||
| _scrollView.get().bounds = CGRectMake(0.0, -10.0, viewSize.width, 0); | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
|
|
||
| // First time since creation that the dimensions of its view is known. | ||
| bool firstViewBoundsUpdate = !_viewportMetrics.physical_width; | ||
| _viewportMetrics.device_pixel_ratio = scale; | ||
|
|
@@ -1146,42 +1191,6 @@ - (NSString*)contrastMode { | |
| } | ||
| } | ||
|
|
||
| #pragma mark - Status Bar touch event handling | ||
|
|
||
| // Standard iOS status bar height in points. | ||
| constexpr CGFloat kStandardStatusBarHeight = 20.0; | ||
|
|
||
| - (void)handleStatusBarTouches:(UIEvent*)event { | ||
| CGFloat standardStatusBarHeight = kStandardStatusBarHeight; | ||
| if (@available(iOS 11, *)) { | ||
| standardStatusBarHeight = self.view.safeAreaInsets.top; | ||
| } | ||
|
|
||
| // If the status bar is double-height, don't handle status bar taps. iOS | ||
| // should open the app associated with the status bar. | ||
| CGRect statusBarFrame = [UIApplication sharedApplication].statusBarFrame; | ||
| if (statusBarFrame.size.height != standardStatusBarHeight) { | ||
| return; | ||
| } | ||
|
|
||
| // If we detect a touch in the status bar, synthesize a fake touch begin/end. | ||
| for (UITouch* touch in event.allTouches) { | ||
| if (touch.phase == UITouchPhaseBegan && touch.tapCount > 0) { | ||
| CGPoint windowLoc = [touch locationInView:nil]; | ||
| CGPoint screenLoc = [touch.window convertPoint:windowLoc toWindow:nil]; | ||
| if (CGRectContainsPoint(statusBarFrame, screenLoc)) { | ||
| NSSet* statusbarTouches = [NSSet setWithObject:touch]; | ||
|
|
||
| flutter::PointerData::Change change = flutter::PointerData::Change::kDown; | ||
| [self dispatchTouches:statusbarTouches pointerDataChangeOverride:&change]; | ||
| change = flutter::PointerData::Change::kUp; | ||
| [self dispatchTouches:statusbarTouches pointerDataChangeOverride:&change]; | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #pragma mark - Status bar style | ||
|
|
||
| - (UIStatusBarStyle)preferredStatusBarStyle { | ||
|
|
||
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.
Add comment for what this is supposed to be for.
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.
It's a mathematical constant like pi. It has meaning outside of the context of usage. Documentation and design should be hierarchical, if you start documenting where things are used, you'll get cyclical documentation which is harder to maintain and thus ultimately less useful.
Added static declaration.