Don't respond to the `insertionPointColor` selector on iOS 17+ #46373

LongCatIsLooong · 2023-09-28T17:42:11Z

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.

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

hellohuanlin · 2023-09-28T19:49:42Z

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

 // the screen.
+//
+// These are not documened methods. On iOS 17, the insertion point color is also
+// used to paint the highlighted background of the selected IME candidate:


what else do we use this function for?

The paragraph above says that it's used to make sure the system caret / selection handles aren't visible.

stuartmorgan-g · 2023-09-28T19:51:33Z

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

 // because Scribble interactions require the view to have it's actual frame on
 // the screen.
+//
+// These are not documened methods. On iOS 17, the insertion point color is also


Nit: documented

stuartmorgan-g · 2023-09-28T19:54:09Z

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

+// used to paint the highlighted background of the selected IME candidate:
+// https://github.com/flutter/flutter/issues/132548
+// So the respondsToSelector method is overridden to return NO for this method
+// on iOS 17+.


I'm confused as to why we are both implementing it but claiming not to. Are we relying on an implementation detail of some system code calling this unconditionally and other code checking whether its implemented first, and relying on that distinction to make things work? If so, that seems incredibly fragile, and the sort of thing that's likely to cause another round of fire drills in a subsequent iOS version.

UIKit falls back to UIColor.insertionPointColor if the receiver doesn't respond to insertionPointColor. That color is unfortunately private.

some system code calling this unconditionally

The method is actually private (but it's in the codebase for a while and no complains from app store) and it's not part of any protocols, so UIKit has to check respondsToSelector first.

The method is actually private (but it's in the codebase for a while and no complains from app store) and it's not part of any protocols, so UIKit has to check respondsToSelector first.

Then how does this change not break the "Prevent UIKit from showing selection handles or highlights" use case that the implementation is documented as being for?

On iOS 17 there's new API for adding system UI for text selection/cursor/highlights: https://developer.apple.com/documentation/uikit/uitextselectiondisplayinteraction?language=objc . Without adding that interaction to the input view (and we don't add UIITextInteraction to the view unless the framework is currently sending editing state updates, as a hack to fix a different bug), UIKit doesn't seem to add selection handles or highlights automatically on iOS 17.

I see; please update the comments on this method then to explain this, and that it's bypassed on iOS 17+ by a respondsToSelector: check.

Hi @fbcouch do you remember when is insertionPointColor called during scribble? I'd like to verify if this is still needed on iOS17.

If I recall correctly, it was basically during any text interaction – since we changed to show the frame in it's proper place on the screen, iPadOS was able to draw it's own selection handles, etc

stuartmorgan-g · 2023-09-28T19:54:41Z

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

+      return NO;
+    }
+  }
+  return [FlutterTextInputView instancesRespondToSelector:selector];


Why isn't this just calling super?

According to the documentation [super respondsToSelector:] is equivalent to [self respondsToSelector:] https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418583-respondstoselector#discussion, so i thought there was going to be an infinite loop. Let me try it out.

Well that worked. Changed to calling super.

The context there is important:

You cannot test whether an object inherits a method from its superclass by sending respondsToSelector: to the object using the super keyword. This method will still be testing the object as a whole, not just the superclass’s implementation. Therefore, sending respondsToSelector: to super is equivalent to sending it to self.

For the purposes of testing where a method's implementation lives, it's equivalent. That doesn't mean it doesn't follow actual method dispatch rules.

hellohuanlin · 2023-10-02T22:46:25Z

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

+  if (@available(iOS 17.0, *)) {
+    // See the comment on this method.
+    if (selector == @selector(insertionPointColor)) {
+      return NO;


We need to be careful if this breaks other functionalities that uses insertionPointColor.

LongCatIsLooong · 2023-10-12T18:05:32Z

I did some basic IME input testing and didn't notice any UI weirdness.

stuartmorgan-g

LGTM as long as we're reasonably confident that we've manually tested all of the cases that the code being bypassed was originally intended to fix.

…17+ (flutter/engine#46373)

…136752) flutter/engine@659e68a...289f29b 2023-10-17 [email protected] Don't respond to the `insertionPointColor` selector on iOS 17+ (flutter/engine#46373) 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#132548 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-ios label Sep 28, 2023

LongCatIsLooong force-pushed the fix-first-IME-candidate-invisible branch from 35ce1ca to 8ec60f5 Compare September 28, 2023 17:54

Don't respond to the insertionPointColor selector on iOS 17+

05f8e8a

LongCatIsLooong force-pushed the fix-first-IME-candidate-invisible branch from 8ec60f5 to 05f8e8a Compare September 28, 2023 18:09

fix test

30c875c

LongCatIsLooong force-pushed the fix-first-IME-candidate-invisible branch from ed47e43 to 900069b Compare September 28, 2023 18:54

LongCatIsLooong requested review from cyanglaz, hellohuanlin and stuartmorgan-g and removed request for hellohuanlin and stuartmorgan-g September 28, 2023 19:33

hellohuanlin reviewed Sep 28, 2023

View reviewed changes

stuartmorgan-g reviewed Sep 28, 2023

View reviewed changes

comments

2f0c506

LongCatIsLooong force-pushed the fix-first-IME-candidate-invisible branch from 900069b to 2f0c506 Compare September 28, 2023 20:01

LongCatIsLooong added 2 commits September 28, 2023 13:44

review

de9e532

review

3f0981d

hellohuanlin reviewed Oct 2, 2023

View reviewed changes

LongCatIsLooong requested review from hellohuanlin and stuartmorgan-g October 12, 2023 18:05

hellohuanlin approved these changes Oct 13, 2023

View reviewed changes

stuartmorgan-g approved these changes Oct 13, 2023

View reviewed changes

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2023

auto-submit bot merged commit 289f29b into flutter:main Oct 17, 2023

LongCatIsLooong deleted the fix-first-IME-candidate-invisible branch October 17, 2023 19:21

engine-flutter-autoroll mentioned this pull request Oct 17, 2023

Roll Flutter Engine from 659e68a097b5 to 289f29b1ad00 (1 revision) flutter/flutter#136752

Merged

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

289f29b1a Don't respond to the insertionPointColor selector on iOS …

c7e7a6c

…17+ (flutter/engine#46373)

Don't respond to the insertionPointColor selector on iOS 17+ #46373

Don't respond to the insertionPointColor selector on iOS 17+ #46373

Uh oh!

Conversation

LongCatIsLooong commented Sep 28, 2023

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!

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!

LongCatIsLooong commented Oct 12, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Don't respond to the `insertionPointColor` selector on iOS 17+ #46373

Don't respond to the `insertionPointColor` selector on iOS 17+ #46373