Fixed the ability to scroll to the top on iOS 13 #16820

gaaclarke · 2020-02-26T22:24:48Z

Introduced a UIScrollView to FlutterViewController as a workaround.

Tested manually on iOS 13.3, iOS 12.4, iOS 10.3.1.

uiscrollview to FlutterViewController.

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

Kavantix · 2020-02-27T09:38:45Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+
+- (BOOL)scrollViewShouldScrollToTop:(UIScrollView*)scrollView {
+  CGPoint statusBarPoint = CGPointZero;
+  sendFakeTouchEvent(_engine.get(), statusBarPoint, flutter::PointerData::Change::kDown);


Wouldnt it make more sense to send a message to flutter instead of sending fake touch events? @xster

FWIW I did it this way since thats how the code was previously working. I tried to change as little as possible.

I asked heaving in mind that flutter/flutter#42560 will be implemented somewhere in the future.
But it might make more sense to change the way the tap is handled on the dart side.
I believe currently it is done by the scaffold and should probably be moved to the routes.

@xster should I make a separate issue for this or add a comment to the one linked?

xster

The approach is sensible and LGTM.

Here's the fun part: I think we're not far infra-wise to test this in testing/scenario_app/ios tests to create an xcuitest that taps the top of the screen and that checks for a platform message in the framework mock. We don't even need earlgreyflutter since we're not using real flutter.

xster · 2020-03-02T21:18:43Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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;


Add comment for what this is supposed to be for.

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.

xster · 2020-03-02T21:19:03Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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>


Add a comment for the interface like binary messenger

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.

xster · 2020-03-02T21:19:14Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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;


Add a comment for why we own a uiscrollview

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.

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.

xster · 2020-03-02T21:19:43Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+  // 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];


Comment for why in needs to be a scrollview rather than any uiview

That information is already included in the current comment.

xster · 2020-03-03T19:56:29Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+
+- (BOOL)scrollViewShouldScrollToTop:(UIScrollView*)scrollView {
+  CGPoint statusBarPoint = CGPointZero;
+  sendFakeTouchEvent(_engine.get(), statusBarPoint, flutter::PointerData::Change::kDown);


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).

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.

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.

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.

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

xster · 2020-03-03T20:59:57Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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"

+constexpr int kMicrosecondsPerSecond = 1000 * 1000;
+constexpr CGFloat kScrollViewContentSize = 10.0;


I wonder if 10x10 in a uiview equates a calayer with a buffer size of 10x10 (i.e. why not 1x1)?

1x1 seems risky since technically a 1x1 shouldn't have an offset (unless fractional coordinates are allowed). I've moved it to 2x2, worse case scenario it represents 16bytes.

gaaclarke

@xster Let me try a bit with the test. Here is the problem though: In order to catch the breakage that happened in iOS 13 we'd have to run our tests against the latest OS as soon as it was launched. Right now there is a lag time upgrading tests to the new operating system because they are usually closely tied to an operating system.

gaaclarke · 2020-03-03T21:30:43Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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;


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.

gaaclarke · 2020-03-03T21:33:33Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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"

+constexpr int kMicrosecondsPerSecond = 1000 * 1000;
+constexpr CGFloat kScrollViewContentSize = 10.0;


1x1 seems risky since technically a 1x1 shouldn't have an offset (unless fractional coordinates are allowed). I've moved it to 2x2, worse case scenario it represents 16bytes.

gaaclarke · 2020-03-03T21:36:30Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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>


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.

gaaclarke · 2020-03-03T21:37:25Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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;


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.

gaaclarke · 2020-03-03T21:39:06Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+  // 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];


That information is already included in the current comment.

gaaclarke · 2020-03-03T21:44:12Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+
+- (BOOL)scrollViewShouldScrollToTop:(UIScrollView*)scrollView {
+  CGPoint statusBarPoint = CGPointZero;
+  sendFakeTouchEvent(_engine.get(), statusBarPoint, flutter::PointerData::Change::kDown);


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.

gaaclarke · 2020-03-03T22:27:38Z

@xster I played around with scenario tests for the engine. Since we don't have access to the flutter framework we can't add tests as part of this PR. I'll look into adding one for the framework.

xster

That's what I meant by the scenario tests at testing/scenario_app/ios/Scenarios/ScenariosUITests. We don't actually need the Flutter framework (nor do we care about whether the Flutter framework correctly handles this signal downstream in this specific side of the test).

The mock framework is at testing/scenario_app/lib/src. You can use a platform channel to send the fake touch event back for assertions.

The primary objective at this point wouldn't be so much to catch it on all multiplexed versions of iOS, but at least to have a regression test if we ever break this in a refactor. For the versions, it's also a chicken and egg issue between having the test driving and the test environment to get both sides in :)

xster · 2020-03-03T22:33:18Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -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;


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.

gaaclarke · 2020-03-04T02:05:07Z

@xster added UI test

xster · 2020-03-04T07:30:26Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+  // 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
+


nit: whiteline before rather than after.

xster · 2020-03-04T07:32:14Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+
+- (BOOL)scrollViewShouldScrollToTop:(UIScrollView*)scrollView {
+  CGPoint statusBarPoint = CGPointZero;
+  sendFakeTouchEvent(_engine.get(), statusBarPoint, flutter::PointerData::Change::kDown);


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.

xster · 2020-03-04T07:34:19Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/StatusBarTest.m

+- (void)testTapStatusBar {
+  if (@available(iOS 13, *)) {
+    XCUIApplication* systemApp =
+        [[XCUIApplication alloc] initWithBundleIdentifier:@"com.apple.springboard"];


😮 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?

The hook for uiscrollviews always existed (since like ios 2.0), they didn't break that one.

xster · 2020-03-04T07:39:19Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/StatusBarTest.m

+    [[self.application.statusBars firstMatch] tap];
+  }
+
+  XCUIElement* addTextField = self.application.textFields[@"PointerChange.add"];


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?

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.

Doh, you're right, this is a XCUITest

xster · 2020-03-04T18:47:18Z

LGTM

…ine#16820)

`handleStatusBarTouches` is removed in flutter/engine#16820, this part of the documentation is obsolete. Fixes flutter/flutter#63051 (comment)

googlebot added the cla: yes label Feb 26, 2020

Fixed the ability to scroll to the top on ios 13 by introducing a

eceab41

uiscrollview to FlutterViewController.

gaaclarke force-pushed the ios13-fix-scroll-to-top branch from f6570a4 to eceab41 Compare February 26, 2020 22:27

gaaclarke added 4 commits February 26, 2020 15:23

Removed old code for handling tapping the statusbar, tested on 10.3.1.

c2a7861

Updated comments.

b861aff

updated comments again

1db613a

added comment

f182e10

gaaclarke marked this pull request as ready for review February 26, 2020 23:37

gaaclarke requested review from cbracken and xster February 26, 2020 23:38

gaaclarke mentioned this pull request Feb 26, 2020

Tapping appbar no longer scrolls to top flutter/flutter#35050

Closed

gaaclarke commented Feb 27, 2020

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Outdated Show resolved Hide resolved

Kavantix reviewed Feb 27, 2020

View reviewed changes

made the scroll view have height zero

663b804

xster reviewed Mar 3, 2020

View reviewed changes

gaaclarke commented Mar 3, 2020

View reviewed changes

xster reviewed Mar 3, 2020

View reviewed changes

gaaclarke added 4 commits March 3, 2020 17:21

made consts static

7cc5adb

added ui test

944c0ed

moved comment

3eeeeb9

reverted accidental commits

e520d30

xster reviewed Mar 4, 2020

View reviewed changes

removed whitespace

0c88a32

gaaclarke merged commit 59b4d9b into flutter:master Mar 4, 2020

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 4, 2020

59b4d9b Fixed the ability to scroll to the top on iOS 13 (flutter/eng…

87dbd92

…ine#16820)

engine-flutter-autoroll mentioned this pull request Mar 4, 2020

Roll engine e2b74ab80eb4..b77f509e0bb8 (5 commits) flutter/flutter#51965

Merged

cyanglaz mentioned this pull request Nov 16, 2022

handleStatusBarTouches missing in FlutterViewController flutter/flutter#63051

Closed

cyanglaz mentioned this pull request Nov 16, 2022

Add examples on how to embed a Flutter screen into an existing SwiftUI app flutter/website#7828

Merged

3 tasks

cyanglaz pushed a commit to flutter/website that referenced this pull request Nov 22, 2022

Remove documentation about handleStatusBarTouches

f7c54ce

`handleStatusBarTouches` is removed in flutter/engine#16820, this part of the documentation is obsolete. Fixes flutter/flutter#63051 (comment)

cyanglaz mentioned this pull request Nov 22, 2022

Remove documentation about handleStatusBarTouches flutter/website#7861

Merged

3 tasks

Fixed the ability to scroll to the top on iOS 13 #16820

Fixed the ability to scroll to the top on iOS 13 #16820

Uh oh!

Conversation

gaaclarke commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kavantix Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Mar 3, 2020

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Mar 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gaaclarke commented Feb 26, 2020 •

edited

Loading

Kavantix Mar 4, 2020 •

edited

Loading