[Bug] mod-tap on combo toggles mod into key-down state

[filterpaper]

Describe the Bug

PR #15847 appears to have triggered a regression with using mod-tap keys in a combo. Mod is activated and deactivated correctly when combo held down longer than tapping term. When the combo is tapped within tapping term, modifier key-down state is sent instead of the tap keycode.

Test case:

enum combos {
  modtest,
  COMBO_LENGTH
}

uint16_t COMBO_LEN = COMBO_LENGTH;
uint16_t const modtest_combo[] PROGMEM = {KC_Y, KC_U, COMBO_END};

combo_t key_combos[] = {
  [modtest] = COMBO(modtest_combo, RSFT_T(KC_SPC))
}

• KC_Y + YC_U held down longer than TAPPING_TERM: Right-Shift key-down is sent.
• KC_Y + YC_U released after TAPPING_TERM: Right-Shift key-up is sent.
• KC_Y + YC_U tapped and released under TAPPING_TERM: Expected KC_SPC but instead Right-Shift key-up followed by Right-Shift key-down event is sent to host.

Keyboard Used

No response

Link to product page (if applicable)

No response

Operating System

No response

qmk doctor Output

No response

Is AutoHotKey / Karabiner installed

• AutoHotKey (Windows)
• Karabiner (macOS)

Other keyboard-related software installed

No response

Additional Context

No response

[sigprof]

#15847 did this in the combo press code:

@@ -332,8 +332,8 @@ void apply_combo(uint16_t combo_index, combo_t *combo) {
         KEY_STATE_DOWN(state, key_index);
         if (ALL_COMBO_KEYS_ARE_DOWN(state, key_count)) {
             // this in the end executes the combo when the key_buffer is dumped.
-            record->keycode   = combo->keycode;
-            record->event.key = MAKE_KEYPOS(KEYLOC_COMBO, KEYLOC_COMBO);
+            record->keycode    = combo->keycode;
+            record->event.type = COMBO_EVENT;
 
             qrecord->combo_index = combo_index;
             ACTIVATE_COMBO(combo);

Looks like the record->event.key assignment which was removed here still needs to be made (or maybe record->event = MAKE_COMBOEVENT(true); should be used instead of patching the event fields separately, so that it always matches the combo release event). The tapping code uses KEYEQ(tapping_key.event.key, (r->event.key)) even for COMBO_EVENT (it rejects TICK_EVENT and then treats all other kinds of events the same), so a record->event.key mismatch between press and release events would break it.

[sevanteri]

Just tested adding record->event = MAKE_COMBOEVENT(true); there and it indeed seems to fix this.

[filterpaper]

The regression has also impacted combos with OSM. Test case:

enum combos {
  modtest,
  osmshift,
  COMBO_LENGTH
}

uint16_t COMBO_LEN = COMBO_LENGTH;
uint16_t const modtest_combo[] PROGMEM = {KC_Y, KC_U, COMBO_END};
uint16_t const osmshift_combo[] PROGMEM = {KC_Z, KC_X, COMBO_END};

combo_t key_combos[] = {
  [modtest] = COMBO(modtest_combo, RSFT_T(KC_SPC)),
  [osmshift] = COMBO(osmshift_combo, OSM(MOD_LSFT))
}

Regressed behaviour:

• KC_Z+KC_X tapped and released: Left-Shift key-down is sent to host and remains down

Expected behaviour:

• KC_Z+KC_X tapped and released: No report to host
• KC_I tapped and released:
  • Left-Shift key-down
  • KC_I key-down
  • Left-Shift key-up
  • KC_I key-up