shift-tab no longer bound to ui_focus_prev

[ndarilek]

Noticed in Beta3 and master that, while tab advances focus, shift-tab no longer works. This behavior worked in 3.1.1, and I rely on it for the accessibility plugin. It also probably breaks workflow for any other keyboard-heavy users.

Thanks for looking into this.

[ndarilek]

Looked into this briefly and it seems like the binding still exists, but it doesn't trigger. Maybe shift state isn't correctly captured?

[Calinou]

Did it work in previous 3.2 betas or alphas? Knowing this will make it easier to bisect the regression.

[ndarilek]

Not sure, I just tried Beta3 and master. I know it worked on stable. But it fails in both the editor and my own UIs, so should hopefully be easy to reproduce.

[dankan1890]

Broken between beta1 and beta1.99mono.

[ndarilek]

Are you currently running a bisect? If not, any chance you have the commit SHA for beta1? It isn't a tag, and it'd probably save me half a dozen bisect steps. Thanks.

[dankan1890]

At the moment I have no time to work on the code, the commits are:
Beta1 - 077b5f6
Beta1.99 - b7ea22c

[akien-mga]

Probably caused by #30721, CC @NilsIrl (I had the feeling that everytime anyone tried to fix this kind of issue a regression popped up..)

[ndarilek]

commit 0024dd7bb5a8a5194ed0283fc506edcd8b4a7737 (refs/bisect/bad)
Merge: cafb888361 24e1039eb6
Author: Nils ANDRÉ-CHANG <nils@nilsand.re>
Date:   Thu Sep 12 21:28:49 2019 +0100
    Merge branch 'master' into tab_key

commit cafb888361eba08297dd88b18dc71f4d418525c0 (HEAD)
Author: Nils ANDRÉ-CHANG <nils@nilsand.re>
Date:   Sat Jul 20 22:32:49 2019 +0100
    Allow tab key to be used for shortcuts

[NilsIrl]

Beat me to it 🥇

So I guess we can just revert the commit? And think about fixing the issue properly. From what I understand, is_action_pressed doesn't take into account modifiers and maybe the code that checks for CTRL+Tab is at the wrong place.

I don't mind reverting the commit but keep in mind that it introduces another bug

[NilsIrl]

The problem is that mods also is true if shift is pressed.

https://github.com/NilsIrl/godot/blob/0024dd7bb5a8a5194ed0283fc506edcd8b4a7737/scene/main/viewport.cpp#L2287-L2302

So a fix could be done that fixes both bugs (current and the one that was meant to be fixed by the #30721 )

But it still isn't a "proper" fix

[NilsIrl]

Nope, I think I got everything wrong.

Aren't the shortcut the same thing and context dependant?

Cause right now, it's one or the other and my fix didn't fix anything other than choose one over the other (might as well revert the commit).

It was actually correct the whole time I just had different bindings making me think this was incorrect.

[akien-mga]

I think I'll revert the commit for now, I feel like any fix we try right now might have potential for adding other issues.

We can then take the time to reassess this for 4.0 (and possibly a 3.2.x release if we can make a fix that does not break compatibility). Changing modifiers handling in is_action_pressed and generally reviewing their handling in InputEvent might indeed be necessary for 4.0, there have been many issues around this topic over the years.