[Bug] Weak mods not cleared properly when using Layer Tap and quicky tapping shifted keycodes.

[drashna]

Describe the Bug

Have LT(1, KC_SPACE), on layer one, have KC_COLN (shifted semicolon), and KC_LBRC (not shifted). If you hit LT, then the colon, and then any non-shifted key, it will be shifted.

Eg, this should produce: :] but will produce :}. Not every time, but if you do this fast enough, it will happen consistently.

I have tracked this down to the code change in #9941. However reverting this breaks a number of other pathways. Even partially reverting it (adding the clear weak mods to both sections).

it may be possible to fix the other features, as well, but.....

System Information

Keyboard: Moonlander, Charybdis, Ergodox EZ Revision (if applicable): N/A Operating system: MacOS, Windows qmk doctor output: Not relevant to this

Any keyboard related software installed?

• [ ] AutoHotKey (Windows)
• [ ] Karabiner (macOS)
• [ ] Other:

Additional Context

Explicitly reverting the code change from #9941 fixes the issue. But breaks other features, such as caps words.

@IsaacElenbaas @getreuer a heads up

[KarlK90]

Is this the same behavior I described in this unit-test (that is currently disabled because it fails)? https://github.com/qmk/qmk_firmware/blob/develop/tests/basic/test_action_layer.cpp#L362-L402 It sounds very familiar but might be a tad different in the root cause.

[IsaacElenbaas]

It shouldn't be that, here weak mods are not being cleared where they should.

My guess is in this case the LT delays the COLN because if the LT is released under TAPPING_TERM (without HOLD_ON_OTHER_KEYPRESS) the action will instead be a roll of SPACE and whatever is under the COLN. The LBRC press clears weak mods then evaluates the previous two keys because the LT has not been released, leaving the COLN's weak shift on for its evaluation in the same scan.

When weak mods were cleared in process_action the LBRC's handling would clear them because process_action is called for each event in the "buffer."

I considered leaving them both there in that PR instead of removing the old, that might be the best solution. I'll look into it more later.

[drashna]

@KarlK90 Nop, not that behavior. @IsaacElenbaas nailed exactly what appears to be happening, actually.

As for re-adding the origina clare weak mods creates other behavior issues too (for instance, with caps words, auto-shift, etc).

[sigprof]

So before #9941 weak mods were cleared in process_action(), which gets called after process_record_quantum(), therefore various features that have their code called from process_record_quantum() could see stale weak mods. After #9941 weak mods are cleared in action_exec(), which gets called much earlier and definitely before process_record_quantum(), but also before action_tapping_process(), therefore sometimes weak mods can be cleared too early (BTW, this is similar to what happens with process_combo_event() after the combo rework — combos handled there might be processed in a wrong order relative to dual-role keys, and the way around it is to map the combo to a custom keycode instead). Then #12471 fixed another weak mods clearing issue with tap dance (that code apparently needs its own clearing, because it gets executed in the code path for an unrelated key press event).

Maybe the correct solution is to move that clear_weak_mods() to process_record(), so that it would come after action_tapping_process() with its queue, but still before process_record_quantum()?

[KarlK90]

For now just leaving a comment that this regression should definitely be captured in a unit-test so we don't encounter it again after future refactoring.

[getreuer]

Dumb question: What do "weak mods" mean?

If there is documentation covering this, I missed it. I have been under the impression that the idea is weak mods apply to the current key only and should be cleared after that. Is this accurate? Though, I can imagine exactly when to clear is complicated with features that buffer events, combos, tap-hold keys, auto shift, and tap dance (are there others?).

It would be useful to come up with a definition of how weak mods should work. Then we can write tests to check that these event-buffering features handle weak mods as we intend.

[IsaacElenbaas]

You are correct, weak mods apply only to the "current" (they're touchy because that's hard to define) key. If keys such as KC_COLN could be sent by just telling the OS to type : instead of S+; they wouldn't be necessary AFAIK. tap_code16, SEND_STRING (?), etc. do rely on them because they are there to use though.

Weak mods are not too spread throughout code or anything, finding a correct singular place to clear them should be the end of it. The original "clearing them on keypress sounds right" was thought to be correct from 2016-2020, and I just moved them so far back that they don't get cleared late enough, not thinking of this complex case.

We do need to document how and why everything pre-process_record works, how key/mod states are different and may change between each stage, and where more complex new features should be implemented. Cleanup too, especially with Retro Shift's additions process_tapping is a 350-line nightmare and at some point we'll run into maintenance problems (E: thankfully somewhat readable now).

[real-felix]

Hey, what's the status of this bug? I experience this quite often because I use a lot of modifiers on my keyboard. For example, « (which I use often) is AltGr+W, so if want to type «o (for example), I get «œ because AltGr+O is œ.

The fact that a modifier can “bleed” to another key is really a major issue IMHO. If I can help, please let me know how so.

[TomSearle]

Is there a workaround for this? Having a keyboard that outputs the wrong key 50% of the time is a pretty rough break.

[OscarCreator]

Is there a workaround for this? Having a keyboard that outputs the wrong key 50% of the time is a pretty rough break.

You could try this. https://github.com/qmk/qmk_firmware/issues/18627#issuecomment-1475253352

[TomSearle]

Is there a workaround for this? Having a keyboard that outputs the wrong key 50% of the time is a pretty rough break.

You could try this. #18627 (comment)

Appreciate the reply but my layer key was already MO(1) (although I have tried every variation). I have now made my layer switching keys single purpose, as I previously had space on tap, and layer switch on hold. This seems to have made an appreciable difference, but it's not 100%.