Send PointerSignalKind.scale events from web by moffatman · Pull Request #36348 · flutter-team-archive/engine

moffatman · 2022-09-22T13:26:45Z

Send PointerSignalKind.scale instead of PointerSignalKind.scroll if ctrl key is pressed on wheel event.

Part of flutter/flutter#112103

Sequence

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.

justinmc

I want to make sure this is customizable at the app-level if possible, otherwise looks fine to me.

CC @mdebbar Another one that could use a backup review from a web person, thank you!

justinmc · 2022-09-26T23:31:32Z

-      scrollDeltaX: deltaX,
-      scrollDeltaY: deltaY,
-    );
+    if (event.ctrlKey) {


What happens on Mac with ctrl and cmd?

ctrlKey is just how the browser sends scroll events, for legacy reasons. It's still called ctrlKey on the mac. In my mac Chrome, neither seem to cause a zoom if i hold them and scroll.

Wheel events with ctrlKey on Mac do not zoom/scale. Instead they just scroll. But with this PR, the scrolling will stop, right?

My understanding is that ctrl (not cmd) and scroll will continue to do the same thing (scroll) after this PR, is that right @moffatman?

justinmc · 2022-09-26T23:32:44Z

-      scrollDeltaY: deltaY,
-    );
+    if (event.ctrlKey) {
+      _pointerDataConverter.convert(


It would be nice if we could decide what combination of key + scroll does what at the framework level, in case some developers have different opinions about that. Is that something that we could make possible? I get that we need to prevent the browser from zooming in at the engine level though.

justinmc · 2022-09-26T23:32:57Z

+    }
+    else {


Should be } else {.

moffatman · 2022-09-27T00:41:44Z

To clarify, right now we are ignoring these ctrl-zoom events altogether. So this isn't causing things that would scroll before to now cause zooms.

mdebbar · 2022-09-27T17:19:49Z

@moffatman you are right that we ignore ctrl-zoom now, but that's not the case on Mac/iOS.

moffatman · 2022-09-27T19:33:57Z

@mdebbar So on macOS, when using ctrl-scroll wheel, what's the expected behavior?

mdebbar · 2022-10-06T17:57:14Z

@moffatman I believe on macOS, it should just scroll even if ctrl is pressed.

moffatman · 2022-10-06T18:02:01Z

@mdebbar I noticed sites which offer trackpad gestures (Ex: Figma) do not handle that properly, they will zoom-in if holding ctrl and scrolling. I guess the only option is to watch document onkey events and have a parallel tracking of the real ctrlKey. Do you think that's worth doing?

mdebbar · 2022-10-06T18:28:14Z

@moffatman I think, by default, we should do what the platform does (in this case, browsers on macOS scroll even if ctrl is pressed).

Apps can customize this behavior if they want to. I'm not sure if they can do such customization today with Flutter or not. Thoughts @justinmc ?

justinmc · 2022-10-06T19:47:45Z

Right now I don't believe there's a straightforward way to override a keyboard shortcut and a mouse gesture combined.

@Renzo-Olivares would that be possible with your upcoming gesture shortcuts work?

skia-gold · 2022-10-20T06:05:14Z

Gold has detected about 8 new digest(s) on patchset 3.
View them at https://flutter-engine-gold.skia.org/cl/github/36348

Piinks · 2022-11-22T00:12:42Z

Right now I don't believe there's a straightforward way to override a keyboard shortcut and a mouse gesture combined.

I asked @gspencergoog about this recently, I was looking into shift+scroll to flip the scroll direction. He recommended not tying it into shortcuts, but just checking the keyboard when a scroll event comes in. So I have a proposal out that would add that key modifier on the inherited ScrollBehavior. Maybe something similar would suit here for ctrl?
flutter/flutter#115610

goderbauer · 2023-03-07T23:51:38Z

@moffatman @justinmc @mdebbar @Piinks Are there still plans for this PR? It is getting a little stale...

moffatman · 2023-03-08T00:23:04Z

Remaining issues

Is there something that should be done here for future compatibility with framework-defined modifier keys
- I'm not so sure here, it doesn't make sense as the events are different types
Using the mouse wheel while holding ctrl is not supposed to zoom on macOS, just scroll.
- macOS scroll+zooms and trackpad pinches will both have event.ctrlKey, so we can't tell them apart from the event alone. Will need to add some sort of tracking of key-presses in order to tell if it's a true ctrlKey or not.

To start with, I will fix the merge conflicts at least..

justinmc

LGTM 👍

I'm on board with moving forward with this PR as-is. It seems to me like we're matching browser behavior as close as practical right now. If we do decide that we want to support scaling with other keys, we'll have to rethink our event signalKinds and maybe come up with some combined event that includes both scrolling and scaling. That's probably beyond the scope of this PR.

justinmc · 2023-03-16T20:39:23Z

-      scrollDeltaX: deltaX,
-      scrollDeltaY: deltaY,
-    );
+    if (event.ctrlKey) {


My understanding is that ctrl (not cmd) and scroll will continue to do the same thing (scroll) after this PR, is that right @moffatman?

moffatman · 2023-03-16T21:06:07Z

I still need to add some sort of tracking for the ctrl key press. Since right now it will zoom instead of scroll on macOS safari.

moffatman · 2023-03-17T04:29:59Z

Would appreciate some review from anyone involved in web keyboard handling. I made some very lazy changes to expose what I needed to peek at the data. Maybe there is a better way?

mdebbar

Other than the kPhysicalControlRight oversight, this PR looks good to me!

Piinks · 2023-03-17T19:42:21Z

Looks like there are some merge conflicts FYI

…chive/engine#36348)

flutter-dashboard Bot added embedder Related to the embedder API platform-android platform-web Code specifically for the web engine labels Sep 22, 2022

This was referenced Sep 22, 2022

Add PointerScaleEvent and use in InteractiveViewer flutter/flutter#112172

Merged

ui.PointerSignalKind forwards compatibility for scale flutter/flutter#112170

Merged

Add discrete scale pointer signal #36342

Merged

moffatman force-pushed the web_scale branch 2 times, most recently from 5eff5dd to ea4cb08 Compare September 22, 2022 19:30

justinmc reviewed Sep 26, 2022

View reviewed changes

GaryQian removed the platform-android label Sep 27, 2022

moffatman force-pushed the web_scale branch from ea4cb08 to 7c0c7e6 Compare September 27, 2022 19:44

moffatman force-pushed the web_scale branch from 7c0c7e6 to 0b5d839 Compare October 20, 2022 05:40

moffatman force-pushed the web_scale branch from 0b5d839 to 9e2cb62 Compare March 8, 2023 00:30

justinmc approved these changes Mar 16, 2023

View reviewed changes

mdebbar approved these changes Mar 17, 2023

View reviewed changes

Comment thread lib/web_ui/lib/src/engine/pointer_binding.dart Outdated

moffatman force-pushed the web_scale branch from b39ef2e to c45f1ce Compare March 17, 2023 15:48

moffatman added 3 commits March 17, 2023 17:22

Discrete pinch-to-zoom for web

7c80e7e

Add test

3ce3a8f

Check if physical ctrlKey is pressed

2314eda

moffatman force-pushed the web_scale branch from c45f1ce to 2314eda Compare March 17, 2023 22:50

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2023

auto-submit Bot merged commit be9b333 into flutter-team-archive:main Mar 20, 2023

engine-flutter-autoroll mentioned this pull request Mar 20, 2023

Roll Flutter Engine from 68334e9590a8 to be9b3335c069 (2 revisions) flutter/flutter#123038

Merged

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

be9b333 Send PointerSignalKind.scale events from web (flutter-team-ar…

9c9075e

…chive/engine#36348)

HansMuller mentioned this pull request Mar 24, 2023

Flutter Framework Team: Notable Commits flutter/flutter#121415

Closed

Uh oh!

Conversation

moffatman commented Sep 22, 2022

Sequence

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

moffatman Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

mdebbar Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

moffatman commented Sep 27, 2022

Uh oh!

mdebbar commented Sep 27, 2022

Uh oh!

moffatman commented Sep 27, 2022

Uh oh!

mdebbar commented Oct 6, 2022

Uh oh!

moffatman commented Oct 6, 2022

Uh oh!

mdebbar commented Oct 6, 2022

Uh oh!

justinmc commented Oct 6, 2022

Uh oh!

skia-gold commented Oct 20, 2022

Uh oh!

Piinks commented Nov 22, 2022

Uh oh!

goderbauer commented Mar 7, 2023

Uh oh!

moffatman commented Mar 8, 2023

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

moffatman commented Mar 17, 2023

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Piinks commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

moffatman commented Mar 16, 2023 •

edited

Loading