fix(core): include Flickable-clipped items in tab focus traversal by amirHdev · Pull Request #10780 · slint-ui/slint

amirHdev · 2026-02-14T12:44:42Z

Summary

Tab navigation in Window::move_focus() filtered candidates using ItemRc::is_visible().
For FocusScopes inside a Flickable, off-screen items are clipped and therefore not visible, so they were skipped before focus could be assigned. This prevented keyboard users from reaching items below the fold.

This change:

Adds ItemRc::is_visible_or_clipped_by_flickable()
Uses it for FocusReason::TabNavigation in Window::move_focus()
Keeps existing behavior for other focus reasons:
- Programmatic: always allowed
- others: still require is_visible()

Once focus is accepted, existing try_scroll_into_visible() logic can reveal the newly focused item.

Tests

Added regression test:

tests/cases/issues/issue_10321_tab_focus_flickable.slint

The test verifies tab traversal visits all focus scopes inside a Flickable in order, including items initially outside the viewport (no skipping).

Validation

Executed:

RUST_BACKTRACE=1 cargo test -p test-driver-rust -- issue_10321_tab_focus_flickable

Tab navigation in Window::move_focus filtered candidates by ItemRc::is_visible(), which excluded off-screen focusable items clipped by Flickable. As a result, focus never reached those items and reveal-on-focus could not run. Use a TabNavigation-specific eligibility check that still rejects hidden items but allows candidates clipped by a Flickable ancestor, so focus can be assigned and existing try_scroll_into_visible logic can reveal them. Add a regression test for issue slint-ui#10321 that verifies tab traversal visits all focus scopes inside a Flickable, including those initially outside the viewport.

CLAassistant · 2026-02-14T12:44:48Z

All committers have signed the CLA.

LeonMatthes

Hi @amirHdev , thank you for the contribution!

This is going in a good direction, I think the test could be refactored a bit more, and this may not solve all edge cases correctly, but I think it should solve the problem in the majority of cases 👍

LeonMatthes · 2026-02-19T14:38:27Z

tests/cases/issues/issue_10321_tab_focus_flickable.slint

+        FocusScope {
+            y: 0px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 0; }
+        }
+        FocusScope {
+            y: 70px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 1; }
+        }
+        FocusScope {
+            y: 140px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 2; }
+        }
+        FocusScope {
+            y: 210px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 3; }
+        }
+        FocusScope {
+            y: 280px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 4; }
+        }
+        FocusScope {
+            y: 350px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 5; }
+        }


This should really be in a VerticalLayout with a for loop

LeonMatthes · 2026-02-19T14:43:24Z

tests/cases/issues/issue_10321_tab_focus_flickable.slint

+        FocusScope {
+            y: 0px;
+            width: parent.width;
+            height: 60px;
+            focus-gained(_) => { root.focused-index = 0; }
+        }


Suggested change

FocusScope {

y: 0px;

width: parent.width;

height: 60px;

focus-gained(_) => { root.focused-index = 0; }

}

FocusScope {

y: 0px;

width: parent.width;

height: 60px;

focus-gained(_) => { root.focused-index = 0; }

Rectangle { background: "red"; }

}

It would be nice to have differently colored rectangles in these FocusScopes so that this test can be debugged in the live-preview or slint-viewer.

LeonMatthes · 2026-02-19T14:50:06Z

tests/cases/issues/issue_10321_tab_focus_flickable.slint

+// Copyright © SixtyFPS GmbH <info@slint.dev>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
+
+export component TestCase inherits Window {


This test behaves unexpectedly when opened in the live-preview.
When pressing tab it only cycles between the first two elements in the flickable, but then focuses an item in the live-previews button bar.

It does work correctly in slint-viewer, so this could be an existing issue in our tab handling.

@amirHdev I don't think you have to necessarily fix this in this PR, but it would be great if you could give it a look.

LeonMatthes · 2026-02-19T15:06:15Z

internal/core/item_tree.rs

+            let clips_children = ancestor.borrow().as_ref().clips_children();
+            if !clips_children {
+                continue;
+            }


nit: this could be moved into the first loop to avoid looping over irrelevant ancestors twice

LeonMatthes · 2026-02-19T15:21:32Z

internal/core/item_tree.rs

+                && clip.min.x <= geometry.max.x
+                && clip.min.y <= geometry.max.y;
+            if !is_visible {
+                return ancestor.downcast::<crate::items::Flickable>().is_some();


If my understanding is correct, this loop goes outside-in from the window towards the item and stops at the first item that reduces the clip by so much that the item is no longer visible.

However, we then stop traversing there and don't check if anything else will reduce the clip even further, which may cause the item to be invisible after all. That could be a problematic edge case.
But I think that would probably be very tricky to solve fully, and is probably rather rare, so the current approach should be good-enough for now.

Refactor the regression test to use VerticalLayout with a for loop and add colored rectangles for easier visual debugging in viewer/live tooling. Also streamline ItemRc::is_visible_or_clipped_by_flickable by collecting clipping ancestors only once and iterating outside-in with cached flickable flags. When clipping makes the item invisible, continue processing to reject cases where a non-flickable clipper also applies, reducing false positives in edge cases.

amirHdev · 2026-02-19T16:14:02Z

@LeonMatthes Thanks for your feedback.
consider to review it please.

Thanks

LeonMatthes

This version unfortunately trades one issue for another, so I think it's okay if you just stick with the original version.

LeonMatthes · 2026-02-19T16:40:41Z

internal/core/item_tree.rs

+            if !is_visible {
+                if !is_flickable {
+                    return false;
+                }


This heuristic unfortunately is a little bit too simple.
Because it uses the global clip rect, instead of local coordinates, if there is any item between the flickable and the target that has clip enabled, this will immediately return false, because as soon as the clip makes the item invisible, the is_visible check will never return true again, thereby running into this branch.

You can test this by modifying the test case to add a rectangle around the focusscope that has clipping enabled:

for _[index] in [0, 1, 2, 3, 4, 5] : Rectangle { width: 100%; height: 60px; clip: true; FocusScope { width: 100%; height: 60px; focus-gained(_) => { root.focused-index = index; } Rectangle { width: 100%; height: 100%; background: index == 0 ? red : index == 1 ? orange : index == 2 ? yellow : index == 3 ? green : index == 4 ? blue : purple; } } }

The previous approach is probably good enough so you can stick with what you used before.

LeonMatthes · 2026-02-19T16:42:56Z

tests/cases/issues/issue_10321_tab_focus_flickable.slint

+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 0);
+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 1);
+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 2);
+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 3);
+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 4);
+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 5);
+slint_testing::send_keyboard_string_sequence(&instance, "\t");
+assert_eq!(instance.get_focused_index(), 0);


This should probably be a loop as well.

Suggested change

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 0);

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 1);

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 2);

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 3);

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 4);

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 5);

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 0);

for i in 0..=5 {

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), i);

}

slint_testing::send_keyboard_string_sequence(&instance, "\t");

assert_eq!(instance.get_focused_index(), 0);

Revert the stricter clip heuristic in ItemRc::is_visible_or_clipped_by_flickable to the original approach, which is less sensitive to intermediate clipped items and aligns with reviewer feedback. Update the issue slint-ui#10321 regression test to use a loop for tab traversal assertions.

LeonMatthes

Thank you for the contribution :)

LeonMatthes self-requested a review February 19, 2026 14:37

LeonMatthes reviewed Feb 19, 2026

View reviewed changes

amirHdev requested a review from LeonMatthes February 19, 2026 16:14

LeonMatthes reviewed Feb 19, 2026

View reviewed changes

amirHdev requested a review from LeonMatthes February 19, 2026 17:16

LeonMatthes enabled auto-merge (squash) February 19, 2026 18:28

LeonMatthes approved these changes Feb 19, 2026

View reviewed changes

LeonMatthes merged commit b05ff06 into slint-ui:master Feb 19, 2026
42 checks passed

Conversation

amirHdev commented Feb 14, 2026

Summary

Tests

Validation

Uh oh!

CLAassistant commented Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonMatthes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirHdev commented Feb 19, 2026

Uh oh!

LeonMatthes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonMatthes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

CLAassistant commented Feb 14, 2026 •

edited

Loading