Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web - Fix selection jump on Chrome for Android #41202

Merged

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Apr 14, 2023

Description

This PR fixes cursor jump on Chrome for Android when the user taps in a multiline TextField.

Using the following code sample:

Code sample
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Text Field Focus',
      home: MyCustomForm(),
    );
  }
}

// Define a custom Form widget.
class MyCustomForm extends StatelessWidget {
  const MyCustomForm({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Text Field Focus'),
      ),
      backgroundColor: Colors.amber,
      body: Padding(
        padding: const EdgeInsets.all(16.0),
        child: TextField(
          decoration: const InputDecoration(
            fillColor: Colors.white,
            filled: true
          ),
          autofocus: true,
          maxLines: 3,
          controller: TextEditingController(text: '1\n2\n3\n4\n'),
        ),
      ),// This trailing comma makes auto-formatting nicer for build methods.
    );
  }
}

On a mobile browser, once the page is loaded, tap after the number 3:

  • Before this PR: the TextField content is automaticaly scrolled and the selection is set after number 1.
Enregistrement.de.l.ecran.2023-04-14.a.15.03.16.mov

Implementation

A multiline TextField relies on an HTML <textarea> elements. When a tap occurs the selection should be updated from Flutter not by the HTML element itself.
This PR prevents mouse events on Chrome for Android. Those events conflicts with Flutter selection changes.
Previously, mouse events were only prevented on desktop but they are also emitted on mobile, see https://bugs.chromium.org/p/chromium/issues/detail?id=119216#c11.

Related Issue

Related to flutter/flutter#124483 (partial fix because the issue is also reproducible on iOS/Safari).

Tests

Adds 1 test.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 14, 2023
@bleroux bleroux marked this pull request as draft April 14, 2023 12:59
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@bleroux bleroux force-pushed the fix_selection_jump_on_mobile_browsers branch 3 times, most recently from 0d0446a to 540e5e5 Compare April 14, 2023 13:44
@bleroux bleroux changed the title Web - Fix selection jump on mobile browsers Web - Fix selection jump on Chrome for Android Apr 14, 2023
@bleroux bleroux marked this pull request as ready for review April 14, 2023 14:03
@bleroux bleroux requested a review from mdebbar April 14, 2023 14:08
@bleroux bleroux force-pushed the fix_selection_jump_on_mobile_browsers branch from 540e5e5 to 79f64a3 Compare April 14, 2023 14:23
@bleroux bleroux force-pushed the fix_selection_jump_on_mobile_browsers branch from 79f64a3 to 96a3dac Compare April 14, 2023 17:20
@kevmoo
Copy link
Contributor

kevmoo commented Apr 14, 2023

Thanks for doing the tweaks here!

@bleroux bleroux requested a review from kevmoo April 28, 2023 20:56
@bleroux
Copy link
Contributor Author

bleroux commented Apr 28, 2023

@mdebbar @kevmoo If you have some bandwidth, thank you to review this PR.

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me, thanks for the contribution!

But I don't think it solves the linked issue. It only hides the symptom. Can this be done in IOSTextEditingStrategy too?

lib/web_ui/test/engine/text_editing_test.dart Outdated Show resolved Hide resolved
@bleroux bleroux force-pushed the fix_selection_jump_on_mobile_browsers branch from 96a3dac to 025423d Compare May 2, 2023 14:48
@bleroux
Copy link
Contributor Author

bleroux commented May 2, 2023

@mdebbar Thank you for the review!

But I don't think it solves the linked issue. It only hides the symptom.

Do you have in mind another way to fix this? Because it might help to fix the iOS side.

Can this be done in IOSTextEditingStrategy too?

This solution is not applicable on the iOS side because IOSTextEditingStrategy relies on DOM clickevent to tackle tap/longPress behaviors:

/// On iOS long press works differently than a single tap.
///
/// On a normal tap the virtual keyboard comes up and users can enter text
/// using the keyboard.
///
/// The long press on the other hand focuses on the element without bringing
/// up the virtual keyboard. It allows the users to modify the field by using
/// copy/cut/select/paste etc.
///
/// After a long press [domElement] is positioned to the correct place. If the
/// user later single-tap on the [domElement] the virtual keyboard will come
/// and might shift the page up.
///
/// In order to prevent this shift, on a `click` event the position of the
/// element is again set somewhere outside of the page and
/// [_positionInputElementTimer] timer is restarted. The element will be
/// placed to its correct position after [_delayBeforePlacement].
void _addTapListener() {
subscriptions.add(DomSubscription(activeDomElement, 'click', (_) {
// Check if the element is already positioned. If not this does not fall
// under `The user was using the long press, now they want to enter text
// via keyboard` journey.
if (_canPosition) {
// Re-place the element somewhere outside of the screen.
initializeElementPlacement();
// Re-configure the timer to place the element.
_schedulePlacement();
}
}));
}

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label May 2, 2023
@auto-submit auto-submit bot merged commit c4a2712 into flutter:main May 2, 2023
29 checks passed
@bleroux bleroux deleted the fix_selection_jump_on_mobile_browsers branch May 2, 2023 19:35
@mdebbar
Copy link
Contributor

mdebbar commented May 2, 2023

Do you have in mind another way to fix this? Because it might help to fix the iOS side.

Well, that's a tricky issue that we've had for a while (see flutter/flutter#99918 and flutter/flutter#70841 for example). The only solution for it is to overhaul our text editing system (discussed in flutter/flutter#120613).

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 2, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 2, 2023
…125909)

flutter/engine@cd989fb...c4a2712

2023-05-02 leroux_bruno@yahoo.fr Web - Fix selection jump on Chrome for Android (flutter/engine#41202)
2023-05-02 skia-flutter-autoroll@skia.org Roll Dart SDK from 84f3080c3165 to ea1fce8e0aa7 (1 revision) (flutter/engine#41672)
2023-05-02 magder@google.com Stop specifiying Macmini8,1 in ci builders, use inherited mac_model dimension (flutter/engine#41223)

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 jimgraham@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
3 participants