Skip to content
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

Fix latches #611

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix latches #611

wants to merge 2 commits into from

Conversation

wismill
Copy link
Member

@wismill wismill commented Jan 24, 2025

Prior to this MR, "latch A down" → "latch B down" → "latch A up" → "latch B up" would result in only B being latched. After this MR, both A and B end up latched.

Changed latching behavior so that the same rules used to detect whether an action breaks a latch after the latching key is released, are now also used to detect whether the action prevents the latch from triggering at the latching key release.

The major effect of this change is that depressing and releasing two latching keys simultaneously will now activate both latches, as expected. This will allow us to provide proper Sticky Key support.

Note that it seems X11 handle simultaneous modifiers only if pressed sequentially, not simultaneously. But I have not done extensive tests. It seems a bug however; according to the XKB specification §2.1:

Latched modifiers or groups apply only to the next key event that does not change keyboard state.

CC @Jules-Bertholet @mahkoh


@Jules-Bertholet I extracted the bug fix from #569 (good catch!), reworked it and added a big bunch of tests.

wismill and others added 2 commits January 24, 2025 16:25
- Added a big bunch of tests for modifier latch. No yet exhaustive,
  but should cover the most usual use cases.
- Added missing test cases for breaking the group latch. Ideally, more
  tests should be added to match the coverage of modifiers latches.
Prior to this commit, "latch A down" → "latch B down" →
"latch A up" → "latch B up" would result in only B being latched.
After this commit, both A and B end up latched.

Changed latching behavior so that the same rules used to detect
whether an action breaks a latch after the latching key is released,
are now also used to detect whether the action prevents the latch
from triggering at the latching key release.

The major effect of this change is that depressing and releasing
two latching keys simultaneously will now activate both latches, as
expected. This will allow us to provide proper Sticky Key support.

Co-authored-by: Jules Bertholet <[email protected]>
Co-authored-by: Pierre Le Marre <[email protected]>
@wismill wismill added bug Indicates an unexpected problem or unintended behavior state Indicates a need for improvements or additions to the xkb_state API labels Jan 24, 2025
@wismill wismill added this to the 1.8.0 milestone Jan 24, 2025
@wismill wismill requested review from bluetech and whot January 24, 2025 15:39
@mahkoh
Copy link

mahkoh commented Jan 24, 2025

The specification says

if no keys were operated simultaneously with the latching modifier key, key release events [latch the modifier]

There are two abiguities:

  1. What does "operated" mean? I'm taking this to mean pressed or released. X11 contains multiple incompatible interpretations of this.

    For LatchMods, it is interpreted at presses-only. In the case you're describing, the B modifiers should be latched. For SetMods, it is interpreted as presses or releases. A contradiction.

  2. What does "simultaneous" mean? X11 consistently interprets this as "a press or release after the key in question was pressed". I'm following this lead. But one could also include the situation where another key was already being held down (i.e. the act of holding a key down being considered an operation in itself). In that case, A down -> B down -> B up -> A up should cause no latches to be activated.

In the scenario that you are describing, I'm not activating any latches since the behavior of SetMods from X11 seems to be the desired one with the behavior of LatchMods being a bug due to the structure of the code.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Jan 24, 2025

I believe X11’s behavior of no simultaneous latches is the one that most strictly matches the spec. The spec says “latched modifiers or groups apply only to the next key event that does not change keyboard state”, yes—but the modifier is only properly considered latched after the latching key is released. Therefore, the “does not change keyboard state” requirement doesn’t apply to pressing two latch keys simultaneously.

The issue, of course, is that the spec behavior around latching is simply terrible! That’s why I filed #569 to deliberately deviate from it.

@wismill
Copy link
Member Author

wismill commented Jan 24, 2025

In the scenario that you are describing, I'm not activating any latches since the behavior of SetMods from X11 seems to be the desired one with the behavior of LatchMods being a bug due to the structure of the code.

@mahkoh Yet I do not think this is the expected behavior. The motivation of latches is accessibility. In Xorg they can be activated by the StickyKeys control.

Now, imagine a user who needs to press Ctrl+Shift+A. They may choose to press the modifiers one the same side. Being close to each other, it is possible that the keys are tapped simultaneously (simultaneously down), rather than tapped sequentially (down/up sequence). With your take, it would not work and the user would just type A.

But I am not an accessibility expert, so ultimately I would implement whatever experts advise to get proper Sticky Keys. If they say the feature is broken anyway and that the spec is ambiguous and the Xorg implementation is buggy, then I would say it’s up to each implementation to do their best to satisfy their users.

@wismill
Copy link
Member Author

wismill commented Jan 24, 2025

FYI activating Sticky Keys in some DE under Wayland:

  • KDE Plasma 6.2.5 follows the same path than this MR.
  • Gnome 47 locks the modifiers press simultaneously. I have no idea why.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Jan 24, 2025

There is a risk that this PR might actually make the experience worse for some users, if the “new latches break existing latches for non-preserved modifiers in the modifier list of the type of the key that triggered the new latch” change from #569 is not also adopted.

Usually, when you press two latching keys in succession, you want both modifiers to end up latched. This MR ensures that this always happens. However, sometimes you actually do want the first latch to be dropped. This is the case for layouts like bépo that use ISO_Level5_Latch as a dead key; activating that latch needs to break prior latches, otherwise the dead key is unusable.

With xkbcommon in its current state, combining sticky Shift or AltGr with an ISO_Level5_Latch dead key is still minimally usable as long as you are careful to press the keys simultaneously. Merging this PR alone, without the other changes in #569, breaks that use-case.

If they say the feature is broken anyway and that the spec is ambiguous and the Xorg implementation is buggy, then I would say it’s up to each implementation to do their best to satisfy their users.

So, if we are to accept deviation from the spec anyway in order to better satisfy users, there is really no reason not to adopt all of #569.

@wismill
Copy link
Member Author

wismill commented Jan 24, 2025

This is the case for layouts like bépo that use ISO_Level5_Latch as a dead key;

@Jules-Bertholet Bépo does not use LevelFive modifier at all.

activating that latch needs to break existing latches, otherwise the dead key is unusable.

I do not understand the issue of your use case. Please pickup a layout in xkeyboard-config and write down a key sequence with the expected + current MR behavior to illustrate.

If they say the feature is broken anyway and that the spec is ambiguous and the Xorg implementation is buggy, then I would say it’s up to each implementation to do their best to satisfy their users.

So, if we are to accept deviation from the spec anyway in order to better satisfy users, there is really no reason not to adopt all of #569.

This is not what I wrote. Your MR explicitly deviates from the spec and this is not the subject here. Here we try to match the spec, but due to the ambiguities we try to achieve a common interpretation. Whether your MR will be accepted or not will not be considered until release 1.10, as I commented.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Jan 24, 2025

Bépo does not use LevelFive modifier at all.

Oh, my bad! The German E1 layout will do, then.

Imagine you start with that layout, and then set up sticky keys, replacing Shift and AltGr with latching versions. Then, press AltGr followed by <AC04> (ISO_Level5_Latch). The desired behavior here is for the AltGr latch to be broken, otherwise levels 5 and 6 are entirely inaccessible.

@mahkoh
Copy link

mahkoh commented Jan 24, 2025

  • KDE Plasma 6.2.5 follows the same path than this MR.
  • Gnome 47 locks the modifiers press simultaneously. I have no idea why.

I'd be interested in seeing the actual keymaps that you tested this with.

@wismill
Copy link
Member Author

wismill commented Jan 24, 2025

I'd be interested in seeing the actual keymaps that you tested this with.

@mahkoh I just activated the Sticky Keys feature in each desktop. Any keymap would do.

@mahkoh
Copy link

mahkoh commented Jan 24, 2025

I thought you were talking about the behavior of LatchMods. But it seems that you're talking about how these compositors treat changes in the depressed mods when their respective sticky key implementations are active.

@wismill
Copy link
Member Author

wismill commented Jan 24, 2025

@mahkoh yes, I meant the sticky keys implementation of the Wayland compositors. I think most1 compositors use xkbcommon, which does not support this, so it’s up to each compositor. And not all of them implement it correctly, so I opened #596. On the other hand the latch behavior without sticky keys should be the same on all compositors otherwise. That is, if they do not mess with keyboard state.

Footnotes

  1. If not all, apart Jay with kbvm 🙂. Curious to know if there is another implementation of XKB in use.

@wismill wismill mentioned this pull request Jan 25, 2025
1 task
@wismill
Copy link
Member Author

wismill commented Jan 25, 2025

I added the tests to #618, but with the current behavior. The current MR will then require rebase.

@Jules-Bertholet
Copy link
Contributor

Here we try to match the spec, but due to the ambiguities we try to achieve a common interpretation.

The spec says “if no keys were operated simultaneously with the latching modifier key”, not “if no keys that don’t affect keyboard state were operated simultaneously with the latching modifier key”. The meaning of “operated” is ambiguous, as pointed out by @mahkoh. But the meaning of “no keys” is not ambiguous at all, and it doesn’t match this PR.


The way I see it:

  • If we want DEs like KDE to adopt xkbcommon’s version of sticky keys, this PR’s changes are necessary, because otherwise it would be a regression compared to their existing setup.
  • If this PR’s changes are adopted, the “new latches break existing latches for non-preserved modifiers in the modifier list of the type of the key that triggered the new latch” change from Various fixes and improvements to latching behavior #569 also needs to be adopted, as otherwise layouts like German E1 + sticky AltGr break.
  • If we adopt that change, then latches will sometimes be able to break other latches, so it would be really confusing if locks can’t break latches. So it would also make sense to adopt the “locks break latches” change from Various fixes and improvements to latching behavior #569.
  • So, in summary, we should adopt Various fixes and improvements to latching behavior #569 in its entirety instead of splitting it apart.

@Jules-Bertholet
Copy link
Contributor

Actually, scratch that, “operated” isn’t ambiguous either. As it’s used in the description of XkbActionMessage, it clearly means “press or release”. So really, nothing about this part of the spec is ambiguous.

@wismill
Copy link
Member Author

wismill commented Jan 25, 2025

So, is your understanding that the only use case considered by XKB to trigger latches is keys strictly sequentially tapped (i.e. press then released)?

We currently also trigger the latch if whatever different key is released between the press and release, i.e. A press + B release + A release will latch A (but not B if it was a latch).

@mahkoh
Copy link

mahkoh commented Jan 25, 2025

FYI the shift key on my android phone does not latch if another key is pressed while the shift key is pressed.

@Jules-Bertholet
Copy link
Contributor

So, is your understanding that the only use case considered by XKB to trigger latches is keys strictly sequentially tapped (i.e. press then released)?

That is my understanding of the spec, yes. However, I also believe that this part of the spec is bad, and xkbcommon’s current behavior, where the latch still forms if another key is released, is better.

@wismill wismill modified the milestones: 1.8.0, 1.11.0 Jan 27, 2025
@wismill
Copy link
Member Author

wismill commented Jan 27, 2025

Let’s not rush and wait 1.11 to see if we want to deviate from the spec.

@wismill wismill marked this pull request as draft January 27, 2025 16:47
@wismill wismill added X11 legacy: compatibility Indicate a need to ensure compatibility with X11 needs info and removed bug Indicates an unexpected problem or unintended behavior labels Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs info state Indicates a need for improvements or additions to the xkb_state API X11 legacy: compatibility Indicate a need to ensure compatibility with X11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants