Some input method fixes #2111

lilydjwg · 2024-01-22T14:53:52Z

I'm sorry to bother you on this thing again but bugs have been found in previous days.

IM sent keys got sent to grab nodes after returning from handle_keyboard_key. This caused e.g. the switcher plugin to deactivate early, or not deactivate.

…s event to IM e.g. in Firefox, place focus anywhere not a text input, press Ctrl-L to focus the urlbar, l would be pressed for Firefox continuously without this patch because we got two release events for one press event.

ammen99 · 2024-01-22T15:36:28Z

src/core/seat/keyboard.cpp

 {
    using namespace std::chrono;

    auto& input = wf::get_core_impl().input;
    auto& seat  = wf::get_core_impl().seat;

-    if (wf::get_core_impl().im_relay->is_im_sent(handle))
-    {
-        mod_binding_key = 0;


I am confused, isn't mod_binding_key supposed to be reset on im keys? With this change, it no longer is reset.

I'm restoring the whole wf::keyboard_t::handle_keyboard_key function to previous state and moving the IM code to the on_key callback.

Ok but the modifier binding key should be reset even for grabbed events. It is used to detect when a modifier binding is pressed (e.g super pressed and released shortly after). Any keys in between reset the modifier binding, grabbed or not.

It should be reset later. It was here just because we returned early here.

ammen99 · 2024-01-22T15:38:09Z

src/core/seat/seat-impl.hpp

@@ -57,6 +57,7 @@ struct seat_t::impl

    void set_keyboard_focus(wf::scene::node_ptr keyboard_focus);
    wf::scene::node_ptr keyboard_focus;
+    bool is_grab = false;


I am highly sceptical of such approaches as you might have noticed .. what makes grab nodes special here? Why is this not a problem for other nodes (normal client surfaces)?

Because it intercepts the client's processing. Consider the following user events:

keydown alt

keydown tab

keyup tab

The switcher plugin activates at event 2. Now the text input is unfocused, so the input releases all pressed keys, sending keyup tab and keyup alt. The latter causes switcher to think the user has released the alt key.

With 49d1218 the switcher case seems to be fixed, but there may be other complex interactions with other plugins.

so the input releases all pressed keys, sending keyup tab and keyup alt. The latter causes switcher to think the user has released the alt key.

I think I am starting to see what is going on, but why on earth does fcitx5 think it needs to generate these key releases? It is not supposed to do so, at least I don't think why it would need that. I'm suspecting that here we're really adding a workaround for an fcitx5 bug.

Maybe we could just ignore all IM virtual keyboard events (whether to grab or not) until we switch focus? Just like we ignore commits until the focused text input switches properly. Because like now imagine alt-tab was not a shortcut which opens a grab, but immediately would switch and give focus to the next window. During normal interaction, that would be (this is what an imaginary plugin MIGHT do, I'm not saying we have a plugin which does this atm :)):

press alt, press tab

old window loses keyboard focus (no need for release events)

new window get keyboard focus and is told that alt/tab are pressed on enter

user releases alt, tab -> new window gets the release events

So what fcitx5 does would be wrong in this case, because fcitx5 generates additional release events between 2 and 3 (according to your description, if I understood it correctly)

why on earth does fcitx5 think it needs to generate these key releases?

It seems to workaround a sway bug....fcitx/fcitx5@6f92a0c

ammen99 · 2024-01-22T15:47:38Z

src/core/seat/input-method-relay.cpp

@@ -221,6 +222,22 @@ bool wf::input_method_relay::should_grab(wlr_keyboard *kbd)
    return !is_im_sent(kbd);
 }

+bool wf::input_method_relay::check_superfluous_release(uint32_t key, uint32_t state)


Hmm, now that you moved the handle_key() call to seat.cpp, doesn't the seat already do this filtering of keys for you automatically? Isn't dbd98d3#diff-206c23fa3adf45f1a58ee9a5865dc0dec21bd2ba72a2441fb41d3fa88794cb6aR45-R59 executed for the keys here, so why need to check here again?

Note I find the whole IM stuff quite confusing so far because it seems everything is a mess (not on Wayfire's side but the protocols & clients & how they work together), so maybe my intuition is wrong ..

Also, do you have any references as to how gnome implements this or kwin? Do they also have so many checks and workarounds for the various brokenness in the protocols? I am kinda hoping there is an 'easy' solution to all the troubles that you've tried to fix here.

so why need to check here again?

That's exactly why again. Take the Firefox case as example:

No text input in focus. The user sends keydown ctrl and keydown l.

Firefox focuses the address as requested. Text input in focus now.

The user sends keyup l.

Input method passes through the keyup l, and sends via virtual keyboard to the compositor.

In the code you linked, this second keyup l is ignored because from the compositor's view it's already been released.

Firefox never receives the keyup l event, and so auto repeat kicks in and a lot of l is inserted.

Also, do you have any references as to how gnome implements this or kwin?

The answer is super simple: they don't. GNOME uses D-Bus, and kwin uses input method v1 (ref).

The answer is super simple: they don't. GNOME uses D-Bus, and kwin uses input method v1 (ref).

Wayland at its finest :((

5. In the code you linked, this second keyup l is ignored because from the compositor's view it's already been released.

Ok, so repeating the flow of events as to how I see it to avoid misunderstandings:

Keydown ctrl, keydown L

Focus text input

IM grabs keyboard

keyup L -> we go to the code I linked -> L is erased from the set -> L is sent to the IM, not to the client

IM sends virtual keyup L to compensate -> we go to the linked code -> L is not in the set, so it is dropped

I would say that the actual problem is that we erased L from the set in step 4. The set is supposed to contain all pressed, but not released keys for the current focus. Since the key was forwarded to the IM and not to the client, then L should have never been removed from the set.

Since the key was forwarded to the IM and not to the client, then L should have never been removed from the set.

Yes, it works nicely!

Oh no. With text input focused, super+some key to switch focus will leave super pressed...

It seems to be some bad interactions with the IM. I did some change to fcitx5 and this seemed to no longer happen.

ammen99 · 2024-01-22T16:12:58Z

src/core/seat/input-method-relay.cpp

+        // application bugs were observed. In this case, we see only commits
+        // but no preedit strings, so we need care the timing.
+        static std::chrono::milliseconds focus_change_duration{100};
+        if (last_focus_changed.has_value())


I looked at the protocol and I believe what we are doing here is not a real fix for the issue. The protocol includes serials which is what we really need here. So here's the deal as far as I understand it:

When we switch focus or do something of the sort, we can send a done event. This generates a new serial number, serial numbers are counted from 0 to infinity and are incremented for each done event. I briefly looked through wlroots implementation but it might be wrong though (because they reset input_method->current_serial on each commit, which might actually have outdated information!). So we might need to keep track of it ourselves.

For each commit from the text input, we receive the last serial that the input method has received so far (wlroots stores that in input_method->current_serial before emitting the commit event). So this means: until the input method has actually received our new configuration, it will send a commit with an outdated serial.

No, I tried to add a done event after set_focus. It sends a commit with a correct serial. It doesn't make any difference whether we've changed focus. The input method receives a sequence of done events and counts it correctly.

No, I tried to add a done event after set_focus. It sends a commit with a correct serial. It doesn't make any difference whether we've changed focus. The input method receives a sequence of done events and counts it correctly.

Did you also try to check whether the serial belongs to the current focus or is an older event in the commit handler?

Actually there is already a done event when switching focus, and so by adding another done event, the input method replies with two commits, with the same serial.

What do you mean by the serial belongs to the current focus? The serial is just a counter of done events. (I'm reading WAYLAND_DEBUG here.)

ammen99 · 2024-01-22T21:03:35Z

Unfortunately I think I need even more explanations to fit all the pieces of this fcitx5 business around my head, so sorry for bothering you all the time @lilydjwg I hope you don't grow too tired of my questions. But I'd like to understand this code so that I don't break it accidentally with some random future change. So here's what bothers me this time. Why does fcitx5 have both a virtual keyboard and an input method? I thought it is supposed to feed the input directly via the input-method protocol, so what is its own virtual keyboard used for? Why would the compositor need to know whether a virtual keyboard belongs to the IM or not? Which protocol or standard explains this?

Scrap that, I asked on #wlroots, this is indeed how it is all supposed to work .. Some of my points above still stand though ;)

lilydjwg · 2024-01-23T05:01:53Z

I'm closing this pr as the code has changed significantly. I'd like the change to be preserved as history rather than overridden.

lilydjwg added 3 commits January 22, 2024 22:51

input method: fix interaction with plugin grabs

dbd98d3

IM sent keys got sent to grab nodes after returning from handle_keyboard_key. This caused e.g. the switcher plugin to deactivate early, or not deactivate.

input method: fix first commit for non-preediting applications

52eb742

ammen99 reviewed Jan 22, 2024

View reviewed changes

lilydjwg closed this Jan 23, 2024

This was referenced Jan 23, 2024

Some input method fixes (take 2) #2113

Merged

wayland v2: don't release pressed keys without user input fcitx/fcitx5#954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some input method fixes #2111

Some input method fixes #2111

lilydjwg commented Jan 22, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 22, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 23, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 22, 2024

ammen99 Jan 22, 2024 •

edited

Loading

ammen99 Jan 22, 2024

lilydjwg Jan 23, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 22, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 23, 2024

lilydjwg Jan 23, 2024 •

edited

Loading

lilydjwg Jan 23, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 22, 2024

ammen99 Jan 22, 2024

lilydjwg Jan 23, 2024

ammen99 commented Jan 22, 2024 •

edited

Loading

lilydjwg commented Jan 23, 2024

Some input method fixes #2111

Some input method fixes #2111

Conversation

lilydjwg commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammen99 Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilydjwg Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammen99 commented Jan 22, 2024 • edited Loading

lilydjwg commented Jan 23, 2024

ammen99 Jan 22, 2024 •

edited

Loading

lilydjwg Jan 23, 2024 •

edited

Loading

ammen99 commented Jan 22, 2024 •

edited

Loading