Add a focus border on `ScrollContainer` #97521

pafuent · 2024-09-27T03:17:55Z

Also added child_has_focus function to Control. This is used by ScrollContainer to detect if one of its childs has focus and should draw the focus border.
Also added focus SceneStringName.
Also added new unit tests for Control.

Supersedes #59362

pafuent · 2024-09-27T03:20:31Z

@ator-dev, here is the re-implementation of your PR on master
If you have some spare cycles I would really appreciate if you can test it and verify that I capture properly all your intentions.

WhalesState · 2024-09-28T13:44:35Z

I like it, but it may be more useful if we can scroll the view with Ctrl + Arrow keys when the focus style is drawn ie. the ScrollContainer or one of it's children has focus, also you will need to accept the events to avoid focusing another child when the ctrl is pressed.

pafuent · 2024-09-30T11:12:45Z

I like it, but it may be more useful if we can scroll the view with Ctrl + Arrow keys when the focus style is drawn ie. the ScrollContainer or one of it's children has focus, also you will need to accept the events to avoid focusing another child when the ctrl is pressed.

Should I add this in this PR or in another?
To be honest, I tried to do it to see if the change is small and decide if I should create a new PR, but my knowledge about Godot GUI is pretty limited and the result that I got was not satisfactory.

WhalesState · 2024-09-30T12:38:02Z

Should I add this in this PR or in another? To be honest, I tried to do it to see if the change is small and decide if I should create a new PR, but my knowledge about Godot GUI is pretty limited and the result that I got was not satisfactory.

It's ok, I think i can do it later since we can't rely on gui_input() when a child is focused, this will require a new variable focused and a scroll_page to define how much we should scroll when a key is pressed and to use shortcut_input, so i don't think it will be merged to 4.x if we apply all those changes at once.

ator-dev · 2024-09-30T21:46:42Z

Great to see this discussion and thanks for the reimplementation PR!

If you have some spare cycles I would really appreciate if you can test it and verify that I capture properly all your intentions.

I'm sorry, although I mean to, I'm really busy at the moment. What's most important is that the result is accessible and doesn't affect anything weirdly.

pafuent · 2024-10-01T02:21:31Z

I'm sorry, although I mean to, I'm really busy at the moment. What's most important is that the result is accessible and doesn't affect anything weirdly.

No problem!

WhalesState · 2024-10-02T16:11:31Z

Note: The focus StyleBox is clipped for the default theme, this should be documented that it will require a focus StyleBox to be drawn without expanding and adding some content margin for the ScrollContainer panel style to avoid drawing the focus StyleBox behind the children.

Edit: I see you're not using the default focus style for the ScrollContainer, so in both cases, you will need to document this since the users will not be able to see the focus style box because it's empty when they enable this feature.

pafuent · 2024-10-03T02:29:12Z

Note: The focus StyleBox is clipped for the default theme, this should be documented that it will require a focus StyleBox to be drawn without expanding and adding some content margin for the ScrollContainer panel style to avoid drawing the focus StyleBox behind the children.

Could you please elaborate this? I'm still too new to Godot GUI.

Edit: I see you're not using the default focus style for the ScrollContainer, so in both cases, you will need to document this since the users will not be able to see the focus style box because it's empty when they enable this feature.

When you say that I should add/update documentation, Are you talking about this file, right?

I'm wondering where I could document the use set_draw_focus_border because that method is public but is not bind/exposed to Godot Engine users. It's not clear to me if this method will be available through GDExtension just because the method is public. If that is the case, Where I should document that GDExtension new method? If not, Will be enough if I add a comment in the C++ code?

scene/gui/scroll_container.cpp

scene/gui/scroll_container.h

scene/scene_string_names.cpp

scene/theme/default_theme.cpp

scene/gui/control.cpp

tests/scene/test_control.h

WhalesState · 2024-10-03T05:43:55Z

One more change is to check if (Engine::get_singleton()->is_editor_hint()) inside ScrollContainer cpp file to avoid running the code when we run a scene in editor builds.

pafuent · 2024-10-03T20:29:28Z

One more change is to check if (Engine::get_singleton()->is_editor_hint()) inside ScrollContainer cpp file to avoid running the code when we run a scene in editor builds.

Done

pafuent · 2024-10-04T02:03:17Z

@AThousandShips sorry if I tag you wrongly, but this CI job is being running for more than 5 hours. I know I can "stop" it submitting a new commit into the PR, and I'll do it soon, but I would like to let you know because you could set a configuration on GitHub or something to kill stuck CI actions.

pafuent · 2024-11-20T03:38:50Z

I think I got it working.

KoBeWi · 2024-11-20T14:31:44Z

Tested and it seems to work correctly.
ScrollContainer does not need to be focused to display the border, so I think the focus change to the inspector is not needed.
Could be nice to have some default focus style in default theme, you could copy some existing one.

doc/classes/ScrollContainer.xml

scene/gui/scroll_container.cpp

pafuent · 2024-11-20T17:10:30Z

@KoBeWi sorry but I'm not getting this:

ScrollContainer does not need to be focused to display the border, so I think the focus change to the inspector is not needed.

editor/editor_inspector.cpp

doc/classes/ScrollContainer.xml

scene/gui/scroll_container.h

KoBeWi

Overall looks good.

editor/themes/editor_theme_manager.cpp

scene/theme/default_theme.cpp

Also added new unit tests for `Control`. Co-authored-by: ator-dev <dominic.codedeveloper@gmail.com>

akien-mga · 2024-11-29T21:59:40Z

Thanks!

Rindbee · 2025-02-08T15:41:01Z

editor/editor_properties.cpp

@@ -3281,6 +3281,8 @@ void EditorPropertyResource::update_property() {
 				sub_inspector->set_read_only(is_read_only());
 				sub_inspector->set_use_folding(is_using_folding());

+				sub_inspector->set_draw_focus_border(false);


sub_inspector needs to grab focus, but not show the style? It seems that sub_inspector does not need to grab focus.

pafuent requested review from a team as code owners September 27, 2024 03:17

AThousandShips added enhancement topic:editor usability labels Sep 27, 2024

AThousandShips added this to the 4.x milestone Sep 27, 2024

pafuent force-pushed the enhance_scroll_container_focus branch from 4fc8127 to 3fd9448 Compare September 27, 2024 10:54