[ios] Fix hold and drag spacebar does not move cursor when obscureText is true. #40216

LinXunFeng · 2023-03-10T09:26:41Z

Fixes flutter/flutter#122139 with flutter pr flutter/flutter#122383

flutter-dashboard · 2023-03-10T09:26:45Z

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.

chinmaygarde · 2023-03-23T20:13:43Z

@cyanglaz Can you take a look at this or redirect?

justinmc · 2023-03-23T22:25:28Z

@cbracken You mentioned potentially using updateConfig outside of web recently in another PR, right? What did you decide there? This PR is adding support for it on iOS in order to fix a similar bug with obscureText.

Edit: This issue: flutter/flutter#122300

cyanglaz · 2023-03-24T18:39:01Z

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

+  [config setValue:@"NO" forKey:@"obscureText"];
+  [self updateConfig:config];
+
+  XCTAssertFalse(inputView.isSecureTextEntry);


Can we also test if [inputView reloadInputViews]; is called?

cyanglaz · 2023-03-24T18:42:00Z

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

+      // https://github.com/flutter/flutter/issues/122139
+      if (inputView.isSecureTextEntry != isSecureTextEntry) {
+        inputView.secureTextEntry = isSecureTextEntry;
+        [inputView reloadInputViews];


The document of reloadInputViews this method says:
You can use this method to refresh the custom input view or input accessory view associated with the current object when it’s the first responder. The views are replaced immediately, without animating them into place. If the current object isn’t the first responder, this method has no effect.
(https://developer.apple.com/documentation/uikit/uiresponder/1621110-reloadinputviews?language=objc)

"The views are replaced immediately" sounds like the some view inside the inputView is replaced? What are other side effects of replacing the view? Do we not need to restore any state of the view?

I didn't find any relevant side effect descriptions, and there was no problem after testing, so I think that the reloadInputViews method simply refreshes the keyboard view and does not cause other side effects. And when switching secureTextEntry to true, the keyboard will not have input accessory view, so there is no need to save the state.

In addition, I did a test in a native project, calling the reloadInputViews method directly will not affect the state in input accessory view and caps lock state.

reloadInputViews1.mov

cbracken · 2023-03-24T18:55:03Z

You mentioned potentially using updateConfig outside of web recently in another PR, right? What did you decide there?

@justinmc I have the patch sitting in a branch as well as a framework patch. The framework patch only looks at obscureText since it was just a quick test to verify the engine side, so I think we'd want to decide whether we want to have it send and update on all changes or just on obscureText to keep things simple initially.

Will send out the embedder side and we can discuss what we'd like to do for the framework end.

cyanglaz · 2023-03-27T15:25:39Z

@cbracken Does your work include or affect iOS? Do we need to hold off landing this for now?

chinmaygarde · 2023-04-06T20:10:56Z

@cbracken @cyanglaz Are we still making progress on this?

cyanglaz · 2023-04-06T20:16:46Z

Code looks good to me, but need to confirm with @cbracken on his other PR mentioned in #40216 (comment)

@justinmc Could you also provide some context? Maybe a link to the PR?

justinmc · 2023-04-06T23:53:11Z

Sorry, this is the referenced discussion about sending updateConfig for all configuration changes: flutter/flutter#122300 (comment)

It seems like we might want something like this PR for all platforms and all configuration parameters in the long term.

cyanglaz · 2023-04-07T16:28:38Z

@justinmc Ok. Sounds like we could land this to fix iOS for now and introduce a long term solution for all the platforms later.

cyanglaz

LGTM

auto-submit · 2023-04-07T16:30:03Z

auto label is removed for flutter/engine, pr: 40216, due to This PR has not met approval requirements for merging. You have project association NONE and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-04-07T16:30:04Z

auto label is removed for flutter/engine, pr: 40216, due to Validations Fail.

auto-submit · 2023-04-07T16:58:21Z

auto label is removed for flutter/engine, pr: 40216, due to This PR has not met approval requirements for merging. You have project association NONE and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-04-07T16:58:22Z

auto label is removed for flutter/engine, pr: 40216, due to Validations Fail.

LinXunFeng · 2023-04-07T17:44:32Z

I don't understand why Linux Framework Smoke Tests fails, i just merge branch flutter:main into LinXunFeng:main, how can i fix it?

cyanglaz · 2023-04-07T18:01:03Z

@LinXunFeng Linux Framework Smoke Tests is failing upstream on the latest commit. Not sure why luci-engine is not marked as red, it seems a bit concerning that luci-engine is not blocking the merge. cc @godofredoc

I'm not sure about why Mac Unopt timed out, I triggered a re-run.

godofredoc · 2023-04-07T19:15:33Z

@LinXunFeng Linux Framework Smoke Tests is failing upstream on the latest commit. Not sure why luci-engine is not marked as red, it seems a bit concerning that luci-engine is not blocking the merge. cc @godofredoc

I'm not sure about why Mac Unopt timed out, I triggered a re-run.

luci-engine is the current state of the engine tree with the signal being calculated from the last engine commit. Linux framework tests calculate the latest green commit from the framework to use for these tests. It can happen that the latest green commit from the framework does not work well with the engine ToT commit. This type of issue is usually identified when the engine is rolled to the framework, at that point a few reverts may happen in the engine and the pending PRs will need to get rebased to pick up the revert changes.

cyanglaz · 2023-04-07T19:19:16Z

I see
Looking at https://ci.chromium.org/p/flutter/g/engine/console, it feels like the latest engine commit is red without understanding the context of Linux Framework Smoke Tests

godofredoc · 2023-04-07T23:06:21Z

I see Looking at https://ci.chromium.org/p/flutter/g/engine/console, it feels like the latest engine commit is red without understanding the context of Linux Framework Smoke Tests

@cyanglaz the milo dashboardd was deprecated a long time ago. The current source of truth is https://flutter-dashboard.appspot.com/#/build?taskFilter&authorFilter&messageFilter&hashFilter&showMac=true&showWindows=true&showiOS=true&showLinux=true&showAndroid=true&showStaging=false&repo=engine&branch=master

cyanglaz · 2023-04-08T00:17:43Z

@LinXunFeng The mac test failure also does not seem to be related. It is failing on an impeller unit test. Can you reproduce it locally?


Failed Command:

/Volumes/Work/s/w/ir/cache/builder/src/out/host_debug_unopt/impeller_unittests --gtest_repeat=2 --gtest_shuffle

Exit Code: -11

LinXunFeng · 2023-04-08T00:39:55Z

@LinXunFeng The mac test failure also does not seem to be related. It is failing on an impeller unit test. Can you reproduce it locally?
Failed Command:

/Volumes/Work/s/w/ir/cache/builder/src/out/host_debug_unopt/impeller_unittests --gtest_repeat=2 --gtest_shuffle

Exit Code: -11

yes, the error log is as follows

➜  host_debug_unopt git:(f24f62f) ✗ ./impeller_unittests --gtest_repeat=2 --gtest_shuffle
[INFO:test_timeout_listener.cc(76)] Test timeout of 120 seconds per test case will be enforced.

Repeating all tests (iteration 1) . . .

Note: Randomizing tests' orders with a seed of 32449 .
[==========] Running 698 tests from 22 test suites.
[----------] Global test environment set-up.
[----------] 1 test from PipelineDescriptorTest
[ RUN      ] PipelineDescriptorTest.PrimitiveTypeHashEquality
[       OK ] PipelineDescriptorTest.PrimitiveTypeHashEquality (1 ms)
[----------] 1 test from PipelineDescriptorTest (1 ms total)

[----------] 8 tests from Compute/ComputeTest
[ RUN      ] Compute/ComputeTest.HeartCubicsToStrokeVertices/Metal
[ERROR:flutter/fml/backtrace.cc(108)] Caught signal SIGSEGV during program execution.
Frame 0: 0x107255446 testing::internal::EqHelper::Compare<>()
Frame 1: 0x107254577 impeller::testing::ComputeTest_HeartCubicsToStrokeVertices_Test::TestBody()::$_0::operator()()
Frame 2: 0x1072543fd std::_LIBCPP_ABI_NAMESPACE::__invoke[abi:v15000]<>()
Frame 3: 0x10725435b std::_LIBCPP_ABI_NAMESPACE::__invoke_void_return_wrapper<>::__call<>()
Frame 4: 0x107254303 std::_LIBCPP_ABI_NAMESPACE::__function::__alloc_func<>::operator()[abi:v15000]()
Frame 5: 0x1072522cf std::_LIBCPP_ABI_NAMESPACE::__function::__func<>::operator()()
Frame 6: 0x109636dcf std::_LIBCPP_ABI_NAMESPACE::__function::__value_func<>::operator()[abi:v15000]()
Frame 7: 0x10963271a std::_LIBCPP_ABI_NAMESPACE::function<>::operator()()
Frame 8: 0x1097d5f3b ___ZN8impeller16CommandBufferMTL16OnSubmitCommandsENSt21_LIBCPP_ABI_NAMESPACE8functionIFvNS_13CommandBuffer6StatusEEEE_block_invoke
Frame 9: 0x7ff8192b5380 MTLDispatchListApply
Frame 10: 0x7ff8192b58a6 -[_MTLCommandBuffer didCompleteWithStartTime:endTime:error:]
Frame 11: 0x7ff8192b5646 -[MTLIOAccelCommandBuffer didCompleteWithStartTime:endTime:error:]
Frame 12: 0x7ff8192b5520 -[_MTLCommandQueue commandBufferDidComplete:startTime:completionTime:error:]
Frame 13: 0x7ff81928d0d0 ioAccelCommandQueueBlockFenceCallback
Frame 14: 0x7ff812e19981 IODispatchCalloutFromCFMessage
Frame 15: 0x7ff812e19802 _IODispatchCalloutWithDispatch
Frame 16: 0x7ff80fc10f40 dispatch_mig_server
Frame 17: 0x7ff80fbf5a44 _dispatch_client_callout
Frame 18: 0x7ff80fbf8500 _dispatch_continuation_pop
Frame 19: 0x7ff80fc09dff _dispatch_source_invoke
Frame 20: 0x7ff80fbfb964 _dispatch_lane_serial_drain
Frame 21: 0x7ff80fbfc5b4 _dispatch_lane_invoke
Frame 22: 0x7ff80fc06ad7 _dispatch_workloop_worker_thread
Frame 23: 0x7ff80fd71ce3 _pthread_wqthread
Frame 24: 0x7ff80fd70c67 start_wqthread

[1]    18218 segmentation fault  ./impeller_unittests --gtest_repeat=2 --gtest_shuffle

…t is true (#122139)

LinXunFeng · 2023-04-09T03:39:51Z

@cyanglaz I rebase changes onto flutter:main, and now only the Linux linux_web_engine fails to run, it seems to fail because of network problem, please trigger a re-run.

auto-submit · 2023-04-09T20:13:38Z

auto label is removed for flutter/engine, pr: 40216, due to This PR has not met approval requirements for merging. You have project association NONE and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-04-09T20:13:39Z

auto label is removed for flutter/engine, pr: 40216, due to Validations Fail.

hellohuanlin · 2023-04-12T18:26:22Z

CC @moffatman this may be related to the floating cursor PR you have

hellohuanlin · 2023-04-12T18:34:31Z

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

+      // The feature of holding and draging spacebar to move cursor is affected by
+      // secureTextEntry, so when obscureText is updated, we need to update secureTextEntry
+      // and call reloadInputViews.
+      // https://github.com/flutter/flutter/issues/122139


not sure if i follow this. can you explain more why setting secrueTextEntry and the reload fixes the space bar cursor interaction? Or is it an undocumented behavior by apple here?

First of all, it is clear that when secureTextEntry is true, the space bar cursor interaction is not allowed, otherwise it is allowed. This is the correct interaction behavior on iOS native, so we need to update secureTextEntry in time when switching obscureText. I did not find a description of whether this space bar cursor interaction is affected by secureTextEntry in Apple's official documentation, this is just the conclusion obtained after testing.

Then, when the value of the secureTextEntry is changed, the interaction and style of the keyboard are still before the secureTextEntry is changed, so here I call the reloadInputViews method to refresh keyboard.

LinXunFeng · 2023-04-21T00:34:11Z

@cyanglaz Hi, both have approved, please add autosubmit label again, thank you very much. : )

justinmc · 2023-04-25T16:31:18Z

We also need a secondary review on the framework PR (flutter/flutter#122383) if anyone has a minute.

…obscureText is true. (flutter/engine#40216)

…125504) flutter/engine@60c4e61...0e23698 2023-04-25 linxunfeng@yeah.net [ios] Fix hold and drag spacebar does not move cursor when obscureText is true. (flutter/engine#40216) 2023-04-25 skia-flutter-autoroll@skia.org Roll Dart SDK from 337365e42613 to 170137fc759b (2 revisions) (flutter/engine#41483) 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 jsimmons@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

#122383) Fixes #122139 with engine pr [flutter/engine#40216](flutter/engine#40216)

flutter-dashboard bot added platform-ios needs tests labels Mar 10, 2023

LinXunFeng mentioned this pull request Mar 10, 2023

[ios] fix hold and drag spacebar does not move cursor when obscureTex… flutter/flutter#122383

Merged

chinmaygarde changed the title ~~[ios] fix hold and drag spacebar does not move cursor when obscureTex…~~ [ios] fix hold and drag spacebar does not move cursor when obscureText is true. Mar 23, 2023

chinmaygarde changed the title ~~[ios] fix hold and drag spacebar does not move cursor when obscureText is true.~~ [ios] Fix hold and drag spacebar does not move cursor when obscureText is true. Mar 23, 2023

cyanglaz reviewed Mar 24, 2023

View reviewed changes

cyanglaz approved these changes Apr 7, 2023

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2023

cyanglaz added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Apr 7, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2023

LinXunFeng added 3 commits April 9, 2023 10:56

[ios] fix hold and drag spacebar does not move cursor when obscureTex…

fe4ac52

…t is true (#122139)

add test

ca8094b

supplementary test to judge whether reloadInputViews method is called

8bd7030

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2023

cyanglaz requested a review from hellohuanlin April 9, 2023 20:14

hellohuanlin reviewed Apr 12, 2023

View reviewed changes

hellohuanlin approved these changes Apr 19, 2023

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 25, 2023

auto-submit bot merged commit 0e23698 into flutter:main Apr 25, 2023

engine-flutter-autoroll mentioned this pull request Apr 25, 2023

Roll Flutter Engine from 60c4e61416cd to 0e236982086b (2 revisions) flutter/flutter#125504

Merged

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

0e2369820 [ios] Fix hold and drag spacebar does not move cursor when …

816e297

…obscureText is true. (flutter/engine#40216)

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 6, 2023

[ios] fix hold and drag spacebar does not move cursor when obscureTex… (

7c356d0

#122383) Fixes #122139 with engine pr [flutter/engine#40216](flutter/engine#40216)

justinmc mentioned this pull request May 8, 2023

Update keyboard view when keyboardType has changed. flutter/flutter#126040

Closed

[ios] Fix hold and drag spacebar does not move cursor when obscureText is true. #40216

[ios] Fix hold and drag spacebar does not move cursor when obscureText is true. #40216

Uh oh!

Conversation

LinXunFeng commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Mar 10, 2023

Uh oh!

chinmaygarde commented Mar 23, 2023

Uh oh!

justinmc commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

LinXunFeng Mar 25, 2023

Choose a reason for hiding this comment

Uh oh!

cyanglaz Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

LinXunFeng Mar 25, 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!

cbracken commented Mar 24, 2023

Uh oh!

cyanglaz commented Mar 27, 2023

Uh oh!

chinmaygarde commented Apr 6, 2023

Uh oh!

cyanglaz commented Apr 6, 2023

Uh oh!

justinmc commented Apr 6, 2023

Uh oh!

cyanglaz commented Apr 7, 2023

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Apr 7, 2023

Uh oh!

auto-submit bot commented Apr 7, 2023

Uh oh!

auto-submit bot commented Apr 7, 2023

Uh oh!

auto-submit bot commented Apr 7, 2023

Uh oh!

LinXunFeng commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz commented Apr 7, 2023

Uh oh!

godofredoc commented Apr 7, 2023

Uh oh!

cyanglaz commented Apr 7, 2023

Uh oh!

godofredoc commented Apr 7, 2023

Uh oh!

cyanglaz commented Apr 8, 2023

Uh oh!

LinXunFeng commented Apr 8, 2023

Uh oh!

LinXunFeng commented Apr 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auto-submit bot commented Apr 9, 2023

Uh oh!

auto-submit bot commented Apr 9, 2023

Uh oh!

hellohuanlin commented Apr 12, 2023

Uh oh!

hellohuanlin Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

LinXunFeng Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

LinXunFeng commented Mar 10, 2023 •

edited

Loading

justinmc commented Mar 23, 2023 •

edited

Loading

LinXunFeng Mar 25, 2023 •

edited

Loading

LinXunFeng commented Apr 7, 2023 •

edited

Loading

LinXunFeng commented Apr 9, 2023 •

edited

Loading

LinXunFeng Apr 13, 2023 •

edited

Loading