Fixed the ability to scroll to the top on iOS 13 #16820
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,15 +29,6 @@ FLUTTER_EXPORT | |
|
|
||
| @property(strong, nonatomic) UIWindow* window; | ||
|
|
||
| @end | ||
|
|
||
| #endif // FLUTTER_FLUTTERDARTPROJECT_H_ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,9 @@ | |
| #import "flutter/shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.h" | ||
| #import "flutter/shell/platform/darwin/ios/platform_view_ios.h" | ||
|
|
||
| static constexpr int kMicrosecondsPerSecond = 1000 * 1000; | ||
| static constexpr CGFloat kScrollViewContentSize = 2.0; | ||
|
|
||
| NSNotificationName const FlutterSemanticsUpdateNotification = @"FlutterSemanticsUpdate"; | ||
|
|
||
| NSNotificationName const FlutterViewControllerWillDealloc = @"FlutterViewControllerWillDealloc"; | ||
|
|
@@ -86,7 +89,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, UIScrollViewDelegate> | ||
| @property(nonatomic, readwrite, getter=isDisplayingFlutterUI) BOOL displayingFlutterUI; | ||
| @end | ||
|
|
||
|
|
@@ -123,6 +126,11 @@ @implementation FlutterViewController { | |
| // Coalescer that filters out superfluous keyboard notifications when the app | ||
| // is being foregrounded. | ||
| fml::scoped_nsobject<FlutterCoalescer> _updateViewportMetrics; | ||
| // This scroll view 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 with height zero and a content offset so we can get those events. See also: | ||
| // https://github.com/flutter/flutter/issues/35050 | ||
| fml::scoped_nsobject<UIScrollView> _scrollView; | ||
|
There was a problem hiding this comment. Add a comment for why we own a uiscrollview There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| } | ||
|
|
||
| @synthesize displayingFlutterUI = _displayingFlutterUI; | ||
|
|
@@ -329,6 +337,40 @@ - (void)loadView { | |
| self.view.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; | ||
|
|
||
| [self installSplashScreenViewIfNecessary]; | ||
| UIScrollView* scrollView = [[UIScrollView alloc] init]; | ||
|
There was a problem hiding this comment. Comment for why in needs to be a scrollview rather than any uiview There was a problem hiding this comment. 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(kScrollViewContentSize, kScrollViewContentSize); | ||
| // This is an arbitrary offset that is not CGPointZero. | ||
| scrollView.contentOffset = CGPointMake(kScrollViewContentSize, kScrollViewContentSize); | ||
| [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); | ||
|
There was a problem hiding this comment. Wouldnt it make more sense to send a message to flutter instead of sending fake touch events? @xster There was a problem hiding this comment. FWIW I did it this way since thats how the code was previously working. I tried to change as little as possible. There was a problem hiding this comment. 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? There was a problem hiding this comment. 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). There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 +611,6 @@ - (void)flushOngoingTouches { | |
| flutter::PointerData pointer_data; | ||
| pointer_data.Clear(); | ||
|
|
||
| // Use current time. | ||
| pointer_data.time_stamp = [[NSDate date] timeIntervalSince1970] * kMicrosecondsPerSecond; | ||
|
|
||
|
|
@@ -835,6 +876,10 @@ - (void)viewDidLayoutSubviews { | |
| CGSize viewSize = self.view.bounds.size; | ||
| CGFloat scale = [UIScreen mainScreen].scale; | ||
|
|
||
| // Purposefully place this not visible. | ||
| _scrollView.get().frame = CGRectMake(0.0, 0.0, viewSize.width, 0.0); | ||
| _scrollView.get().contentOffset = CGPointMake(kScrollViewContentSize, kScrollViewContentSize); | ||
|
|
||
| // 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 style | ||
|
|
||
| - (UIStatusBarStyle)preferredStatusBarStyle { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // Copyright 2020 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #import <XCTest/XCTest.h> | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface StatusBarTest : XCTestCase | ||
| @property(nonatomic, strong) XCUIApplication* application; | ||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| // Copyright 2020 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #import "StatusBarTest.h" | ||
|
|
||
| @implementation StatusBarTest | ||
|
|
||
| - (void)setUp { | ||
| [super setUp]; | ||
| self.continueAfterFailure = NO; | ||
|
|
||
| self.application = [[XCUIApplication alloc] init]; | ||
| self.application.launchArguments = @[ @"--tap-status-bar" ]; | ||
| [self.application launch]; | ||
| } | ||
|
|
||
| - (void)testTapStatusBar { | ||
| if (@available(iOS 13, *)) { | ||
| XCUIApplication* systemApp = | ||
| [[XCUIApplication alloc] initWithBundleIdentifier:@"com.apple.springboard"]; | ||
|
There was a problem hiding this comment. 😮 so they straight up moved the status bar out of the app and added some special hook to the uiscrollview to be able to catch it? There was a problem hiding this comment. The hook for uiscrollviews always existed (since like ios 2.0), they didn't break that one. |
||
| XCUIElement* statusBar = [systemApp.statusBars firstMatch]; | ||
| if (statusBar.isHittable) { | ||
| [statusBar tap]; | ||
| } else { | ||
| XCUICoordinate* coordinates = [statusBar coordinateWithNormalizedOffset:CGVectorMake(0, 0)]; | ||
| [coordinates tap]; | ||
| } | ||
| } else { | ||
| [[self.application.statusBars firstMatch] tap]; | ||
| } | ||
|
|
||
| XCUIElement* addTextField = self.application.textFields[@"PointerChange.add"]; | ||
|
There was a problem hiding this comment. oh ha. Though can't you just put the channel handler you had in the AppDelegate in this test itself? Then you can just [self waitForExpectations] against its own listener like AppLifecycleTests? There was a problem hiding this comment. You can't do that. AppLifecycleTests are unit tests and run in the same process as the app. UI tests run as a separate process, I'd have to do IPC to communicate between the processes. There is a builtin mechanism to do IPC for UI elements though so that's what I'm using here. There was a problem hiding this comment. Doh, you're right, this is a XCUITest |
||
| BOOL exists = [addTextField waitForExistenceWithTimeout:1]; | ||
| XCTAssertTrue(exists, @""); | ||
| XCUIElement* upTextField = self.application.textFields[@"PointerChange.up"]; | ||
| exists = [upTextField waitForExistenceWithTimeout:1]; | ||
| XCTAssertTrue(exists, @""); | ||
| } | ||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // Copyright 2020 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:convert'; | ||
| import 'dart:ui'; | ||
|
|
||
| import 'scenario.dart'; | ||
|
|
||
| /// A scenario that sends back messages when touches are received. | ||
| class TouchesScenario extends Scenario { | ||
| /// Constructor for `TouchesScenario`. | ||
| TouchesScenario(Window window) : super(window); | ||
|
|
||
| @override | ||
| void onBeginFrame(Duration duration) {} | ||
|
|
||
| @override | ||
| void onPointerDataPacket(PointerDataPacket packet) { | ||
| window.sendPlatformMessage( | ||
| 'touches_scenario', | ||
| utf8.encoder | ||
| .convert(const JsonCodec().encode(<String, dynamic>{ | ||
| 'change': packet.data[0].change.toString(), | ||
| })) | ||
| .buffer | ||
| .asByteData(), | ||
| 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.
Add a comment for the interface like binary messenger
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.
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.