Allow to focus individual tabs in `TabBar`/`TabContainer` #79104

DrRevert · 2023-07-06T13:32:41Z

This is a refactor of #49928 compatible with stable releases of Godot 4, previous version was based on pre-alpha codebase. Since then few things changed: Tabs class was renamed to TabBar and TabContainer now internally uses TabBar.

Calinou · 2023-07-06T15:35:58Z

Do you have a testing project available to test this pull request? If you do, please upload it here 🙂

DrRevert · 2023-07-06T15:43:30Z

Yeah I have, but it's very bare bones
project.zip

KoBeWi · 2023-07-10T19:28:24Z

It does not work well with a controller:

godot.windows.editor.dev.x86_64_6INRTJghI8.mp4

Something like #63168 should be implemented probably.

Also there doesn't seem to be any way to disable TabContainer focus (without using script), as they internal TabBar is focusable. I wonder if we should make it easier 🤔 It'd be useful when someone implements custom tab switching with keys different than left/right.

DrRevert · 2023-07-11T07:41:53Z

It does not work well with a controller:
godot.windows.editor.dev.x86_64_6INRTJghI8.mp4

Something like #63168 should be implemented probably.

Thanks, I will look into that.

Also there doesn't seem to be any way to disable TabContainer focus (without using script), as they internal TabBar is focusable. I wonder if we should make it easier thinking It'd be useful when someone implements custom tab switching with keys different than left/right.

I think it should be possible to use Tab key to switch focus away and I vaguely remember containers not letting focus outside was a thing at least in 3.x.
As for custom switching - I could move the functionality into separate methods (bool switch_to_next_tab, bool switch_to_prev_tab - it will return true if switching was successful to know when to consume event) so implementing custom switching won't require writing the same logic to omit disabled tabs.

DrRevert · 2023-07-11T12:05:02Z

Added the fix for the gamepads based on similar implementation in #63168
Logic for changing tabs moved to separate methods and exposed.

As for the changing focus from tab_container: in my case it is possible to move the focus away using Tab key and if the active Tab contains any focusable element I can easily navigate to it using up and down. It's not possible to move focus away from container but I'm pretty sure that logic is the same for all containers.

KoBeWi · 2023-07-11T12:11:08Z

Ok I wasn't clear. I meant making the container not focusable at all, not taking away focus. TabContainer is special, because it has an internal TabBar which itself can take focus and you don't have control over it.
The problem with that is when you want to make a keyboard-only and/or controller-only UI, because all controls respond to mouse by default. This can be disabled by changing mouse filter, but it's not available for the internal TabBar of TabContainer. When someone clicks the TabBar, you will end up with a focus state that shouldn't be possible.

DrRevert · 2023-07-11T13:12:38Z

Ok I wasn't clear. I meant making the container not focusable at all, not taking away focus. TabContainer is special, because it has an internal TabBar which itself can take focus and you don't have control over it. The problem with that is when you want to make a keyboard-only and/or controller-only UI, because all controls respond to mouse by default. This can be disabled by changing mouse filter, but it's not available for the internal TabBar of TabContainer. When someone clicks the TabBar, you will end up with a focus state that shouldn't be possible.

Alright. I think exposing the focus mode of the internal TabBar to the TabContainer as parameter should do a trick, then based on the needs everyone can setup Tabs and containers the way they want.

DrRevert · 2023-07-11T16:10:23Z

Exposed methods for changing tabs in TabContainer (forgot about those earlier).
Exposed the FocusMode of the internal TabBar in the TabContainer - this will allow to opt-out from focusable tabs in container so the behavior will be the same as it is now without this change.

KoBeWi · 2023-07-11T22:24:35Z

All tabs in the editor are focusable and use the style from the default theme:

I think they shouldn't be focusable or at least editor theme needs a tab focus style.

scene/gui/tab_container.cpp

scene/gui/tab_bar.h

DrRevert · 2023-07-12T07:56:47Z

All tabs in the editor are focusable and use the style from the default theme: I think they shouldn't be focusable or at least editor theme needs a tab focus style.

I don't really see this as a problem TBH. I think that focusable tabs in editor is just another way to interact with editor without a mouse, granted many people will still use mouse but being able to just press Tab few times and navigate through entire editor UI as an alternative sounds like a good idea, unless something is broken in editor because left/right is supposed to do something else but the event is intercepted by the TabBar logic causing the tabs to be switched instead or it's too easy to switch tabs when you don't want to.

As for style I just copied the style of the button when in focus. It looks okay to me here, but I'm open to change if there is a concrete proposition how it should look like. For example: this is how tab with focus looked like in Windows 7
or maybe make it smaller similar how it's on this site?

EDIT: I took another look at the editor and just now noticed that all the focus styles are tinted blue in default editor style. ~~I think I will need some help how to properly set those up for editor styles, not sure where this is stored in the codebase.~~ Nevermind I think I got it:

As for possibly shrinking the rectangle I thought I could use negative margins but that's not possible, the change would have to be done when element is drawn, having to add some arbitrary value to the mix.

DrRevert · 2023-07-12T11:34:58Z

Focused tab in editor now uses accent color

Calinou

Tested locally (rebased on top of master 287f3aa), it works as expected.

DrRevert · 2023-08-28T11:10:28Z

If exposing the option needs more discussion, it should be removed and made default for now (since this would be consistent with other classes). We can't easily remove it later as it would break compatibility.

Ok I'll be removing the rollover parameter, however I don't think it should stay off for the time being. I'm looking at the other Control nodes to compare the behaviors and the closest I found is the ItemList which doesn't rollover/loop the input. (Side note: ItemList appears to block the input if the next element is not selectable and/or disabled, which for me is a bug.) Maybe I'm missing something, @YuriSizov could you provide me an example of the class which has a rollover behavior active (and which can't be turned off)?

YuriSizov · 2023-08-30T13:34:40Z

could you provide me an example of the class which has a rollover behavior active (and which can't be turned off)?

Honestly, I just followed @YeldhamDev's feedback on this one, sorry 🙃 I'm fine with either option that makes sense.

DrRevert · 2023-08-30T13:36:28Z

Alright. I'm finishing up the merge from master as there were some changes in editor_themes files. I should be done in few minutes.

YuriSizov · 2023-08-30T13:37:24Z

As you do, please amend the commit message to follow project's preferred style. The current title of the PR should be a good option.

DrRevert · 2023-08-30T13:45:35Z

Now it should be fine. Let me know if I miss something... again.

YuriSizov · 2023-08-30T13:52:48Z

Your commit message is slightly wrong, seems like the old name is appended to the new one :)

YuriSizov

Other than the commit message, I think it's good now.

DrRevert · 2023-08-30T13:57:29Z

Commit message fixed - hopefully didn't break anything else.

YeldhamDev · 2023-08-30T14:16:33Z

@DrRevert could you provide me an example of the class which has a rollover behavior active (and which can't be turned off)?

From the top of my head, PopupMenu. However, if you could provide examples from other GUI toolkits that allow toggling the rollover behavior in tabs, we could give in for the sake of familiarity with those tools.

DrRevert · 2023-08-30T14:24:32Z

@DrRevert could you provide me an example of the class which has a rollover behavior active (and which can't be turned off)?

From the top of my head, PopupMenu. However, if you could provide examples from other GUI toolkits that allow toggling the rollover behavior in tabs, we could give in for the sake of familiarity with those tools.

I'm not aware of any GUI toolkit allowing for such a customization.
As for PopupMenu it does make sense to have a rollover as user is expecting for this menu to stay on top other elements and should disappear entirely when user is done interacting with it. Tabs on the other hand are always present.

YeldhamDev · 2023-08-30T15:07:39Z

BTW, a reminder that one can press Shift in order to get out of a focused Control, so even having the rollover behavior always on (which seems to not be the case currently) you won't get stuck on it.

akien-mga · 2023-09-04T06:51:48Z

doc/classes/TabBar.xml

+		<method name="select_next_available">
+			<return type="bool" />
+			<description>
+				Selects first available tab on the right from the currently selected. Returns [code]true[/code] if tab selection changed.


I think UI mirroring will contradict this, as with a RTL layout the "next" tab will be to the left. So this may need to be rephrased to avoid this ambiguity (same in TabContainer). CC @bruvzg

Indeed, in RTL layout, "next" is "left".

Oh, in that case we have a bigger issue, as pressing left on keyboard or controller will go in opposite direction in RTL. I wasn't aware tabs are reordered in this way.

Suggested change

Selects first available tab on the right from the currently selected. Returns [code]true[/code] if tab selection changed.

Selects the first available tab on the right from the currently selected. Returns [code]true[/code] if tab selection changed.

Same below

Oh, in that case we have a bigger issue, as pressing left on keyboard or controller will go in opposite direction in RTL. I wasn't aware tabs are reordered in this way.

The fix should be pretty simple, you would need to check is_layout_rtl() and use the opposite direction for the target tab. It would be fine to have a bit of code repetition between the next and the previous tab method.

I'm having a little problem with rephrasing that sentence to be grammatically correct. How does
"Selects the first available tab with greater/lower index than the currently selected." sounds like? In case of lower index it sounds like the function will always select first tab ever so I'm not sure about this one.

DrRevert · 2023-09-17T13:47:54Z

I though I already submitted the change in doc description like a week ago. Sorry about the delay.
Right to left layout is now accounted for, I made the check outside the select_xxx_available for the possible top/down layout in the future.

AThousandShips · 2023-09-17T13:49:14Z

Needs a rebase

DrRevert · 2023-09-17T16:59:04Z

Rebased and modified to be compatible with 2924bfd

akien-mga

Looks good to me.

AThousandShips · 2023-09-25T14:40:24Z

doc/classes/TabBar.xml

+		<method name="select_next_available">
+			<return type="bool" />
+			<description>
+				Selects the first available tab with greater index than the currently selected. Returns [code]true[/code] if tab selection changed.


Suggested change

Selects the first available tab with greater index than the currently selected. Returns [code]true[/code] if tab selection changed.

Selects the first available tab with higher index than the currently selected. Returns [code]true[/code] if tab selection changed.

Just potential nitpick, higher/lower are more complementary

Sorry I missed your review comment while merging my last batch.

AThousandShips · 2023-09-25T14:40:41Z

doc/classes/TabContainer.xml

+		<method name="select_next_available">
+			<return type="bool" />
+			<description>
+				Selects the first available tab with greater index than the currently selected. Returns [code]true[/code] if tab selection changed.


Same here in that case

akien-mga · 2023-09-25T15:19:45Z

Thanks!

DrRevert requested review from a team as code owners July 6, 2023 13:32

DrRevert marked this pull request as draft July 6, 2023 13:34

Calinou added enhancement topic:editor usability topic:input topic:gui labels Jul 6, 2023

Calinou added this to the 4.x milestone Jul 6, 2023

DrRevert force-pushed the focusable_tabs_refactor branch from d1197b5 to 4be5836 Compare July 6, 2023 14:25

DrRevert marked this pull request as ready for review July 6, 2023 15:11

DrRevert mentioned this pull request Jul 6, 2023

Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad #49928

Closed

4 tasks

DrRevert force-pushed the focusable_tabs_refactor branch from 4be5836 to 035c18e Compare July 11, 2023 11:58

DrRevert force-pushed the focusable_tabs_refactor branch from 035c18e to 28844b7 Compare July 11, 2023 15:49

KoBeWi reviewed Jul 11, 2023

View reviewed changes

scene/gui/tab_container.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Jul 11, 2023

View reviewed changes

scene/gui/tab_bar.h Outdated Show resolved Hide resolved

DrRevert force-pushed the focusable_tabs_refactor branch from 28844b7 to f8eea52 Compare July 12, 2023 11:30

KoBeWi approved these changes Jul 12, 2023

View reviewed changes

Calinou approved these changes Jul 12, 2023

View reviewed changes

YuriSizov changed the title ~~Focusable UI tabs~~ Allow to focus individual tabs in TabBar/TabContainer Aug 30, 2023

DrRevert force-pushed the focusable_tabs_refactor branch 2 times, most recently from 7eb2614 to c5a6498 Compare August 30, 2023 13:44

YuriSizov approved these changes Aug 30, 2023

View reviewed changes

DrRevert force-pushed the focusable_tabs_refactor branch from c5a6498 to c769cd2 Compare August 30, 2023 13:56

akien-mga reviewed Sep 4, 2023

View reviewed changes

DrRevert force-pushed the focusable_tabs_refactor branch from c769cd2 to f6694b2 Compare September 5, 2023 20:07

DrRevert force-pushed the focusable_tabs_refactor branch from f6694b2 to bc5bfd0 Compare September 17, 2023 13:44

Allow to focus individual tabs in TabBar/TabContainer

18811ac

DrRevert force-pushed the focusable_tabs_refactor branch from bc5bfd0 to 18811ac Compare September 17, 2023 16:50

akien-mga approved these changes Sep 25, 2023

View reviewed changes

AThousandShips reviewed Sep 25, 2023

View reviewed changes

akien-mga merged commit c57d9f3 into godotengine:master Sep 25, 2023
15 checks passed

Calinou mentioned this pull request Nov 20, 2023

Implement navigating a TabContainer's tabs with keyboard or controller inputs #25877

Closed

YuriSizov mentioned this pull request Jan 24, 2024

Fix dock visibility issues #84122

Merged

	Selects first available tab on the right from the currently selected. Returns [code]true[/code] if tab selection changed.
	Selects the first available tab on the right from the currently selected. Returns [code]true[/code] if tab selection changed.

	Selects the first available tab with greater index than the currently selected. Returns [code]true[/code] if tab selection changed.
	Selects the first available tab with higher index than the currently selected. Returns [code]true[/code] if tab selection changed.

Allow to focus individual tabs in TabBar/TabContainer #79104

Allow to focus individual tabs in TabBar/TabContainer #79104

Conversation

DrRevert commented Jul 6, 2023

Calinou commented Jul 6, 2023

DrRevert commented Jul 6, 2023

KoBeWi commented Jul 10, 2023

DrRevert commented Jul 11, 2023 • edited Loading

DrRevert commented Jul 11, 2023

KoBeWi commented Jul 11, 2023

DrRevert commented Jul 11, 2023

DrRevert commented Jul 11, 2023

KoBeWi commented Jul 11, 2023

DrRevert commented Jul 12, 2023 • edited Loading

DrRevert commented Jul 12, 2023

Calinou left a comment

Choose a reason for hiding this comment

DrRevert commented Aug 28, 2023

YuriSizov commented Aug 30, 2023

DrRevert commented Aug 30, 2023

YuriSizov commented Aug 30, 2023

DrRevert commented Aug 30, 2023

YuriSizov commented Aug 30, 2023

YuriSizov left a comment

Choose a reason for hiding this comment

DrRevert commented Aug 30, 2023

YeldhamDev commented Aug 30, 2023

DrRevert commented Aug 30, 2023

YeldhamDev commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrRevert Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrRevert commented Sep 17, 2023

AThousandShips commented Sep 17, 2023

DrRevert commented Sep 17, 2023

akien-mga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Sep 25, 2023

Allow to focus individual tabs in `TabBar`/`TabContainer` #79104

Allow to focus individual tabs in `TabBar`/`TabContainer` #79104

DrRevert commented Jul 11, 2023 •

edited

Loading

DrRevert commented Jul 12, 2023 •

edited

Loading

DrRevert Sep 4, 2023 •

edited

Loading