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

PhysicalX / PhysicalY are incorrect in web engine when clicking on top of flt-text-editing-host #125948

Closed
2 tasks done
jezell opened this issue May 3, 2023 · 11 comments · Fixed by flutter/engine#41870
Closed
2 tasks done
Assignees
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list platform-web Web applications specifically r: fixed Issue is closed as already fixed in a newer version

Comments

@jezell
Copy link

jezell commented May 3, 2023

Is there an existing issue for this?

Use case

  1. Add a widget to the screen that has text editing. Position it somewhere so that the left and top edges are in the middle of the screen.
  2. Set a breakpoint in PointerEventConverter on case ui.PointerChange.down
  3. Without the textbox selected, click inside the text control and note the physicalX
  4. Click in the same spot and note that the physicalX is much different.

The engine seems to assume that the flutter view was the source of all events, so when it hits flt-text-editing-host it delivers incorrect pointer event data.

Proposal

Perhaps the web engine should make sure that the event was actually on the canvas host element it thinks it was on, and if not, convert the coordinates when converting the DomEvent to PointerData.

@jezell
Copy link
Author

jezell commented May 3, 2023

A temporary hack for anyone encountering this is that you can set pointer-events:none to flt-text-editing-host, but maybe that has undesirable effects in some cases (not popping keyboard on mobile browsers?).

@darshankawar darshankawar added the in triage Presently being triaged by the triage team label May 3, 2023
@darshankawar
Copy link
Member

Thanks for the report @jezell

Use case

  1. Add a widget to the screen that has text editing. Position it somewhere so that the left and top edges are in the middle of the screen.
  2. Set a breakpoint in PointerEventConverter on case ui.PointerChange.down
  3. Without the textbox selected, click inside the text control and note the physicalX
  4. Click in the same spot and note that the physicalX is much different.

Can you provide us a minimal complete code sample for this ?

@darshankawar darshankawar added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label May 3, 2023
@jezell
Copy link
Author

jezell commented May 3, 2023

@darshankawar yeah, I'll try to put one together. Was seeing it inside our app with unreliable selection when zoomed and took a bit to track down why. I think I should be able to put something together that illustrates the problem now that I know the cause.

@github-actions github-actions bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label May 3, 2023
@jezell
Copy link
Author

jezell commented May 3, 2023

This sample illustrates the problem. Click twice over the textbox, the first click will hit the canvas. The second click should hit the text editing host. Note that it prints widely different coordinates for the local positionX despite hitting the same position:

import 'package:flutter/material.dart';
import 'package:flutter/gestures.dart';

void main() {
  runApp(const MyApp());
}

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

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});
  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    final scale = Matrix4.identity();
    scale.translate(-100, -100);
    scale.scale(2);

    return Material(
        child: Stack(
           
            children: [
          Transform(
              alignment: Alignment.topLeft,
              transform: scale,
              child: Stack(children: [
                Positioned(
                    top: 100,
                    left: 100,
                    child: SizedBox(
                        width: 4000,
                        height: 4000,
                        child: ClipRect(
                            child: Listener(
                                onPointerDown: (event) =>
                                    print(event.localPosition),
                                child: TextFormField(
                                    initialValue:
                                        'XXXXXX ${scale.getMaxScaleOnAxis()}')))))
              ]))
        ]));
  }
}

@darshankawar
Copy link
Member

Thanks for the update. I ran it on web and observed that upon first two clicks in the textfield, the offset values are different:

A Dart VM Service on Chrome is available at: http://127.0.0.1:50892/ttEsqW0bftY=
The Flutter DevTools debugger and profiler on Chrome is available at: http://127.0.0.1:9100?uri=http://127.0.0.1:50892/ttEsqW0bftY=
Offset(187.3, 29.5)
Offset(93.6, 20.7)

Then subsequent clicks shows correct offsets as below:

Offset(44.2, 19.7)
Offset(44.2, 19.7)
Offset(44.1, 18.9)
Offset(44.1, 18.9)
stable, master flutter doctor -v
[!] Flutter (Channel stable, 3.7.12, on macOS 12.2.1 21D62 darwin-x64, locale
    en-GB)
    • Flutter version 3.7.12 on channel stable at
      /Users/dhs/documents/fluttersdk/flutter
    ! Warning: `flutter` on your path resolves to
      /Users/dhs/Documents/Fluttersdk/flutter/bin/flutter, which is not inside
      your current Flutter SDK checkout at
      /Users/dhs/documents/fluttersdk/flutter. Consider adding
      /Users/dhs/documents/fluttersdk/flutter/bin to the front of your path.
    ! Warning: `dart` on your path resolves to
      /Users/dhs/Documents/Fluttersdk/flutter/bin/dart, which is not inside your
      current Flutter SDK checkout at /Users/dhs/documents/fluttersdk/flutter.
      Consider adding /Users/dhs/documents/fluttersdk/flutter/bin to the front
      of your path.
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 4d9e56e694 (3 days ago), 2023-04-17 21:47:46 -0400
    • Engine revision 1a65d409c7
    • Dart version 2.19.6
    • DevTools version 2.20.1
    • If those were intentional, you can disregard the above warnings; however
      it is recommended to use "git" directly to perform update checks and
      upgrades.

[!] Xcode - develop for iOS and macOS (Xcode 12.3)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    ! Flutter recommends a minimum Xcode version of 13.
      Download the latest version or update via the Mac App Store.
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] VS Code (version 1.62.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.21.0

[✓] Connected device (5 available)
    • SM G975F (mobile)       • RZ8M802WY0X • android-arm64   • Android 11 (API 30)
    • Darshan's iphone (mobile)  • 21150b119064aecc249dfcfe05e259197461ce23 •
      ios            • iOS 14.4.1 18D61
    • iPhone 12 Pro Max (mobile) • A5473606-0213-4FD8-BA16-553433949729     •
      ios            • com.apple.CoreSimulator.SimRuntime.iOS-14-3 (simulator)
    • macOS (desktop)            • macos                                    •
      darwin-x64     • Mac OS X 10.15.4 19E2269 darwin-x64
    • Chrome (web)               • chrome                                   •
      web-javascript • Google Chrome 98.0.4758.80

[✓] HTTP Host Availability
    • All required HTTP hosts are available

! Doctor found issues in 1 category.

[!] Flutter (Channel master, 3.11.0-1.0.pre.44, on macOS 12.2.1 21D62
    darwin-x64, locale en-GB)
    • Flutter version 3.11.0-1.0.pre.44 on channel master at
      /Users/dhs/documents/fluttersdk/flutter
    ! Warning: `flutter` on your path resolves to
      /Users/dhs/Documents/Fluttersdk/flutter/bin/flutter, which is not inside
      your current Flutter SDK checkout at
      /Users/dhs/documents/fluttersdk/flutter. Consider adding
      /Users/dhs/documents/fluttersdk/flutter/bin to the front of your path.
    ! Warning: `dart` on your path resolves to
      /Users/dhs/Documents/Fluttersdk/flutter/bin/dart, which is not inside your
      current Flutter SDK checkout at /Users/dhs/documents/fluttersdk/flutter.
      Consider adding /Users/dhs/documents/fluttersdk/flutter/bin to the front
      of your path.
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision ebf3ea1755 (2 hours ago), 2023-05-03 23:25:08 -0400
    • Engine revision da45a21708
    • Dart version 3.1.0 (build 3.1.0-72.0.dev)
    • DevTools version 2.23.1
    • If those were intentional, you can disregard the above warnings; however
      it is recommended to use "git" directly to perform update checks and
      upgrades.
      
[!] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    • Android SDK at /Users/dhs/Library/Android/sdk
    ✗ cmdline-tools component is missing
      Run `path/to/sdkmanager --install "cmdline-tools;latest"`
      See https://developer.android.com/studio/command-line for more details.
    ✗ Android license status unknown.
      Run `flutter doctor --android-licenses` to accept the SDK licenses.
      See https://flutter.dev/docs/get-started/install/macos#android-setup for
      more details.

[✓] Xcode - develop for iOS and macOS (Xcode 13.2.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 13C100
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] IntelliJ IDEA Ultimate Edition (version 2021.3.2)
    • IntelliJ at /Applications/IntelliJ IDEA.app
    • Flutter plugin version 65.1.4
    • Dart plugin version 213.7228

[✓] VS Code (version 1.62.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.29.0

[✓] Connected device (3 available)
    • Darshan's iphone (mobile) • 21150b119064aecc249dfcfe05e259197461ce23 • ios
      • iOS 15.3.1 19D52
    • macOS (desktop)           • macos                                    •
      darwin-x64     • macOS 12.2.1 21D62 darwin-x64
    • Chrome (web)              • chrome                                   •
      web-javascript • Google Chrome 109.0.5414.119

[✓] Network resources
    • All expected network resources are available.

! Doctor found issues in 1 category.
      
[!] Xcode - develop for iOS and macOS (Xcode 12.3)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    ! Flutter recommends a minimum Xcode version of 13.
      Download the latest version or update via the Mac App Store.
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] VS Code (version 1.62.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.21.0

[✓] Connected device (5 available)
    • SM G975F (mobile)       • RZ8M802WY0X • android-arm64   • Android 11 (API 30)
    • Darshan's iphone (mobile)  • 21150b119064aecc249dfcfe05e259197461ce23 •
      ios            • iOS 14.4.1 18D61
    • iPhone 12 Pro Max (mobile) • A5473606-0213-4FD8-BA16-553433949729     •
      ios            • com.apple.CoreSimulator.SimRuntime.iOS-14-3 (simulator)
    • macOS (desktop)            • macos                                    •
      darwin-x64     • Mac OS X 10.15.4 19E2269 darwin-x64
    • Chrome (web)               • chrome                                   •
      web-javascript • Google Chrome 98.0.4758.80

[✓] HTTP Host Availability
    • All required HTTP hosts are available

! Doctor found issues in 1 category.



@darshankawar darshankawar added c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. platform-web Web applications specifically c: proposal A detailed proposal for a change to Flutter and removed in triage Presently being triaged by the triage team labels May 4, 2023
@jezell
Copy link
Author

jezell commented May 4, 2023

@darshankawar yes, that is because from then on you are clicking on the text host unless it loses focus. All of those values after the first are incorrect coordinates. Only the first one is the true coordinate.

@yjbanov yjbanov added the P1 High-priority issues at the top of the work list label May 4, 2023
@yjbanov
Copy link
Contributor

yjbanov commented May 4, 2023

Tentatively making it a P3, as it may actually be a regression either due to custom element embedding or the top-level DOM restructure to fix form autofill.

@htoor3
Copy link
Contributor

htoor3 commented May 5, 2023

Investigated this issue a bit and found a couple of things:

  • It's a regression due to the DOM structure change.
  • Seems to be related to how we're computing the offsets on the engine computeEventOffsetToTarget
  • In the test app, the correct offset and the calculated offset appear to be off by a factor of the scale transformation applied
  • Since text nodes have moved outside of the shadowDOM, we compute their offsets using the platform view offset logic _computeOffsetRelativeToActualTarget. This function doesn't support computing offsets for transformed elements (see [web] Teach pointer_binding how to understand 3D transformed apps. #117091), which I suspect is the issue.
  • The reason this seems to be fixed with setting pointer-events: none on the text editing host node is because in that case, we don't enter the platform view offset conditional because the target and actualTarget are the same flutterViewElement. So it ends up using ui.Offset(event.offsetX, event.offsetY) which works with transforms of the parent element.

I'm curious as to what side effects might occur by setting pointer-events: none on the parent element and letting the compute logic fall through to the case that supports transforms. Main thing I would suspect is that the pointer clicks would register inaccurately, but basic testing shows that the offsets still calculate correctly. Some quick testing shows that keyboard still pops up as well.

I can do some more extensive testing to see if that's a viable solution. We also might be able to get away with just skipping the platform offset calculation when target is input. The other option would be support transforms for the platform view offset calculations (see above linked issue) and leave the text editing host as is.

Edit: pointer-events: none isn't a viable solution because it breaks functionality when the element doesn't receive click events (e.g. context menu). flutter/engine#41870 is attempting to fix this via using the transform matrix to transform the received pointer event.

@htoor3 htoor3 self-assigned this May 15, 2023
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 19, 2023
#41870)

Text inputs have moved outside of the shadowDOM and are now using the pointer event offset calculation algorithm that platform views use.  However, transforms (e.g. scaling) applied to the input element aren't currently accounted for, which leads to incorrect offsets and clicks being registered inaccurately.

This PR attempts to transform those offset coordinates using the transform matrix data that is included in the geometry information sent over to `text_editing.dart` from the framework.

## Issues

* Fixes flutter/flutter#125948 (text editing)
* Fixes flutter/flutter#126661 (platform view scaling)
* Fixes flutter/flutter#126754

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
ditman pushed a commit to ditman/flutter-engine that referenced this issue May 19, 2023
* git cherry-pick 4f3f7bb (+ conflict fixes)

This cherry-pick backports flutter#41870 to Flutter 3.10.

Original commit message below:

[web] Fix event offset for transformed widgets (and text input nodes). (flutter#41870)

Text inputs have moved outside of the shadowDOM and are now using the pointer event offset calculation algorithm that platform views use.  However, transforms (e.g. scaling) applied to the input element aren't currently accounted for, which leads to incorrect offsets and clicks being registered inaccurately.

This PR attempts to transform those offset coordinates using the transform matrix data that is included in the geometry information sent over to `text_editing.dart` from the framework.

* Fixes flutter/flutter#125948 (text editing)
* Fixes flutter/flutter#126661 (platform view scaling)
* Fixes flutter/flutter#126754
ditman pushed a commit to ditman/flutter-engine that referenced this issue May 19, 2023
* git cherry-pick 4f3f7bb (+ conflict fixes)

This cherry-pick backports flutter#41870 to Flutter 3.10.

Original commit message below:

[web] Fix event offset for transformed widgets (and text input nodes). (flutter#41870)

Text inputs have moved outside of the shadowDOM and are now using the pointer event offset calculation algorithm that platform views use.  However, transforms (e.g. scaling) applied to the input element aren't currently accounted for, which leads to incorrect offsets and clicks being registered inaccurately.

This PR attempts to transform those offset coordinates using the transform matrix data that is included in the geometry information sent over to `text_editing.dart` from the framework.

* Fixes flutter/flutter#125948 (text editing)
* Fixes flutter/flutter#126661 (platform view scaling)
* Fixes flutter/flutter#126754
@ditman
Copy link
Member

ditman commented May 19, 2023

A fix is coming to Flutter master soon: #127233 (we're also attempting to cherry-pick so it becomes available to everybody on stable (3.10)

@darshankawar darshankawar added the r: fixed Issue is closed as already fixed in a newer version label May 22, 2023
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 24, 2023
This cherry-pick backports #41870 to Flutter 3.10.

* git cherry-pick 4f3f7bb (+ conflict fixes)

Original commit message below:

[web] Fix event offset for transformed widgets (and text input nodes). (#41870)

Text inputs have moved outside of the shadowDOM and are now using the pointer event offset calculation algorithm that platform views use.  However, transforms (e.g. scaling) applied to the input element aren't currently accounted for, which leads to incorrect offsets and clicks being registered inaccurately.

This PR attempts to transform those offset coordinates using the transform matrix data that is included in the geometry information sent over to `text_editing.dart` from the framework.

* Fixes flutter/flutter#125948 (text editing)
* Fixes flutter/flutter#126661 (platform view scaling)
* Fixes flutter/flutter#126754

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@ditman
Copy link
Member

ditman commented May 24, 2023

A hotfix to alleviate this issue has been released on stable, version: 3.10.2 (See full changelog).

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list platform-web Web applications specifically r: fixed Issue is closed as already fixed in a newer version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants