fix: TapTextFieldTapRegion not working on ios safari/chrome/webview

[Satsrag]

For this issue and this

engine/lib/web_ui/lib/src/engine/text_editing/text_editing.dart

Line 1710 in 47b15df

if (windowHasFocus && isFastCallback) {

There should be use || instead of &&. The fourth comment above, if isFastCallback will remain the focus, so we call activeDomElement.focus().

If it is not fast callback >= 200ms and windowHasFocus is true, we also call activeDomElement.focus() to remain focused to wait for the framework to manage the focus change. In this case, if the user taps the TapTextFieldTapRegion, the active element should not lose focus. If we use &&, in this case, the engine sends TextConnectionClose to the framework to lose focus. So, the framework has no chance to manage the focus.

[flutter-dashboard]

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

[Renzo-Olivares]

Given the original introduction of isFastCallback here #31718 , it is a bit strange that this code does not cause any test to fail.

From the explanation given here #31718 (comment) it sounds like we want to close the input connection when the "done" button is pressed on a keyboard. The logic without isFastCallback was only checking if the window had focus before refocusing the textfield. In the case of the "done" button this would cause the keyboard to stay open because the window has focus so the textfield is refocused. With windowHasFocus && isFastCallback, pressing the "done" button will close the input connection because it was not a fast callback. With windowHasFocus || isFastCallback it sounds like the "done" button would once again fall under the same issue where the keyboard can't hide because the window has focus. What do you think about this?

cc @mdebbar if you had any context on this issue.

[Satsrag]

I didn't know this before. So we should find a way to detect the difference between clicking the done or TapTextFieldTapRegion.

[Satsrag]

Added test for this. However, it seems to the test platform don't work on ios safari.

[vasilich6107]

Hi @Renzo-Olivares @Satsrag
Is there any chance to merge this PR?

[Renzo-Olivares]

@Satsrag Apologies for the long silence on this PR. Are you still interested in moving forward with this PR? If so, this PR will need to be re-opened in https://github.com/flutter/flutter.

If I understand correctly the proposed solution is to add a new timer that detects touches on the active window. If there was a touch before the blur callback was triggered then we refocus the input field and let the framework manage the focus. If there is no touch before the blur callback was triggered then we close the input connection.

This is my understanding of the issue:

Blur Trigger	Behavior before fast callback fix
Safari right after input focus	Input field should be refocused to give framework a chance to handle focus
User taps on other focusable element	Input field should be refocused to give framework a chance to handle focus
Tab or browser backgrounded	Input connection is closed
Pressing "done"	Input field should be refocused to give framework a chance to handle focus (bug because keyboard can't hide)

Pressing "done" on the iOS keyboard triggers a blur event, but it is indistinguishable from other blur events. So before the fast callback fix was implemented it would always fall under the case of refocusing the input field which would prevent the keyboard from hiding. The fix was to implement a check for a "fast callback", which differentiates the blur event sent by Safari right after input, and the blur event sent when "done" is pressed, the former being the "fast callback" case. I think the issue with the "fast callback" fix is that now when the user taps on another focusable element, it is considered not a "fast callback" and as a result causes the input connection to close.

Blur Trigger	Expected Behavior
Safari right after input focus	Input field should be refocused to give framework a chance to handle focus
User taps on other focusable element	Input field should be refocused to give framework a chance to handle focus
Tab or browser backgrounded	Input connection is closed
Pressing "done"	Input connection is closed

Blur Trigger	Current Behavior
Safari right after input focus	Input field should be refocused to give framework a chance to handle focus
User taps on other focusable element	Input connection is closed
Tab or browser backgrounded	Input connection is closed
Pressing "done"	Input connection is closed

Now with your proposal we are adding another level of differentiation by detecting taps on the window right before the blur event. I think this is reasonable though I wonder if we should also check for mouse clicks and other pointer devices that aren't covered in touchstart.

[jtmcdole]

Monorepo Migration Completed

TL;DR: Please migrate your PR to flutter/flutter

The flutter/engine repository has been migrated to the flutter/flutter repository. This PR will no longer be landed here and must be migrated. To migrate your PR to the flutter repository, please follow these steps:

1. Create a patch for this PR:

   • Generate a patch set that represents the changes in this PR. The exact method will vary depending on your PR's history. If your PR includes merges, it is highly recommended that you rebase it onto main first.

   git format-patch $START_COMMIT..$END_COMMIT --stdout > combined_patch.patch

2. Update the patch for the new engine location:

   • The engine source code now resides in the engine/ subdirectory within the flutter/flutter repository. You'll need to update the file paths in your patch accordingly.

   # Insert the `engine/` prefix to all paths. Note that this usage works on macOS and
   # linux versions of sed.
   sed -i.bak \
   -e 's|^\(diff --git a/\)\(.*\) b/\(.*\)|\1engine/\2 b/engine/src/flutter/\3|' \
   -e 's|^\(--- a/\)\(.*\)|\1engine/src/flutter/\2|' \
   -e 's|^\(\+\+\+ b/\)\(.*\)|\1engine/src/flutter/\2|' \
   combined_patch.patch

3. Checkout and set up your Flutter development environment:

   • Follow the instructions in Setting up the Framework development environment to check out the flutter/flutter repository.
   • Remember to rename the origin remote to upstream (e.g., git remote rename origin upstream).

4. Set up the engine development environment within the monorepo:

   • Follow the updated instructions in Setting up the engine environment, paying close attention to the changes in gclient setup and location.

5. Apply the patch to a new branch:

   • Create a new branch in your flutter/flutter repository.
   • Apply the updated patch to this branch:

   git apply combined_patch.patch

6. Open a new PR:

   • Open a new PR in the flutter/flutter repository with your changes.
   • Reference this original PR in the new PR's description using flutter/engine#<PR_NUMBER>.

---

This is a canned message

[Satsrag]

@Satsrag Apologies for the long silence on this PR. Are you still interested in moving forward with this PR? If so, this PR will need to be re-opened in https://github.com/flutter/flutter.

If I understand correctly the proposed solution is to add a new timer that detects touches on the active window. If there was a touch before the blur callback was triggered then we refocus the input field and let the framework manage the focus. If there is no touch before the blur callback was triggered then we close the input connection.

This is my understanding of the issue:

Blur Trigger Behavior before fast callback fix
Safari right after input focus Input field should be refocused to give framework a chance to handle focus
User taps on other focusable element Input field should be refocused to give framework a chance to handle focus
Tab or browser backgrounded Input connection is closed
Pressing "done" Input field should be refocused to give framework a chance to handle focus (bug because keyboard can't hide)
Pressing "done" on the iOS keyboard triggers a blur event, but it is indistinguishable from other blur events. So before the fast callback fix was implemented it would always fall under the case of refocusing the input field which would prevent the keyboard from hiding. The fix was to implement a check for a "fast callback", which differentiates the blur event sent by Safari right after input, and the blur event sent when "done" is pressed, the former being the "fast callback" case. I think the issue with the "fast callback" fix is that now when the user taps on another focusable element, it is considered not a "fast callback" and as a result causes the input connection to close.

Blur Trigger Expected Behavior
Safari right after input focus Input field should be refocused to give framework a chance to handle focus
User taps on other focusable element Input field should be refocused to give framework a chance to handle focus
Tab or browser backgrounded Input connection is closed
Pressing "done" Input connection is closed
Blur Trigger Current Behavior
Safari right after input focus Input field should be refocused to give framework a chance to handle focus
User taps on other focusable element Input connection is closed
Tab or browser backgrounded Input connection is closed
Pressing "done" Input connection is closed
Now with your proposal we are adding another level of differentiation by detecting taps on the window right before the blur event. I think this is reasonable though I wonder if we should also check for mouse clicks and other pointer devices that aren't covered in touchstart.

@Renzo-Olivares You are right. Other events that aren't covered in touchstart should be considered. First, I need some time to understand what events need to be considered. Then I re-open a PR in https://github.com/flutter/flutter. Thank you.