Repository: slint-ui/slint (language: rust)

Issue:
Popups should expose their accessibility tree
### Bug Description

When a popup element is shown, its accessibility tree doesn't seem to be merged with the parent one. I would expect that the root element of the popup would be added as a child of the element which contains the popup.

This is not even a case of keyboard focus not moving properly, the child elements of any popup are not part of the accessibility tree, even if they are configured to be.

### Reproducible Code (if applicable)

```slint
The issue can easily be reproduced with the todo example:

1. Open Accessibility Insights for Windows
2. Run the todo example: `cargo run -p todo`
3. Click on the Close button of the example window to make the confirmation popup appear
4. Observe in Accessibility Insights that the content of the popup cannot be found
```


### Environment Details

- Slint Version: commit d6f83a2af663de2128104fe4241e8b12f308db8d
- Platform/OS: Windows 11, Windows 10
- Programming Language: Rust
- Backend/Renderer: winit


### Product Impact

I'm working on supporting combo boxes in AccessKit. Not being able to interact with the list of possible choices that appear when expanding the combo box is a blocker.


Discussion / hints:
Makes sense. I wonder how to do that with AccessKit for the case where the popup is not a top-level window (with winit).
It depends on the content of the popup: In the case of the todo example, since the popup is used to show a modal dialog, it should add a new element of role `Role::Dialog` as a child of the element containing the popup. As for combo boxes, it should add an element of type `Role::ListBox` (I'm working on this part but I can't see my changes due to this bug).

I can see that when the focus is requested inside a popup, the AccessKit adapter is notified about this but no tree update gets generated since all elements appear to have `AccessibilityRole::None`. I'm not even entirely sure which element actually gets the focus.
Ahhh I see. This makes sense but will require some additional work. The popup is not part of the same item tree (and accesskit tree), but we may need a way to make the association of the item tree of the popup belonging to a certain accessible element, so that we can build the AccessKit tree for this tree. Maybe an `accessible-popup: the_popup_name;` type of property.

Last resort is that we have a call/discussion in January in our public mattermost.
We have the `Node::owns` property which can be used to indicate that a node has children which are not semantically its descendents. But this could only work if the popup tree is contained in the main window tree. Do you mean this isn't the case currently? I cannot actually find the popup item in the descendents of the root window, but I only performed quick debugging. I'm not even sure if the root window tree is still there while a popup is open.

Fix patch:
diff --git a/internal/backends/winit/accesskit.rs b/internal/backends/winit/accesskit.rs
index ded7c676670..8fc5f134da3 100644
--- a/internal/backends/winit/accesskit.rs
+++ b/internal/backends/winit/accesskit.rs
@@ -13,8 +13,8 @@ use i_slint_core::accessibility::{
 use i_slint_core::api::Window;
 use i_slint_core::item_tree::{ItemTreeRc, ItemTreeRef, ItemTreeWeak};
 use i_slint_core::items::{ItemRc, WindowItem};
-use i_slint_core::lengths::ScaleFactor;
-use i_slint_core::window::WindowInner;
+use i_slint_core::lengths::{LogicalPoint, ScaleFactor};
+use i_slint_core::window::{PopupWindowLocation, WindowInner};
 use i_slint_core::SharedString;
 use i_slint_core::{properties::PropertyTracker, window::WindowAdapter};
 
@@ -195,7 +195,11 @@ impl AccessKitAdapter {
                         let scale_factor = ScaleFactor::new(window.scale_factor());
                         let item = self.nodes.item_rc_for_node_id(cached_node.id)?;
 
-                        let mut node = self.nodes.build_node_without_children(&item, scale_factor);
+                        let mut node = self.nodes.build_node_without_children(
+                            &item,
+                            scale_factor,
+                            Default::default(),
+                        );
 
                         node.set_children(cached_node.children.clone());
 
@@ -226,6 +230,18 @@ impl AccessKitAdapter {
     }
 }
 
+fn accessible_parent_for_item_rc(mut item: ItemRc) -> ItemRc {
+    while !item.is_accessible() {
+        if let Some(parent) = item.parent_item() {
+            item = parent;
+        } else {
+            break;
+        }
+    }
+
+    item
+}
+
 struct NodeCollection {
     next_component_id: u32,
     components_by_id: HashMap<u32, ItemTreeWeak>,
@@ -235,19 +251,7 @@ struct NodeCollection {
 }
 
 impl NodeCollection {
-    fn accessible_parent_for_item_rc(&self, mut item: ItemRc) -> ItemRc {
-        while !item.is_accessible() {
-            if let Some(parent) = item.parent_item() {
-                item = parent;
-            } else {
-                break;
-            }
-        }
-
-        item
-    }
-
-    fn focus_node(&self, window_adapter_weak: &Weak<WinitWindowAdapter>) -> NodeId {
+    fn focus_node(&mut self, window_adapter_weak: &Weak<WinitWindowAdapter>) -> NodeId {
         window_adapter_weak
             .upgrade()
             .filter(|window_adapter| {
@@ -260,7 +264,7 @@ impl NodeCollection {
                     .borrow()
                     .upgrade()
                     .map(|focus_item| {
-                        let parent = self.accessible_parent_for_item_rc(focus_item);
+                        let parent = accessible_parent_for_item_rc(focus_item);
                         parent
                             .accessible_string_property(AccessibleStringProperty::DelegateFocus)
                             .and_then(|s| s.parse::<usize>().ok())
@@ -274,7 +278,7 @@ impl NodeCollection {
                             .try_component()
                             .map(|component_rc| ItemRc::new(component_rc, 0))
                     })
-                    .and_then(|focus_item| self.find_node_id_by_item_rc(focus_item))
+                    .map(|focus_item| self.find_node_id_by_item_rc(focus_item))
             })
             .unwrap_or_else(|| self.root_node_id)
     }
@@ -286,47 +290,76 @@ impl NodeCollection {
         Some(ItemRc::new(component, index))
     }
 
-    fn find_node_id_by_item_rc(&self, mut item: ItemRc) -> Option<NodeId> {
-        item = self.accessible_parent_for_item_rc(item);
+    fn find_node_id_by_item_rc(&mut self, mut item: ItemRc) -> NodeId {
+        item = accessible_parent_for_item_rc(item);
 
         self.encode_item_node_id(&item)
     }
 
-    fn encode_item_node_id(&self, item: &ItemRc) -> Option<NodeId> {
+    fn encode_item_node_id(&mut self, item: &ItemRc) -> NodeId {
         let component = item.item_tree();
         let component_ptr = ItemTreeRef::as_ptr(ItemTreeRc::borrow(component));
-        let component_id = *(self.component_ids.get(&component_ptr)?);
+        let component_id = match self.component_ids.get(&component_ptr) {
+            Some(&component_id) => component_id,
+            None => {
+                let component_id = self.next_component_id;
+                self.next_component_id += 1;
+                self.component_ids.insert(component_ptr, component_id);
+                self.components_by_id.insert(component_id, ItemTreeRc::downgrade(component));
+                component_id
+            }
+        };
+
         let index = item.index();
-        Some(NodeId((component_id as u64) << u32::BITS | (index as u64 & u32::MAX as u64)))
+        NodeId((component_id as u64) << u32::BITS | (index as u64 & u32::MAX as u64))
     }
 
     fn build_node_for_item_recursively(
         &mut self,
         item: ItemRc,
         nodes: &mut Vec<(NodeId, Node)>,
+        popups: &[AccessiblePopup],
         scale_factor: ScaleFactor,
+        window_position: LogicalPoint,
     ) -> NodeId {
         let tracker = Box::pin(PropertyTracker::default());
 
-        let mut node =
-            tracker.as_ref().evaluate(|| self.build_node_without_children(&item, scale_factor));
+        let mut node = tracker
+            .as_ref()
+            .evaluate(|| self.build_node_without_children(&item, scale_factor, window_position));
+
+        let id = self.encode_item_node_id(&item);
+
+        let popup_child = popups.iter().find_map(|popup| {
+            if popup.parent_node != id {
+                return None;
+            }
+
+            let popup_item = ItemRc::new(popup.component.clone(), 0);
+            Some(self.build_node_for_item_recursively(
+                popup_item,
+                nodes,
+                popups,
+                scale_factor,
+                popup.location,
+            ))
+        });
 
         let children = i_slint_core::accessibility::accessible_descendents(&item)
-            .map(|child| self.build_node_for_item_recursively(child, nodes, scale_factor))
+            .map(|child| {
+                self.build_node_for_item_recursively(
+                    child,
+                    nodes,
+                    popups,
+                    scale_factor,
+                    window_position,
+                )
+            })
+            .chain(popup_child)
             .collect::<Vec<NodeId>>();
 
         node.set_children(children.clone());
 
-        let component = item.item_tree();
-        let component_ptr = ItemTreeRef::as_ptr(ItemTreeRc::borrow(component));
-        if !self.component_ids.contains_key(&component_ptr) {
-            let component_id = self.next_component_id;
-            self.next_component_id += 1;
-            self.component_ids.insert(component_ptr, component_id);
-            self.components_by_id.insert(component_id, ItemTreeRc::downgrade(component));
-        }
-
-        let id = self.encode_item_node_id(&item).unwrap();
         self.all_nodes.push(CachedNode { id, children, tracker });
 
         nodes.push((id, node));
@@ -351,6 +384,25 @@ impl NodeCollection {
 
         let root_item = ItemRc::new(window_inner.component(), 0);
 
+        let popups = window_inner
+            .active_popups()
+            .iter()
+            .filter_map(|popup| {
+                let PopupWindowLocation::ChildWindow(location) = popup.location else {
+                    return None;
+                };
+
+                let parent_item = accessible_parent_for_item_rc(popup.parent_item.upgrade()?);
+                let parent_node = self.encode_item_node_id(if parent_item.is_accessible() {
+                    &parent_item
+                } else {
+                    &root_item
+                });
+
+                Some(AccessiblePopup { location, parent_node, component: popup.component.clone() })
+            })
+            .collect::<Vec<_>>();
+
         self.all_nodes.clear();
         let mut nodes = Vec::new();
 
@@ -358,7 +410,9 @@ impl NodeCollection {
             self.build_node_for_item_recursively(
                 root_item,
                 &mut nodes,
+                &popups,
                 ScaleFactor::new(window.scale_factor()),
+                Default::default(),
             )
         });
         self.root_node_id = root_id;
@@ -370,7 +424,12 @@ impl NodeCollection {
         }
     }
 
-    fn build_node_without_children(&self, item: &ItemRc, scale_factor: ScaleFactor) -> Node {
+    fn build_node_without_children(
+        &self,
+        item: &ItemRc,
+        scale_factor: ScaleFactor,
+        window_position: LogicalPoint,
+    ) -> Node {
         let is_checkable = item
             .accessible_string_property(AccessibleStringProperty::Checkable)
             .is_some_and(|x| x == "true");
@@ -425,7 +484,7 @@ impl NodeCollection {
         }
 
         let geometry = item.geometry();
-        let absolute_origin = item.map_to_window(geometry.origin);
+        let absolute_origin = item.map_to_window(geometry.origin) + window_position.to_vector();
         let physical_origin = (absolute_origin * scale_factor).cast::<f64>();
         let physical_size = (geometry.size * scale_factor).cast::<f64>();
         node.set_bounds(accesskit::Rect {
@@ -595,3 +654,9 @@ impl DeferredAccessKitAction {
         }
     }
 }
+
+struct AccessiblePopup {
+    location: LogicalPoint,
+    parent_node: NodeId,
+    component: ItemTreeRc,
+}
diff --git a/internal/core/window.rs b/internal/core/window.rs
index cbe14c10266..4d359842ebe 100644
--- a/internal/core/window.rs
+++ b/internal/core/window.rs
@@ -373,7 +373,8 @@ impl crate::properties::PropertyDirtyHandler for WindowRedrawTracker {
 }
 
 /// This enum describes the different ways a popup can be rendered by the back-end.
-enum PopupWindowLocation {
+#[derive(Clone)]
+pub enum PopupWindowLocation {
     /// The popup is rendered in its own top-level window that is know to the windowing system.
     TopLevel(Rc<dyn WindowAdapter>),
     /// The popup is rendered as an embedded child window at the given position.
@@ -382,17 +383,20 @@ enum PopupWindowLocation {
 
 /// This structure defines a graphical element that is designed to pop up from the surrounding
 /// UI content, for example to show a context menu.
-struct PopupWindow {
+#[derive(Clone)]
+pub struct PopupWindow {
     /// The ID of the associated popup.
     popup_id: NonZeroU32,
     /// The location defines where the pop up is rendered.
-    location: PopupWindowLocation,
+    pub location: PopupWindowLocation,
     /// The component that is responsible for providing the popup content.
-    component: ItemTreeRc,
+    pub component: ItemTreeRc,
     /// Defines the close behaviour of the popup.
     close_policy: PopupClosePolicy,
     /// the item that had the focus in the parent window when the popup was opened
     focus_item_in_parent: ItemWeak,
+    /// The item from where the Popup was invoked from
+    pub parent_item: ItemWeak,
 }
 
 #[pin_project::pin_project]
@@ -552,6 +556,11 @@ impl WindowInner {
         self.component.borrow().upgrade()
     }
 
+    /// Returns a slice of the active poppups.
+    pub fn active_popups(&self) -> core::cell::Ref<'_, [PopupWindow]> {
+        core::cell::Ref::map(self.active_popups.borrow(), |v| v.as_slice())
+    }
+
     /// Receive a mouse event and pass it to the items of the component to
     /// change their state.
     ///
@@ -1103,6 +1112,7 @@ impl WindowInner {
             component: popup_componentrc.clone(),
             close_policy,
             focus_item_in_parent: focus_item,
+            parent_item: parent_item.downgrade(),
         });
 
         popup_id


Test patch: