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
Send PointerSignalKind.scale events from web #36348
Conversation
5eff5dd
to
ea4cb08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
scrollDeltaX: deltaX, | ||
scrollDeltaY: deltaY, | ||
); | ||
if (event.ctrlKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens on Mac with ctrl and cmd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wheel events with ctrlKey
on Mac do not zoom/scale. Instead they just scroll. But with this PR, the scrolling will stop, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that ctrl (not cmd) and scroll will continue to do the same thing (scroll) after this PR, is that right @moffatman?
scrollDeltaY: deltaY, | ||
); | ||
if (event.ctrlKey) { | ||
_pointerDataConverter.convert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be } else {
.
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. |
@moffatman you are right that we ignore ctrl-zoom now, but that's not the case on Mac/iOS. |
@mdebbar So on macOS, when using ctrl-scroll wheel, what's the expected behavior? |
ea4cb08
to
7c0c7e6
Compare
@moffatman I believe on macOS, it should just scroll even if ctrl is pressed. |
@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? |
@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 ? |
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? |
Gold has detected about 8 new digest(s) on patchset 3. |
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 |
@moffatman @justinmc @mdebbar @Piinks Are there still plans for this PR? It is getting a little stale... |
Remaining issues
To start with, I will fix the merge conflicts at least.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
scrollDeltaX: deltaX, | ||
scrollDeltaY: deltaY, | ||
); | ||
if (event.ctrlKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that ctrl (not cmd) and scroll will continue to do the same thing (scroll) after this PR, is that right @moffatman?
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. |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the kPhysicalControlRight
oversight, this PR looks good to me!
Looks like there are some merge conflicts FYI |
Send
PointerSignalKind.scale
instead ofPointerSignalKind.scroll
if ctrl key is pressed on wheel event.Part of flutter/flutter#112103
Sequence
Pre-launch Checklist
writing and running engine tests.
///
).