Workaround OSK issues on electron

[tarek-y-ismail]

Closes #3580.

I haven't wrapped my head around how everything connects together regarding how we manage the OSK and its state. So consider this PR as set of workarounds/shims until then.

Fixes a few issues:

1. The "Interval" field showing the OSK for a split second
   This was because commit_state was called before any other code had the chance to set pending_state, which caused the input handler to be disabled, hiding the OSK.

2. The "Interval" field (and similar fields) didn't get input changes as you used the OSK
   This was due to text_changed requiring pending_state to be set, despite not using it. And taking into account the possible mismanagement/client misbehavior clearing pending_state , this caused changes to not go through.

3. Tabbing from the interval field to the minutes/hours/days field to its right opened the OSK in number mode.
   This was due to reset being called between the content type being set in the state and the state being actually committed, causing Mir to lose the state and not commit it to the handler, leading to the previous mode (numbers) carrying over to the new field.

Some things I've noticed while working:

1. When using miral-kiosk, the client crashes after the OSK shows up for one frame. miral-app was used for testing and debugging so far. This wasn't the case when I originally worked on this issue. I tried going back to v2.17.0 and it still occurs there.
2. In text fields like the ones in "Compliance reports", text is cleared from the field once it loses focus unless you press enter on your keyboard or the OSK. I'm not sure if this is a bug or intended behavior.

[AlanGriffiths]

I'm unclear why we need both an optional pending_state and a pending_state_used. I realise that pending_state_used == pending_state isn't an invariant as proposed, but it is too close to being one for me to be convinced we need both.

Looking through the code, it looks as though pending_state should be set if and only if we are between activate() and deactivate() transitions.

As you have noted the reset() method breaks the state machine by resetting pending_state_used regardless of activate() and deactivate() transitions.

I think reset() behaving badly is the immediate cause of the other problems.

The root cause, of course, is that using pending_state.is_set() to track the activate() and deactivate() transitions is cryptic at best.

In summary, I think pending_state_used and the other changes proposed are unnecessary, but that both reset() and commit_state() should be resetting the value stored in pending_state (n.b. not the optional itself)

[tarek-y-ismail]

Just as Alan suggested, going with optional::emplace instead of optional::reset somewhat fixes the issue a lot more elegantly. The keyboard pops up in text mode, and the user can manually switch to number mode to interact with numeric fields. I see this as a good middle ground between not having a keyboard at all and having things "work" but risking future bugs with hacky workarounds. @Saviq What do you think?

[Saviq]

@Saviq What do you think?

So rather than popping up, and hiding, the numerical keyboard, we'd pop up the standard keyboard? Discarding the input field type?

Was Mir doing something wrong wrt. to the protocol? If not, I don't think we should be breaking that. If the OSK is reacting badly to us using the protocol correctly, that's something to fix in Squeekboard, IMO.

[tarek-y-ismail]

I think it's more of the client (electron) misbehaving. We get a reset request between activate and deactivate for no apparent reason. On main, this causes us to null the optional which effectively disables the keyboard, Alan's solution guarantees at least a usable OSK.

[Saviq]

Alan's solution guarantees at least a usable OSK.

Ah, but a well behaved client will still get the full monty. WFM!

[tarek-y-ismail]

Ah, but a well behaved client will still get the full monty. WFM!

Any examples of such client? Just for testing purposes

[Saviq]

Any examples of such client? Just for testing purposes

I've only actually seen browsers choose the field format, so trying in different browsers (e.g. https://snapcraft.io/wpe-webkit-mir-kiosk), provided they actually support the input method protocol…

[tarek-y-ismail]

Ok, so I just tested the demo URL in the original issue on both electron and wpe-webkit. With no changes to workaround the bug, wpe doesn't seem to exhibit this bug.

[Saviq]

wpe doesn't seem to exhibit this bug.

You mean with WPE and ubuntu-frame-osk, this PR is not needed and everything behaves fine?

So the issue is in Electron? Can you check Chromium out as well?

[tarek-y-ismail]

You mean with WPE and ubuntu-frame-osk, this PR is not needed and everything behaves fine?

It seems so, yes. I think this is mainly because WPE webkit uses TextInputV3, which doesn't have a reset request.

Can you check Chromium out as well?

Chromium 133.0.6875.0 unstable. Without the fix applied, the "Interval" field in the Ignition demo shows the exact same behavior as in Electron, the number field in W3shool's example thingy works fine

[Saviq]

because WPE webkit uses TextInputV3, which doesn't have a reset request.

And Electron / Chromium misuse the reset request in TextInputV1, sending it at the wrong time?

And how does the .reset() > .emplace() affect our protocol compliance? I suppose it clears the pending state, so what it says on the tin?

[tarek-y-ismail]

And Electron / Chromium misuse the reset request in TextInputV1, sending it at the wrong time?

Exactly!

And how does the .reset() > .emplace() affect our protocol compliance? I suppose it clears the pending state, so what it says on the tin?

According to wayland.emersion.fr:

Request zwp_text_input_v1.reset — reset

Should be called by an editor widget when the input state should be reset, for example after the text was changed outside of the normal input method flow.

So yeah, we're still good.

[AlanGriffiths]

And how does the .reset() > .emplace() affect our protocol compliance? I suppose it clears the pending state, so what it says on the tin?

It occurred to me that it still isn't quite right. If we receive a reset() when not active (which is a weird scenario, and I've not checked if the protocol allows it), then we shouldn't be setting a value (by emplace())

[Saviq]

If we receive a reset() when not active (which is a weird scenario, and I've not checked if the protocol allows it)

void zwp_text_input_v1_reset(
    struct zwp_text_input_v1* zwp_text_input_v1
)

Should be called by an editor widget when the input state should be
reset, for example after the text was changed outside of the normal
input method flow.

Guess what. It's undefined.

then we shouldn't be setting a value (by emplace())

It's an empty pending state, so does it matter?

[AlanGriffiths]

It's an empty pending state, so does it matter?

The code is confusing: instead of having active/inactive states, or even an is_active boolean, pending_state is an optional that indicates "active" by having a value (even an empty one). See the rant in my review earlier