Fix Share Screen Crash on iPad #48220

LouiseHsu · 2023-11-20T11:30:14Z

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

hellohuanlin · 2023-11-20T19:07:29Z

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

+    activityViewController.popoverPresentationController.sourceView = engineViewController.view;
+    activityViewController.popoverPresentationController.sourceRect =
+        CGRectMake(CGRectGetWidth(engineViewController.view.bounds) / 2,
+                   CGRectGetHeight(engineViewController.view.bounds) / 2, 0, 0);


UIKit uses this rect to tell where the pop over should be presented. This should be the rect of the selected text

Yeah, but we would have to pass in the rect size/coords from the framework, and then add logic to make sure that the view is being rendered either above/below/next to the text so that it's not clipping off the screen depending on the position of the text and the current view bounds... which is totally doable but since this is a fatal crash I wanted to get it at least non-crashing as soon as possible

Never mind, looks like the positioning is automatically handled 👀 will update this PR

…m:louisehsu/engine into louisehsu/fix-share-screen-crashh-on-ipad

hellohuanlin · 2023-11-21T18:04:51Z

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

+    CGRect transformedRectLeft = [(FlutterTextInputView*)_textInputPlugin.textInputView
+        localRectFromFrameworkTransform:firstRect];
+    CGRect lastRect = [(FlutterTextInputView*)_textInputPlugin.textInputView
+        caretRectForPosition:(FlutterTextPosition*)range.end];


iirc range.end is exclusive.

hellohuanlin · 2023-11-21T18:05:36Z

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

+    UITextRange* range = _textInputPlugin.textInputView.selectedTextRange;
+
+    CGRect firstRect = [(FlutterTextInputView*)_textInputPlugin.textInputView
+        caretRectForPosition:(FlutterTextPosition*)range.start];


take a look at firstRectForRange which takes the range directly.

It doesnt, or at least not usably. After transforming it gives me the coordinates for the top right corner of the first character, which is right, but the size is super, super off.

can you add a note here that we can't use firstRectForRange API since it's current implementation doesn't always return the full rect of the range.

hellohuanlin · 2023-11-21T18:06:07Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h

 - (instancetype)initWithOwner:(FlutterTextInputPlugin*)textInputPlugin NS_DESIGNATED_INITIALIZER;

+- (CGRect)localRectFromFrameworkTransform:(CGRect)incomingRect;
+- (CGRect)caretRectForPosition:(UITextPosition*)position;


why are you exposing these APIs?

can you add a TODO here, explaining that these are exposed to be used for your feature, but we should consider moving your feature inside this plugin so we don't have to expose it.

can you file an issue to further investigate this?

ueman · 2023-11-21T18:07:01Z

Out of curiosity, what's the benefit of accessing the position of the selected text from the engine and not passing it from the framework to the engine?

hellohuanlin · 2023-11-21T18:09:57Z

@ueman The framework does all the positioning, and these values on the engine were passed from the framework in the first place. We don't want to pass the same information again.

hellohuanlin · 2023-11-21T22:06:53Z

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

+    CGRect lastRect = [(FlutterTextInputView*)_textInputPlugin.textInputView
+        caretRectForPosition:(FlutterTextPosition*)range.end];
+    CGRect transformedRectRight = [(FlutterTextInputView*)_textInputPlugin.textInputView
+        localRectFromFrameworkTransform:lastRect];


what does this localRectFromFrameworkTransform do?

you get global coordinates from local ones

Gotcha. The naming is a bit weird and sounds like the opposite.

hellohuanlin · 2023-11-21T22:07:53Z

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

+        CGRectMake(transformedRectLeft.origin.x, transformedRectLeft.origin.y,
+                   transformedRectRight.origin.x - transformedRectLeft.origin.x,
+                   transformedRectLeft.size.height);
+  }


this won't work for RTL language. possibly crash due to negative width

hmm, would it help if i just wrapped in abs() ? I dont know what rtl language is

abs() should work. but please try it out

RTL = right-to-left, e.g. arabic/hebrew

hellohuanlin · 2023-11-22T19:41:04Z

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

                                         applicationActivities:nil] autorelease];
+
+  if (UI_USER_INTERFACE_IDIOM() == UIUserInterfaceIdiomPad) {
+    // On iPad, the share screen is presented in a popover view, and requires a CGRect


requires a sourceView and a sourceRect

hellohuanlin · 2023-11-22T19:42:28Z

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

+    // not always return the full rect of the range.
+    CGRect firstRect = [(FlutterTextInputView*)_textInputPlugin.textInputView
+        caretRectForPosition:(FlutterTextPosition*)range.start];
+    CGRect transformedRectLeft = [(FlutterTextInputView*)_textInputPlugin.textInputView


can you avoid left/right since it's not correct for RTL languages?

…m:louisehsu/engine into louisehsu/fix-share-screen-crashh-on-ipad

…138954) flutter/engine@f331e0a...bec0dac 2023-11-23 [email protected] Fix not being able to hide iOS status bar via setEnabledSystemUIMode (flutter/engine#48271) 2023-11-22 [email protected] [web] Hook the new JS API to the FlutterViewManager (flutter/engine#48283) 2023-11-22 [email protected] Roll Skia from 3a79d7a618aa to 5606ef899116 (1 revision) (flutter/engine#48331) 2023-11-22 [email protected] Fix Share Screen Crash on iPad (flutter/engine#48220) 2023-11-22 [email protected] Delete unused/test only code from FML (flutter/engine#48327) 2023-11-22 [email protected] Roll Skia from 30ecaac60b47 to 3a79d7a618aa (1 revision) (flutter/engine#48328) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Fixes flutter/flutter#138550 ![Simulator Screenshot - iPad Air (5th generation) - 2023-11-21 at 03 33 37](https://github.com/flutter/engine/assets/36148254/15e10e43-816b-43b1-a5ab-75c8add90899)

Fixes flutter/flutter#138550 Original PR: #48220

…lutter#138954) flutter/engine@f331e0a...bec0dac 2023-11-23 [email protected] Fix not being able to hide iOS status bar via setEnabledSystemUIMode (flutter/engine#48271) 2023-11-22 [email protected] [web] Hook the new JS API to the FlutterViewManager (flutter/engine#48283) 2023-11-22 [email protected] Roll Skia from 3a79d7a618aa to 5606ef899116 (1 revision) (flutter/engine#48331) 2023-11-22 [email protected] Fix Share Screen Crash on iPad (flutter/engine#48220) 2023-11-22 [email protected] Delete unused/test only code from FML (flutter/engine#48327) 2023-11-22 [email protected] Roll Skia from 30ecaac60b47 to 3a79d7a618aa (1 revision) (flutter/engine#48328) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Fixes flutter/flutter#138550 Original PR: flutter/engine#48220

LouiseHsu added 2 commits November 20, 2023 03:27

initial commit

52a11ea

formatting

2d678fa

github-actions bot added the platform-ios label Nov 20, 2023

Merge branch 'main' into louisehsu/fix-share-screen-crashh-on-ipad

4a65fdd

LouiseHsu changed the title ~~Louisehsu/fix share screen crashh on ipad~~ Fix Share Screen Crash on iPad Nov 20, 2023

LouiseHsu requested a review from hellohuanlin November 20, 2023 17:31

LouiseHsu marked this pull request as ready for review November 20, 2023 17:31

hellohuanlin reviewed Nov 20, 2023

View reviewed changes

LouiseHsu added 2 commits November 21, 2023 03:26

add positioning for ipad share menu

3b171dc

Merge branch 'louisehsu/fix-share-screen-crashh-on-ipad' of github.co…

d7a55b5

…m:louisehsu/engine into louisehsu/fix-share-screen-crashh-on-ipad

hellohuanlin reviewed Nov 21, 2023

View reviewed changes

add todo

7e72f93

hellohuanlin reviewed Nov 21, 2023

View reviewed changes

LouiseHsu and others added 2 commits November 21, 2023 14:34

fix for RTL language, more TODOs

ae1880d

Merge branch 'main' into louisehsu/fix-share-screen-crashh-on-ipad

20a8f10

hellohuanlin reviewed Nov 22, 2023

View reviewed changes

hellohuanlin approved these changes Nov 22, 2023

View reviewed changes

LouiseHsu added 2 commits November 22, 2023 11:52

fix naming, update comments

12e23aa

Merge branch 'louisehsu/fix-share-screen-crashh-on-ipad' of github.co…

0017713

…m:louisehsu/engine into louisehsu/fix-share-screen-crashh-on-ipad

LouiseHsu added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023

auto-submit bot merged commit 2bd675c into flutter:main Nov 22, 2023

engine-flutter-autoroll mentioned this pull request Nov 22, 2023

Roll Flutter Engine from f331e0afaf2e to 5733891c8d86 (5 revisions) flutter/flutter#138922

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023

2bd675ccb Fix Share Screen Crash on iPad (flutter/engine#48220)

853f6d1

engine-flutter-autoroll mentioned this pull request Nov 23, 2023

Roll Flutter Engine from f331e0afaf2e to 5733891c8d86 (5 revisions) flutter/flutter#138927

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2023

2bd675ccb Fix Share Screen Crash on iPad (flutter/engine#48220)

70f64fa

engine-flutter-autoroll mentioned this pull request Nov 23, 2023

Roll Flutter Engine from f331e0afaf2e to 5733891c8d86 (5 revisions) flutter/flutter#138932

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2023

2bd675ccb Fix Share Screen Crash on iPad (flutter/engine#48220)

6bda178

engine-flutter-autoroll mentioned this pull request Nov 23, 2023

Roll Flutter Engine from f331e0afaf2e to bec0dacaae41 (6 revisions) flutter/flutter#138954

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2023

2bd675ccb Fix Share Screen Crash on iPad (flutter/engine#48220)

531ab4c

LouiseHsu mentioned this pull request Nov 27, 2023

[CP] Fix crash on choosing share in selection controls on ipad #48427

Merged

8 tasks

auto-submit bot pushed a commit that referenced this pull request Nov 30, 2023

[CP] Fix crash on choosing share in selection controls on ipad (#48427)

5a11866

Fixes flutter/flutter#138550 Original PR: #48220

DenisovAV pushed a commit to DenisovAV/flutter-tvos-engine that referenced this pull request May 5, 2024

[CP] Fix crash on choosing share in selection controls on ipad (#48427)

b4b9979

Fixes flutter/flutter#138550 Original PR: flutter/engine#48220

Fix Share Screen Crash on iPad #48220

Fix Share Screen Crash on iPad #48220

Uh oh!

Conversation

LouiseHsu commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

LouiseHsu Nov 21, 2023 • 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!

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!

ueman commented Nov 21, 2023

Uh oh!

hellohuanlin commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LouiseHsu commented Nov 20, 2023 •

edited

Loading

LouiseHsu Nov 21, 2023 •

edited

Loading

hellohuanlin commented Nov 21, 2023 •

edited

Loading