Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix getting wrong focus neighbor when the control is in ScrollContainer #63059

Merged
merged 1 commit into from
Jan 20, 2025
Merged

Fix getting wrong focus neighbor when the control is in ScrollContainer #63059

merged 1 commit into from
Jan 20, 2025

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Jul 16, 2022 •
edited
Loading

Now, navigating in a ScrollContainer without follow_focus enabled will not navigate to the shadow child control.
Then when navigating outside of a ScrollContainer, avoid unintended effects on navigation caused by the ScrollContainer with follow_focus enabled.

Fix #63067, fix #98445.

Before After
001 002
003 004

Works fine when follow_focus is enabled, when not enabled, there is still an issue. See #63067.

@Rindbee Rindbee requested a review from a team as a code owner July 16, 2022 05:44
@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch from d8ea7cb to 717d59d Compare July 16, 2022 06:06
@Chaosus Chaosus added this to the 4.0 milestone Jul 16, 2022
@Rindbee Rindbee marked this pull request as draft September 15, 2022 22:42
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 12, 2023
drwhut added a commit to drwhut/godot that referenced this pull request Jan 23, 2024
@drwhut
Fixed focus going outside of ScrollContainers unexpectedly.
e4a02b9 
The issue was that when the engine was deciding automatically where
the focus neighbour of a control was, it did not take ScrollContainers
into account, so controls outside of the container were technically
closer in distance to the currently focused control than the controls
within the container that were not in view.

This has being fixed by checking for ScrollContainers along the way
in determining where the 'base' control is in the current scene.
If there are no valid neighbours within the container that the focus
can move to, the function proceeds as normal, and focus goes outside
of the container.

This issue has already been reported in Godot, see:
godotengine#63055

The fix was based on this PR for the given issue:
godotengine#63059
drwhut added a commit to drwhut/godot that referenced this pull request Sep 9, 2024
@drwhut
Fixed focus going outside of ScrollContainers unexpectedly.
e18a25e 
The issue was that when the engine was deciding automatically where
the focus neighbour of a control was, it did not take ScrollContainers
into account, so controls outside of the container were technically
closer in distance to the currently focused control than the controls
within the container that were not in view.

This has being fixed by checking for ScrollContainers along the way
in determining where the 'base' control is in the current scene.
If there are no valid neighbours within the container that the focus
can move to, the function proceeds as normal, and focus goes outside
of the container.

This issue has already been reported in Godot, see:
godotengine#63055

The fix was based on this PR for the given issue:
godotengine#63059
@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch 2 times, most recently from da54ca9 to 81890b0 Compare December 31, 2024 08:50
@Rindbee Rindbee marked this pull request as ready for review December 31, 2024 08:51
@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch 10 times, most recently from c8ef806 to b94849c Compare January 3, 2025 14:07
@Rindbee Rindbee mentioned this pull request Jan 3, 2025
Closed
@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch from b94849c to 3da8582 Compare January 3, 2025 15:50
@KoBeWi
Copy link
Member

KoBeWi commented Jan 14, 2025

This breaks inspector focus. The controls inside inspector can't be navigated with keyboard, focus immediately goes outside.

@Rindbee
Copy link
Contributor Author

Rindbee commented Jan 15, 2025

This breaks inspector focus. The controls inside inspector can't be navigated with keyboard, focus immediately goes outside.

Yes, the overlapping case is taken into account, but it does not take into account lowering the priority of the parent container whose focus_mode is FOCUS_ALL. In this case, sibling nodes and their children take precedence.

@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch 3 times, most recently from 5011e21 to cc131c9 Compare January 15, 2025 01:14
@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch from cc131c9 to af3c8b1 Compare January 15, 2025 01:55
@Rindbee
Copy link
Contributor Author

Rindbee commented Jan 15, 2025

Regardless of the focus_mode of the container the control is in, the container is ignored. This is the same behavior as before.

KoBeWi
KoBeWi approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ties Control with ScrollContainer 🤔
Also it's a change in Control, so might be too risky for 4.4 now.

On the other hand, I think the fix waited long enough. Tested some stuff that could break and it works correctly.

@KoBeWi KoBeWi modified the milestones: 4.x, 4.4 Jan 15, 2025
AThousandShips
AThousandShips approved these changes Jan 15, 2025
View reviewed changes
@Rindbee
Fix getting wrong focus neighbor when the control is in `ScrollContai…
e197463 
…ner`

Exclude controls inside a `ScrollContainer` that are outside the visible area of ​​
the `ScrollContainer` when looking for focus neighbors.
@Rindbee Rindbee force-pushed the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch from af3c8b1 to e197463 Compare January 15, 2025 23:57
@Rindbee
Copy link
Contributor Author

Rindbee commented Jan 16, 2025 •
edited
Loading

Sorry, the original code allowed the control to rotate, so using the points array made sense. I ignored the situation.

Eidt:
By the way, the drawing of ScrollContainer does not take rotation into account, which is another issue, #20671 and #101622.

Edit 2:
The situation with containers is more complicated if you consider rotation, which is probably why the last pause occurred. I'm not sure it's worth doing.

@Rindbee
Copy link
Contributor Author

Rindbee commented Jan 16, 2025 •
edited
Loading

Since it uses the global rect to find overlapping controls, it will work even when rotated. A bit better than I thought. The algorithm is simpler than using an array of points.

It may not be ideal in some corner cases, but in most cases it won't make the situation worse.

master:

000.mp4

This PR:

001.mp4

Repiteo
Repiteo approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehh, this timing is pushing it, but I think we can let it slide.

@Repiteo Repiteo merged commit 4707752 into godotengine:master Jan 20, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 20, 2025

Thanks!

@Rindbee Rindbee deleted the fix-getting-wrong-focus-neighbor-in-ScrollContainer branch January 20, 2025 22:39
@Giganzo Giganzo mentioned this pull request Feb 7, 2025
Closed
@kleonc kleonc mentioned this pull request Mar 5, 2025
Open
@tetrapod00 tetrapod00 mentioned this pull request Mar 10, 2025
Open
@Rindbee Rindbee mentioned this pull request Mar 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

@Repiteo Repiteo Repiteo approved these changes

@AThousandShips AThousandShips AThousandShips approved these changes

Assignees
No one assigned
Labels
bug topic:gui
Projects
None yet
Milestone
4.4
6 participants
@Rindbee @KoBeWi @Repiteo @AThousandShips @Chaosus @YuriSizov