"Control" modifier incorrectly consumed on keypad

[ghost]

If you press e.g. the '+' key on the keypad, and the ctrl key is pressed, then the xkb API will report the ctrl modifier as consumed, even though ctrl doesn't change the interpretation of the key (i.e. same keysym).

I'd expect that ctrl is not considered consumed in this case.

The APIs I was looking at were the following:

xkb_keysym_t sym = xkb_state_key_get_one_sym(state, code);
xkb_mod_index_t index = xkb_keymap_mod_get_index(keymap, XKB_MOD_NAME_CTRL);
int active = xkb_state_mod_index_is_active(state,  index, XKB_STATE_MODS_DEPRESSED);
int consumed = xkb_state_mod_index_is_consumed(state, code, index);

active and consumed can both be 1 for code==XKB_KEY_KP_Add.

[bluetech]

The file /usr/share/X11/xkb/symbols/keypad has this in the operator section, which is included by default:

     key <KPAD> {
         type="CTRL+ALT",»       // +VMode switches to the next video mode
         symbols[Group1]= [ KP_Add, KP_Add, KP_Add, KP_Add, XF86_Next_VMode ]
     };

The type CTRL+ALT is defined as follows in types/basic:

    type "CTRL+ALT" {
        modifiers = Control+Alt+Shift+LevelThree;
        map[None] = Level1;
        map[Shift] = Level2;
        map[LevelThree] = Level3;
        map[Shift+LevelThree] = Level4;
        map[Control+Alt] = Level5;
        preserve[Shift] = Shift;
        preserve[Shift+LevelThree] = Shift;
        level_name[Level1] = "Base";
        level_name[Level2] = "Shift";
        level_name[Level3] = "Alt Base";
        level_name[Level4] = "Shift Alt";
        level_name[Level5] = "Ctrl+Alt";
    };

So the type indeed does not explicitly assign a level for just Control. However the way this is interpreted is that there an implicit assignment to Level0. As there is not preserve statement for this combination, the Control modifier is consumed in this case.

Now, you might think this is unintuitive (= not what user expects), and that in cases where the is no explicit assignment the modifiers of the type should not be consumed. And in fact this is what we did initially in libxkbcommon. But this caused problems in other cases and is not what Xlib does. So it was changed to the current behavior here: 1054962

This behavior is debatable but I think we should keep it as-is. If you want to explain the actual problem this caused you (I'm assuming having to do with shortcuts, which are actually very tricky to get right) there might be a solution... Or you can argue for xkeyboard-config to add a preserve[Control] = Control type clauses to the CTRL+ALT type.

[ghost]

So it was changed to the current behavior here: 1054962

This commit looks like it's of cosmetic nature. Got the wrong one?

If you want to explain the actual problem this caused you (I'm assuming having to do with shortcuts, which are actually very tricky to get right) there might be a solution...

This is about handling key shortcuts in a wayland application. I want to distinguish between + and CTRL++. On the other hand SHIFT+1 should produce !, and not SHIFT+!. So I need to know whether a modifier affects the mapping of a key, or whether it's "separate". I thought "consumed" keys are the abstraction for this. Maybe misunderstood the purpose of "consumed" keys? (Or should I always include Control unconditionally, while filtering the other modifiers?)

[bluetech]

I'll reply a little later, but yes, I meant this one: 5417440

[bluetech]

As I said, shortcuts are tricky. Every time you fix one edge case another pops up. So I'd recommend using a toolkit if possible instead of rolling your own (even they still have issues though). But let's try...

Here are some definitions first so things are clear:

significant_mods: these are the mods which are at all relevant for shortcuts. Let's say it's Shift | Ctrl | Alt | Super. The purpose of this is that (commonly a shortcut like Alt+Tab should not match upon Shift+Alt+Tab - the Shift here is important, however it should match upon Alt+NumLock+Tab because NumLock is irrelevant. I'm assuming significant_mods is masked where appropriate.

shortcuts_mods: the modifiers for the shortcuts. E.g. for Ctrl+Alt+Del it's Ctrl | Alt. I assume shortcut_mods ⊆ significant_mods (and generally let's the the

shortcut_keysym: the keysym for the shortcut. E.g. for Ctrl+Alt+Del it's Delete.

Note: some people had the idea of not using keysyms but just the keycodes, thus circumventing the entire issue. But this doesn't work: a "semantic" shortcut like Ctrl+O for Open should use the appropriate (different) physical keys for e.g. qwerty and dvorak. Not to speak about i18n, etc.

active_mods: these are the mods which are active in the keyboard state at the time the shortcut is matched.

consumed_mods: these are the mods which are used up the keysym translation (i.e. selecting the key level to use) in the keyboard state at the time the shortcut is matched. These are the mods considered by the key type (the modifiers clause) minus mods which are preserved (by the appropriate preserve clause if exists).

Well a first rule which seems safe enough is that shortcut_mods ⊆ active_mods. That is, if a shortcut explicitly specifies Shift or Ctrl, they really must be active - not considering the consumed mods yet.
So already a plain + cannot match Ctrl-+ (but doesn't take care of the inverse).

Before the Ctrl-+ let's look at the Shift-! where the consumed is simpler. There are 3 scenarios:

• There's only a shortcut for Shift-!.
• There's only a shortcut for !.
• There are different shortcuts for both Shift-! and !.

For the ! case you do want to mask out consumed_mods from active_mods. For the Shift-! case, assuming the shortcut_mods ⊆ active_mods condition passed, you only need to ensure there are no significant, active, unconsumed, unmatched mods. I think this can be done without breaking the first case.

For the both ! and Shift-! case, they can both match simultaneously, so you need to decide on some policy (e.g. longest match, or definition order). Then you can sort the shortcut table by this policy, or decide on a best match dynamically.

So I think the problematic scenario left for Ctrl-+ after you handled the general cases is when only a shortcut for + is defined but you don't want it to match for Ctrl-+. I don't know what to say about this, the keymap says Ctrl is consumed here so the shortcut should match. But I don't think that's such a big problem.

Sorry for the length, “I didn't have time to write a short letter, so I wrote a long one instead.” :)

[ghost]

Thanks for your explanation. I'm not sure whether I really got it. Because CTRL+ALT plus KP_Add generates XF86_Next_VMode, CTRL is always automatically considered "consumed" when CTRL is active, and KP_Add is hit? I don't understand this. In my opinion, it should be "consumed" only if it actually generates XF86_Next_VMode.

[bluetech]

I'll tell you more: if you just press A with no active modifiers, Shift and Lock are consumed.

Rephrasing your request: if active_mods & type->mask doesn't match any entry in the type, don't consider any modifiers as consumed - as opposed to what happens now, which is an implicit map[...] = Level1, which means all of type->mask is consumed.

While interesting (I haven't considered it too much) it already breaks several keymaps which rely on something called "capitalization transformation". Besides, the implicit rule behavior is what the keymap authors expected when they wrote the keymaps.

Btw, wanted to mention something about 5417440 which I failed to mention in the commit message: the new behavior is actually a "superset" of the old one, since the old one is basically consumed_mods & active_mods.

[ghost]

So what should applications do, which want to use the modifiers anyway in this specific case? Special-case all the keys that are affected by this?

[bluetech]

I'm tempted to say "that's just the way it is" but I'll try to resist :)

GTK seem to use the following heuristic: only add to the consumed modifiers those which appear in a single-modifier entry (e.g. map[Shift] = Level2;) and those of the matching entry (taking into account the appropriate preserve in each case), but not the others in type->mask. They do so by copying the function out of xlib ("MyEnhancedXkbTranslateKeyCode") and changing its internals. Unfortunately(?) you can't do the same...

By seemingly sheer luck, this heuristic works well for the case you describe (couldn't break it in gimp at least); Control has no entry on its own; Shift happens to be preserved; LevelThree is not in their significant_mods; Control+Alt maps to a different keysym. I'll try to play with it a bit, but I don't think we can do something like this.

[fooishbar]

Yeah, this behaviour does seem to be correct, AFAICT. Maybe we could add the optional GTK+-style behaviour with a state flag? Seems to be a bit of a copout, but ...

Funnily enough, this actually tripped me up: https://bugzilla.gnome.org/show_bug.cgi?id=754110

[fooishbar]

@bluetech Any opinion on whether new API, or updating the keymap to have explicit 'consumed' entries, would be better?

[bluetech]

@fooishbar I completely forgot about this, sorry. I replied in the gnome bug.

[fooishbar]

@bluetech No problem. For what it's worth, my litmus test for when xkbcommon was 'done' was when we had something good enough to work for GTK+ shortcuts, so let's fix this and call it 1.0.0. :)

[fooishbar]

@bluetech So, fun thing I just realised: xkb_state_key_get_consumed_mods() is actually a bit overambitious, returning modifiers which aren't in the state. Is this something we should fix as well, or just leave?

The reason I ask is that if we were changing xkb_state_key_get_consumed_mods() to implement GTK+ style behaviour (i.e. only declare mods consumed if they actively contributed to a transformation of that key in the current state), then we'd need to filter down the output of xkb_state_key_get_consumed_mods() in general to key_get_consumed(state, key) & state->effective_mods;

[bluetech]

I've argued in 5417440 that we should keep them. Additionally, it is easy enough for the caller to do this AND, so by not doing it ourselves the function is more useful as it provides more information. That said, right now I can't think of any scenario where knowing that a non-active modifier is consumed is useful.

BTW, looking carefully at GTK code, it is also possible to make it return non-active modifiers as consumed, though they probably didn't intend it (and doesn't trigger with existing keymaps in practice).

[fooishbar]

@bluetech Ah yes, I remember that. I don't think there's any real right answer here: the right answer is to have preserve (and thus implicitly consumed) explicitly specified in all the key types, but that boat sailed about 20 years ago. Your reasoning in 5417440 is definitely correct for that case, but not for the Ctrl+Alt case.

Given the rock vs. hard place scenario we find ourselves in, I think introducing a new entrypoint is the right thing to do, though for the new-style behaviour, we'd either need to special-case Caps+Shift ourselves, or put in HUGE SCREAMING CAPS that users really need to do that as well. (And, maybe lean on xkeyboard-config to add an explicit map[Shift+Lock] = Level1 to ALPHABETIC, though in order to do shortcuts correctly, e.g. Ctrl+Shift+W, you'd need to preserve Shift ...)

[bluetech]

@fooishbar If by special casing Caps+Shift you mean for the caps-lock capitalization transformation thing, then the new modes do not affect this, because internally we keep using the XKB mode when looking at consumed modifiers.

[fooishbar]

@bluetech OK, this looks good to me - ship it!

[bluetech]

@fooishbar Thanks for reviewing. I'll add some tests when I have time - I did not test this at all yet. Would also be nice if the GTK developers try it and see if it solves the problem?

[fooishbar]

@bluetech Sure, I'll try to get GTK+ over and see how that goes - if it works, we can throw it into git, let it bake for a short while, and then push out a 0.7.0. In the meantime, any objections to me pushing a 0.6.0.1 to include the LICENSE file? Apparently not having one hasn't been an issue before, but at least a couple of people have explicitly requested a tarball with one ...

[bluetech]

@fooishbar Sure, it must have been lost since autotools has magic only for the COPYING name. I think we should use 0.6.1 though, even if it's just a packaging fix.

[mgraesslin]

Any update on this? As you can see in the referenced commit (hey I didn't know that works) we just also run into that problem.

[bluetech]

@mgraesslin Due to backward compatibility whatever solution is going to require new API. The current proposal is in this pull request: #31 Would you consider testing it and see if it solves the problem and behaves as expected otherwise? You would need to replace calls to xkb_state_key_get_consumed_mods with xkb_state_key_get_consumed_mods2 using the XKB_CONSUMED_MODE_GTK mode. The name of the mode is, of course, debatable :)

If we can agree on the approach I can prepare the PR for merging (it is missing tests) and do a release.

Also, can you point me at the (old?) kwin code which uses Xlib? Particularly around the use of the mods_rtrn return value of XkbTranslateKeyCode() (which I assume is being used). The equivalent code in GTK was used to specify the behavior of XKB_CONSUMED_MODE_GTK.

[mgraesslin]

Would you consider testing it and see if it solves the problem and behaves as expected otherwise?

I'll give it a try next week.

The name of the mode is, of course, debatable :)

I'd suggest to change it as it's not GTK specific.

Also, can you point me at the (old?) kwin code which uses Xlib?

I cannot as the X11 code works differently. kglobalaccel uses xcb_grab_key for each of the shortcuts it's interested in. It might be that Qt has related code, though.

[mgraesslin]

I just gave it a try. Unfortunately I cannot do a 1:1 port as our code uses xkb_state_mod_index_is_consumed and for that one a new version is not available. The xkb_mod_mask_t doesn't help me as I there is no straight-forward way to get the modifiers from the mask.

[mgraesslin]

In case I got the mapping from mod_mask_t to mod_index_t correctly: patch works, my autotest passes

[bluetech]

I have updated the PR, it is ready to merge. I will wait a few days if there are any comments. I can then do a release.

@mgraesslin I have added xkb_state_mod_index_is_consumed2 as well now, so you can still keep using it if you prefer.