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

htoor3 · 2023-05-09T19:58:03Z

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 PhysicalX / PhysicalY are incorrect in web engine when clicking on top of flt-text-editing-host flutter#125948 (text editing)
Fixes PointerInterceptor reverses transformHitTests in a scaled context flutter#126661 (platform view scaling)
Fixes [web] flutter 3.10.0 transforming HtmlElementView malforms hittest behaviour flutter#126754

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 Hixie said 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.

htoor3 · 2023-05-09T20:26:33Z

lib/web_ui/lib/src/engine/pointer_binding/event_position_helper.dart

+ui.Offset _computeOffsetForInputs(DomMouseEvent event) {
+  final Float32List matrix = textEditing.strategy.geometry!.globalTransform;
+  final FastMatrix32 fastMatrix = FastMatrix32(matrix);
+  fastMatrix.transform(event.offsetX, event.offsetY);


@yjbanov, fastMatrix.transform works pretty well for this use case (scaling and translation). Are there any limitations to this? Scaling and translation are probably the most commonly applied transforms to inputs, but this still work for rotation and skew, or is that something we need to account for?

If this is performance-critical, I'd first check if the matrix is within the limits that FastMatrix32 can deal with and only use it then, and fallback to full matrix otherwise. I thinktransformKindOf should give you the right info.

Why not https://developer.mozilla.org/en-US/docs/Web/API/DOMMatrixReadOnly / DOMMatrix?

@ditman DOMMatrixReadOnly and its instance method transformPoint could certainly work, although it's missing documentation on MDN. But the API seems well supported across different browsers. We can also init it with globalTransform which should be a 16 length array. Do you think there would be significant advantages to using the browser API? Maybe their transformPoint is more optimized?

@yjbanov I don't see any docs outlining the limitations of the FastMatrix32, do you mean it only support matrices that perform simple 2D transforms and Matrix4 should be used for 3D transforms?

(@yjbanov mentioned offline that DOMMatrix might not be fully supported everywhere, so I guess we're sticking to our own implementation!)

mdebbar

Thanks for putting a fix together!

lib/web_ui/lib/src/engine/pointer_binding/event_position_helper.dart

mdebbar · 2023-05-09T20:37:37Z

lib/web_ui/lib/src/engine/pointer_binding/event_position_helper.dart

+  final FastMatrix32 fastMatrix = FastMatrix32(matrix);
+  fastMatrix.transform(event.offsetX, event.offsetY);


@yjbanov is this the right API to use for transforming an offset?

I think we want to support the general case. See my comment above for more details.

htoor3 · 2023-05-16T19:29:58Z

lib/web_ui/lib/src/engine/pointer_binding/event_position_helper.dart

+  assert(targetElement == domElement, 'The targeted input element must be the active input element');
+  final Float32List transformValues = inputGeometry.globalTransform;
+  assert(transformValues.length == 16);
+  final Matrix4 transform = Matrix4(


I replaced FastMatrix32 with Matrix4 @yjbanov

cc @ditman

Since the transform is applied once per DOM event no extra optimization is necessary. The default transform should be fast enough.

yjbanov

LGTM if LGT @ditman

flutter-dashboard · 2023-05-17T16:22:18Z

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.

lib/web_ui/lib/src/engine/pointer_binding/event_position_helper.dart

ditman

Can you give this a quick test with this with this app:

https://github.com/flutter/samples/tree/main/web_embedding/element_embedding_demo

And see if the input is still working as expected when the glassPane is being transformed too? (for example when the app is rotated a few degrees!)

Other than that, this is similar to what we discussed. That globalTransform matrix looks HANDY! 🙏

htoor3 · 2023-05-18T17:46:56Z

Can you give this a quick test with this with this app:

https://github.com/flutter/samples/tree/main/web_embedding/element_embedding_demo

And see if the input is still working as expected when the glassPane is being transformed too? (for example when the app is rotated a few degrees!)

Other than that, this is similar to what we discussed. That globalTransform matrix looks HANDY! 🙏

Just tested with your demo and added an input and it works well. I actually noticed an issue on the demo (without these changes) where if the embedded app is rotated at a steep angle, clicking anywhere in a focused input blurs it. This also seems to be fixed with these changes!

…iews.

ditman · 2023-05-18T23:01:32Z

I've tested the fix against a bunch of test apps (most of them linked above), and it seems to improve things. Currently writing a test. (We should cherry-pick this into 3.10 stable @yjbanov @kevmoo)

Add TODO in the place where we need to compute 3D transforms.

yjbanov

mdebbar

LGTM. Thanks @htoor3 and @ditman for fixing this!

* 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

…put nodes). (flutter/engine#41870)

…127233) flutter/engine@3267fa2...f0c02ae 2023-05-19 [email protected] Roll Fuchsia Linux SDK from eal6a4HeaQon_Y4ml... to TWjmvLCOnYAUgAzvT... (flutter/engine#42168) 2023-05-19 [email protected] [macOS] Extract PlatformView mutator clip updating (flutter/engine#42164) 2023-05-19 [email protected] [web] Fix event offset for transformed widgets (and text input nodes). (flutter/engine#41870) 2023-05-19 [email protected] Roll goldctl to f808dcff91b221ae313e540c09d79696cd08b8de (flutter/engine#42166) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from eal6a4HeaQon to TWjmvLCOnYAU 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] 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

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

…lutter#127233) flutter/engine@3267fa2...f0c02ae 2023-05-19 [email protected] Roll Fuchsia Linux SDK from eal6a4HeaQon_Y4ml... to TWjmvLCOnYAUgAzvT... (flutter/engine#42168) 2023-05-19 [email protected] [macOS] Extract PlatformView mutator clip updating (flutter/engine#42164) 2023-05-19 [email protected] [web] Fix event offset for transformed widgets (and text input nodes). (flutter/engine#41870) 2023-05-19 [email protected] Roll goldctl to f808dcff91b221ae313e540c09d79696cd08b8de (flutter/engine#42166) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from eal6a4HeaQon to TWjmvLCOnYAU 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] 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

htoor3 added 2 commits May 9, 2023 14:55

Add helper to transform event offsets for inputs

e110952

Whitespace

4eb22bc

flutter-dashboard bot added the platform-web Code specifically for the web engine label May 9, 2023

htoor3 requested review from ditman, mdebbar and yjbanov May 9, 2023 20:20

htoor3 commented May 9, 2023

View reviewed changes

mdebbar reviewed May 9, 2023

View reviewed changes

htoor3 added 3 commits May 9, 2023 16:04

Refactor

fd73f9d

Add assertion to make sure target element is the active input element

844d2a3

Replace FastMatrix with Matrix4

04db2b5

htoor3 requested a review from mdebbar May 16, 2023 19:26

htoor3 commented May 16, 2023

View reviewed changes

htoor3 marked this pull request as ready for review May 16, 2023 20:14

yjbanov approved these changes May 16, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into scaled-inputs-fix-2

fb059f5

flutter-dashboard bot added the needs tests label May 17, 2023

htoor3 mentioned this pull request May 17, 2023

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

Closed

2 tasks

mdebbar approved these changes May 17, 2023

View reviewed changes

lib/web_ui/lib/src/engine/pointer_binding/event_position_helper.dart Outdated Show resolved Hide resolved

Simplify Matrix4 creation

da3005b

ditman approved these changes May 18, 2023

View reviewed changes

yjbanov mentioned this pull request May 18, 2023

[web] flutter 3.10.0 transforming HtmlElementView malforms hittest behaviour flutter/flutter#126754

Closed

2 tasks

Use viewport coordinates to compute mouse event offsets on platform v…

e01e764

…iews.

ditman mentioned this pull request May 18, 2023

[Web] [Version 3.10.0] Tappable area of ElevatedButton and GestureDetector is displaced, if it is inside Stack and Align flutter/flutter#126946

Closed

2 tasks

Add unit test for computeEventOffset.

4cd15e1

Add TODO in the place where we need to compute 3D transforms.

ditman requested review from mdebbar and yjbanov May 19, 2023 00:03

ditman changed the title ~~[web] - add helper to calculate offsets for transformed inputs~~ [web] Fix event offset for transformed widgets (and text input nodes). May 19, 2023

yjbanov approved these changes May 19, 2023

View reviewed changes

mdebbar approved these changes May 19, 2023

View reviewed changes

mdebbar added 2 commits May 19, 2023 10:08

Merge branch 'main' into scaled-inputs-fix-2

d4c0840

perspectiveTransform no longer takes a Vector3

19f144a

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label May 19, 2023

auto-submit bot merged commit 4f3f7bb into flutter:main May 19, 2023

ditman mentioned this pull request May 19, 2023

[CP][web] Fix event offset on transformed widgets. #42171

Merged

8 tasks

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter Engine from 3267fa29491a to f0c02aee69db (4 revisions) flutter/flutter#127233

Merged

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

4f3f7bb2e [web] Fix event offset for transformed widgets (and text in…

9873570

…put nodes). (flutter/engine#41870)

		final FastMatrix32 fastMatrix = FastMatrix32(matrix);
		fastMatrix.transform(event.offsetX, event.offsetY);

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

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

Uh oh!

Conversation

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

Uh oh!

Issues

Pre-launch Checklist

Uh oh!

htoor3 May 9, 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!

yjbanov May 9, 2023

Choose a reason for hiding this comment

Uh oh!

ditman May 10, 2023

Choose a reason for hiding this comment

Uh oh!

htoor3 May 10, 2023

Choose a reason for hiding this comment

Uh oh!

ditman May 18, 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!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdebbar May 9, 2023

Choose a reason for hiding this comment

Uh oh!

yjbanov May 9, 2023

Choose a reason for hiding this comment

Uh oh!

htoor3 May 16, 2023

Choose a reason for hiding this comment

Uh oh!

yjbanov May 16, 2023

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented May 17, 2023

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

htoor3 commented May 18, 2023

Uh oh!

ditman commented May 18, 2023

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htoor3 commented May 9, 2023 •

edited

Loading

htoor3 May 9, 2023 •

edited

Loading

ditman May 18, 2023 •

edited

Loading