Bug 490270 - Wayland-native apps get sent wrong modifier key data
Summary: Wayland-native apps get sent wrong modifier key data
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: input (show other bugs)
Version: 6.1.1
Platform: Fedora RPMs Linux
: HI normal
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-14 19:32 UTC by Renner03
Modified: 2024-07-24 17:20 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 6.2.0
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Renner03 2024-07-14 19:32:19 UTC
SUMMARY
if you set a key (alt, shift, ctrl, or all of them) in konsole for highlighting urls it does not work.

STEPS TO REPRODUCE
1. konsole > hamburger menu > new profile
2.  edit new profile, set as default
3. advanced > set ctrl (or any combination) to show URL hints
4. restart konsole
5. type a url like https://example.org

OBSERVED RESULT
Pressing the key combo does not highlight URLs.
However if you press any additional letter and the key combo it will hint the URLs and work as intended

EXPECTED RESULT
it hints URLs when the desired key combination is pressed

SOFTWARE/OS VERSIONS
Linux: Fedora Kinoite 40
KDE Plasma Version: 6.1.1
KDE Frameworks Version: 6.3.0
Qt Version: 6.7.2

ADDITIONAL INFORMATION
There are no colliding shortcuts in system settings that would trigger only pressing ctrl

It seems to be related to the Legacy X11 App Support
If the default settings (Fedora, KDE Neon unstable) are used (as above, plus any key typed while the ctrl, alt or meta keys are pressed) it does NOT work.
If you set "Never" it works.

Any setting other than "Never" is broken.

If you launch konsole with:
QT_QPA_PLATFORM=xcb konsole under Xwayland it will work regardless of the X11 App Support settings

Maybe something related to the recently added single modifier key feature?
Comment 1 Nicolas Fella 2024-07-14 22:57:40 UTC
Also happening with 5.27, so not a new behavior. 

What's suspicious and probably related:

Using "Never" we first get wl_keyboard.key, then wl_keyboard.modifiers:

[14:     wl_keyboard] key: serial: 12960; time: 12368442; key: 37; state: 1 (pressed)
                      sym: Control_L    (65507), utf8: ''
[14:     wl_keyboard] modifiers: serial: 12961; group: 0
                      depressed: 00000004: Control 
                      latched: 00000000
                      locked: 00000000
[14:     wl_keyboard] key: serial: 12962; time: 12372454; key: 37; state: 0 (released)
                      sym: Control_L    (65507), utf8: ''
[14:     wl_keyboard] modifiers: serial: 12963; group: 0
                      depressed: 00000000
                      latched: 00000000
                      locked: 00000000

Using other settings we get first wl_keyboard.modifiers, then wl_keyboard.key:

[14:     wl_keyboard] modifiers: serial: 13475; group: 0
                      depressed: 00000004: Control 
                      latched: 00000000
                      locked: 00000000
[14:     wl_keyboard] key: serial: 13477; time: 12451531; key: 37; state: 1 (pressed)
                      sym: Control_L    (65507), utf8: ''
[14:     wl_keyboard] modifiers: serial: 13478; group: 0
                      depressed: 00000000
                      latched: 00000000
                      locked: 00000000
[14:     wl_keyboard] key: serial: 13480; time: 12452327; key: 37; state: 0 (released)
                      sym: Control_L    (65507), utf8: ''

https://wayland.app/protocols/wayland#wl_keyboard:event:key says 
> If this event produces a change in modifiers, then the resulting wl_keyboard.modifiers event must be sent after this event.
Comment 2 David Edmundson 2024-07-15 13:46:05 UTC
Confirmed. 
Code in Xwayland is:

keyboard->sendModifiers(xkb->modifierState().depressed,
                                xkb->modifierState().latched,
                                xkb->modifierState().locked,
                                xkb->currentLayout());  // update global state

keyboard->sendKey(event->nativeScanCode(), state, xwaylandClient);  // to only xwayland


that's clearly wrong, but it's not a super trivial fix either.
Comment 3 Bug Janitor Service 2024-07-15 14:44:34 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/kwin/-/merge_requests/6106
Comment 4 Nate Graham 2024-07-18 14:16:57 UTC
Cosmetic issue affecting non-default settings with a known workaround; lowering severity and priority.
Comment 5 Nicolas Fella 2024-07-18 14:19:56 UTC
The Konsole issue is minor, but the root cause potentially affects a lot more apps in unknown ways
Comment 6 Nate Graham 2024-07-18 20:39:20 UTC
What are other potential effects, in general terms?
Comment 7 David Edmundson 2024-07-24 15:56:44 UTC
Any wayland app that acts on modifier keys to do stuff is being given wrong data and therefore could be broken with us at fault, which is why I set it high.

But the bugzilla severity doesn't really matter long term, it's being fixed either way.
Comment 8 David Edmundson 2024-07-24 16:05:37 UTC
Git commit 374d8594931861ef7dfbed8eda5f8552aa4f825c by David Edmundson.
Committed on 24/07/2024 at 15:57.
Pushed by davidedmundson into branch 'master'.

xwayland: Only update keyboard modifers for XWayland's keys

KeyboardInterface is a multiplexer, it has a global state to kwin
that forwards events the single focussed window.

XWayland also forwards events to clients, but uses the keyboard interface.
It has some overloads that take a specific client, this was used for key events
but not modifiers.

The end result was not only that XWayland could miss a modifier update, but
also that wayland clients would get modifier updates out of order.
Key events must come first.

M  +56   -0    autotests/integration/x11keyread.cpp
M  +9    -0    src/wayland/keyboard.cpp
M  +1    -0    src/wayland/keyboard.h
M  +34   -3    src/xwayland/xwayland.cpp

https://invent.kde.org/plasma/kwin/-/commit/374d8594931861ef7dfbed8eda5f8552aa4f825c
Comment 9 Nate Graham 2024-07-24 17:20:10 UTC
Got it. Re-titling and re-raising priority, if only for the historical record's sake.