Bug 261348 - [GTK] Remove key event reinjection
Summary: [GTK] Remove key event reinjection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-08 13:38 PDT by Michael Catanzaro
Modified: 2024-02-01 10:17 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2023-09-08 13:38:46 PDT
The key event reinjection logic is causing many problems, see https://gitlab.gnome.org/GNOME/epiphany/-/issues/1915 and bug #260602, and relies on gdk_display_put_event(), which is deprecated and will be removed in GTK 5. Let's remove all of this logic and have the web view always consume all GTK events. If the corresponding DOM event is not handled, then we need to manually activate application accelerators to ensure keyboard shortcuts keep working as expected.
Comment 1 Michael Catanzaro 2023-09-08 14:15:42 PDT
Pull request: https://github.com/WebKit/WebKit/pull/17609
Comment 2 Michael Catanzaro 2024-01-31 09:35:16 PST
(In reply to Michael Catanzaro from comment #1)
> Pull request: https://github.com/WebKit/WebKit/pull/17609

Hi Benjamin, this pull request removes the key event reinjection that you dislike. But:

 * I have failed to convince Carlos Garcia that this is a good idea, so it's not on track to land.
 * We still have scroll wheel event reinjection, and I have no clue what to do about that.

If we don't find a solution for wheel events, and if GTK 5 does not have gdk_display_put_event(), then apps like Geary and Evolution will probably never be able to port to GTK 5. Email clients use a scrolled view containing multiple web views and the parent scrolled view needs to know whether the web view handled the wheel event or not; if WebKit always handles the event, then scrolling the parent view would become extremely difficult as you would have to position the mouse cursor in the margin outside the web views, and if WebKit never handles the event, then the parent view would always scroll inappropriately. But WebKit will not ever be able to handle the events synchronously as that would be devastating for performance. (I think it's not a problem for scrolling via keyboard because that is expected to only scroll the parent view in these applications.)

One option would be to simply un-deprecate gdk_display_put_event() and instead document that it should be rarely used.
Comment 3 Benjamin Otte 2024-01-31 10:10:57 PST
My goal for GTK5 - but I'm not thinking about input very much, and certainly not atm - is to get scroll events to be grouped into sequences - like touch events. And that means ensuring that sequences are consistent, so that you can use it for kinetic scrolling and so that once you started scrolling, there's not suddenly a spinbutton taking over your scroll events because the mouse handed up over the spinbutton for this one frame.

And you can't really have such an infrastructure working well if you allow random code to inject random events.

That doesn't make your use case invalid, and someone will have to figure out how to deal with that, but I'm pretty sure that it means gdk_display_put_event() will need a replacement.
Comment 4 Michael Catanzaro 2024-01-31 12:55:10 PST
Hm. Maybe we can save wheel events for later, and land the key event removal now regardless, so that we can fix Google Docs and other websites that rely on keyboard shortcuts. This is a frequent complaint from users.
Comment 5 Carlos Garcia Campos 2024-02-01 00:52:28 PST
(In reply to Michael Catanzaro from comment #2)
> (In reply to Michael Catanzaro from comment #1)
> > Pull request: https://github.com/WebKit/WebKit/pull/17609
> 
> Hi Benjamin, this pull request removes the key event reinjection that you
> dislike. But:
> 
>  * I have failed to convince Carlos Garcia that this is a good idea, so it's
> not on track to land.

Well, I'm not against the change itself, I never liked the event reinjection, what I don't want is making decisions now based on something that doesn't even exist yet (GTK5).
Comment 6 Michael Catanzaro 2024-02-01 06:57:15 PST
(In reply to Carlos Garcia Campos from comment #5)
> Well, I'm not against the change itself, I never liked the event
> reinjection, what I don't want is making decisions now based on something
> that doesn't even exist yet (GTK5).

Let's focus on the immediate problem then. Keyboard shortcuts are broken on websites, and users are complaining today. Browsers cannot fix this without changes in WebKitGTK.
Comment 7 EWS 2024-02-01 10:16:52 PST
Committed 273922@main (fd1f0b783bb7): <https://commits.webkit.org/273922@main>

Reviewed commits have been landed. Closing PR #17609 and removing active labels.