[iOS] [iPad] fix avoid bottom inset in split view mode #35535

RichardFevrier · 2022-08-19T10:42:35Z

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 Hixie said 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-08-19T10:42:37Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

cyanglaz · 2022-08-24T22:13:29Z

Looks like the bug wasn't what I thought in flutter/flutter#109845 (comment).

@LongCatIsLooong @justinmc Are you the right people to review this?

@RichardFevrier, tests need to be added in order for this PR to land.

justinmc

Happy to review it! I'm no expert on the iOS embedder but this seems reasonable to me. +1 that this needs tests.

I don't know why we used keyboardWillBeHidden in the first place instead of keyboardWillChangeFrame. Seems like this should all still work though, but me or someone else should probably try this out locally and confirm that nothing seems broken.

justinmc · 2022-08-24T23:56:23Z

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

@@ -1190,12 +1185,19 @@ - (void)updateViewportPadding {

 #pragma mark - Keyboard events

+// using keyboardWillChangeFrame instead of keyboardWill(Show/Hide) because of split/floating
+// keyboard on iPad


Capital letter at the beginning and period at the end. Also maybe link to the issue.

I'll fix that this week 👍

justinmc · 2022-08-24T23:57:49Z

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


-  // Ignore keyboard notifications related to other apps.
+  // Ignore keyboard notifications related to other apps if not dismissing.


What happens if you show the keyboard in the other app, then tap the Flutter app's text field?

I haven't tried it, just curious.

It works normally,

the keyboard appear on the other app and the other app is resized

the textfield in the flutter app becomes the first responder so the flutter app is resized at that moment.

I can make a video if necessary

justinmc · 2022-08-24T23:59:24Z

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

-        [[info objectForKey:UIKeyboardAnimationDurationUserInfoKey] doubleValue];
-    [self startKeyBoardAnimation:duration];
-  }
+  // Get the animation duration


Nit: Period at the end (I know you didn't write this comment, but it's a good chance to fix it! 😁).

I'll fix that this week 👍

cyanglaz

Can we also make sure this change doesn't regress flutter/flutter#99951

cyanglaz · 2022-08-25T17:23:01Z

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

@@ -1190,12 +1185,19 @@ - (void)updateViewportPadding {

 #pragma mark - Keyboard events

+// using keyboardWillChangeFrame instead of keyboardWill(Show/Hide) because of split/floating


The comment says "keybaordWill(show/hide), but we don't have a "keyboardWillShow", maybe just "keyboardWillBeHidden"?

keyboardWillShow/keyboardWillHide are part of the UIKit framework; so keyboardWillBeHidden is not appropriated here and my comment is still relevant 🙂
BTW I tried to answer to you in my opened issue last week but I could not without closing the issue.
(nothing related but I loved the document you wrote related to blurs... so interesting to read !)

cyanglaz · 2022-08-25T17:25:46Z

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


-  // Ignore keyboard notifications related to other apps.
+  // Ignore keyboard notifications related to other apps if not dismissing.
+  bool isDismissing =


Maybe:

bool isShowing = CGRectIntersection(keyboardFrame, screenRect);

Good point, I'll fix that this week 👍

cyanglaz · 2022-08-25T17:35:12Z

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

  id isLocal = info[UIKeyboardIsLocalUserInfoKey];
-  if (isLocal && ![isLocal boolValue]) {
+  if (!isDismissing && isLocal && ![isLocal boolValue]) {


Have you tested If the keyboard is brought be by the other app, does the Flutter app's bottom inset adjust? This logic here seems to prevent Flutter app's bottom inset to be adjusted to the correct value in that case. Which is what I was saying in: flutter/flutter#109845 (comment)

As I wrote to @justinmc, if the keyboard is brought by an other app:

the keyboard appear on the other app and the other app is resized

(if you tap in the textfield on the flutter app) the textfield in the flutter app becomes the first responder so the flutter app is resized at that moment.

This is the correct behavior since natives apps have that one.

cyanglaz · 2022-08-25T17:37:42Z

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

-  // Get the animation duration
-  NSTimeInterval duration =
-      [[info objectForKey:UIKeyboardAnimationDurationUserInfoKey] doubleValue];
-
  // Considering the iPad's split keyboard, Flutter needs to check if the keyboard frame is present
  // in the screen to see if the keyboard is visible.
  if (CGRectIntersectsRect(keyboardFrame, screenRect)) {


This line can be changed to if (isShowing) { if you take my suggestion above.

I'll fix that this week 👍

RichardFevrier · 2022-08-31T13:01:48Z

I don't know why we used keyboardWillBeHidden in the first place instead of keyboardWillChangeFrame. Seems like this should all still work though, but me or someone else should probably try this out locally and confirm that nothing seems broken.

Worse than that @justinmc, it calls twice the animation via startKeyBoardAnimation and can probably be related with flutter/flutter/issues/109435

Also something that I wanted to say the week before without closing the issue is:
I do not know why the CI failed at testEnsureBottomInsetIsZeroWhenKeyboardDismissed since this PR can only improve that...

Can we also make sure this change doesn't regress flutter/flutter#99951

I can try that @cyanglaz on my iPad but is there not a Unit test to check it automatically?

And last point, I am okay to make a Unit Test to avoid regressions on that point, but is it possible to write Unit test for split view apps with your framework?

Thanks.

luckysmg · 2022-09-01T15:25:25Z

Maybe just delete the code below can fix this?

// Delete this code in keyboardWillBeHidden
  id isLocal = info[UIKeyboardIsLocalUserInfoKey];
  if (isLocal && ![isLocal boolValue]) {
    return;
  }

luckysmg · 2022-09-07T01:52:08Z

Updated: keyboardWillChangeFrame will include show/hide/changeFrame so maybe keyboardHidden seems to be redundant.

So this look seems better, and we just need to make sure that the
bool isDismissing = CGRectEqualToRect(keyboardFrame, CGRectZero) || keyboardFrame.origin.y >= screenRect.size.height; or
bool isShowing = CGRectIntersectsRect(keyboardFrame, screenRect);
is accurate to judge whether the keyboard is open or close currently😄.

jmagman · 2022-09-14T21:40:27Z

@RichardFevrier were you able to figure out how to write the split view mode integration tests? Also it looks like @luckysmg had some suggestions.
@cyanglaz was this ready for re-review or are you waiting for the unit tests?

cyanglaz · 2022-09-14T22:03:26Z

I can try that @cyanglaz on my iPad but is there not a Unit test to check it automatically?

We do have unit-tests cover it but not integration test. It would be nice to at least do a manual test to make sure it is not regressed. If it is, then we will need to add better tests for flutter/flutter#99951

I think we can land this PR when 1. Unit-tests (or even better integration tests) are added, 2. When we did a manual test that flutter/flutter#99951 is not regressed.

chinmaygarde · 2022-09-22T20:10:38Z

It seems like this is still WIP. Adding the labels. If OTOH no progress is being made, perhaps we can close this?

flutter-dashboard · 2022-10-12T20:13:14Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

jmagman · 2022-12-15T18:52:58Z

@RichardFevrier Thank you for your contribution. This PR has been superseded by #37719 cc @vashworth. If you're still seeing issues related to split view mode please file new issues and we'll take a look!

[iOS] [iPad] fix avoid bottom inset in split view mode

f0da837

flutter-dashboard bot added the platform-ios label Aug 19, 2022

flutter-dashboard bot added the needs tests label Aug 19, 2022

jmagman requested a review from cyanglaz August 24, 2022 21:03

justinmc reviewed Aug 25, 2022

View reviewed changes

cyanglaz suggested changes Aug 25, 2022

View reviewed changes

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Sep 22, 2022

jmagman requested a review from vashworth November 9, 2022 21:14

jmagman closed this Dec 15, 2022


		// Ignore keyboard notifications related to other apps.
		// Ignore keyboard notifications related to other apps if not dismissing.

		@@ -1190,12 +1185,19 @@ - (void)updateViewportPadding {

		#pragma mark - Keyboard events

		// using keyboardWillChangeFrame instead of keyboardWill(Show/Hide) because of split/floating

[iOS] [iPad] fix avoid bottom inset in split view mode #35535

[iOS] [iPad] fix avoid bottom inset in split view mode #35535

Uh oh!

Conversation

RichardFevrier commented Aug 19, 2022

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 19, 2022

Uh oh!

cyanglaz commented Aug 24, 2022

Uh oh!

justinmc 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!

cyanglaz 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!

RichardFevrier commented Aug 31, 2022

Uh oh!

luckysmg commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luckysmg commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented Sep 14, 2022

Uh oh!

cyanglaz commented Sep 14, 2022

Uh oh!

chinmaygarde commented Sep 22, 2022

Uh oh!

flutter-dashboard bot commented Oct 12, 2022

Uh oh!

jmagman commented Dec 15, 2022

Uh oh!

Uh oh!

luckysmg commented Sep 1, 2022 •

edited

Loading

luckysmg commented Sep 7, 2022 •

edited

Loading