From e0053a5399929e8e0d4f07aa18de604ed8766ace Mon Sep 17 00:00:00 2001 From: Matt Campbell Date: Tue, 29 Oct 2024 15:01:24 -0500 Subject: [PATCH] refactor!: Rename `name` to `label` and use `value` for label content (#475) --- common/src/lib.rs | 12 ++-- consumer/src/lib.rs | 14 ++--- consumer/src/node.rs | 70 ++++++++++++---------- consumer/src/tree.rs | 14 ++--- platforms/atspi-common/src/node.rs | 6 +- platforms/macos/src/event.rs | 8 +-- platforms/macos/src/node.rs | 33 ++-------- platforms/windows/examples/hello_world.rs | 6 +- platforms/windows/src/adapter.rs | 7 ++- platforms/windows/src/node.rs | 10 +++- platforms/windows/src/tests/simple.rs | 4 +- platforms/windows/src/tests/subclassed.rs | 4 +- platforms/winit/examples/mixed_handlers.rs | 8 +-- platforms/winit/examples/simple.rs | 8 +-- 14 files changed, 102 insertions(+), 102 deletions(-) diff --git a/common/src/lib.rs b/common/src/lib.rs index 72bc7178..b9b1d061 100644 --- a/common/src/lib.rs +++ b/common/src/lib.rs @@ -784,7 +784,7 @@ enum PropertyId { PopupFor, // String - Name, + Label, Description, Value, AccessKey, @@ -1537,7 +1537,11 @@ node_id_property_methods! { } string_property_methods! { - (Name, name, set_name, clear_name), + /// The label of a control that can have a label. If the label is specified + /// via the [`Node::labelled_by`] relation, this doesn't need to be set. + /// Note that the text content of a node with the [`Role::Label`] role + /// should be provided via [`Node::value`], not this property. + (Label, label, set_label, clear_label), (Description, description, set_description, clear_description), (Value, value, set_value, clear_value), /// A single character, usually part of this node's name, that can be pressed, @@ -1982,7 +1986,7 @@ impl<'de> Visitor<'de> for PropertiesVisitor { PopupFor }, String { - Name, + Label, Description, Value, AccessKey, @@ -2129,7 +2133,7 @@ impl JsonSchema for Properties { PopupFor }, Box { - Name, + Label, Description, Value, AccessKey, diff --git a/consumer/src/lib.rs b/consumer/src/lib.rs index 6bb9e25c..fd0d9969 100644 --- a/consumer/src/lib.rs +++ b/consumer/src/lib.rs @@ -62,7 +62,7 @@ mod tests { }; let label_0_0_ignored = { let mut node = Node::new(Role::Label); - node.set_name("label_0_0_ignored"); + node.set_value("label_0_0_ignored"); node }; let paragraph_1_ignored = { @@ -83,7 +83,7 @@ mod tests { }; let button_1_0_hidden = { let mut node = Node::new(Role::Button); - node.set_name("button_1_0_hidden"); + node.set_label("button_1_0_hidden"); node.set_hidden(); node.set_children(vec![CONTAINER_1_0_0_HIDDEN_ID]); node @@ -101,12 +101,12 @@ mod tests { x1: 90.0, y1: 30.0, }); - node.set_name("label_1_1"); + node.set_value("label_1_1"); node }; let button_1_2_hidden = { let mut node = Node::new(Role::Button); - node.set_name("button_1_2_hidden"); + node.set_label("button_1_2_hidden"); node.set_hidden(); node.set_children(vec![CONTAINER_1_2_0_HIDDEN_ID]); node @@ -123,7 +123,7 @@ mod tests { }; let label_2_0 = { let mut node = Node::new(Role::Label); - node.set_name("label_2_0"); + node.set_label("label_2_0"); node }; let paragraph_3_ignored = { @@ -145,12 +145,12 @@ mod tests { }; let label_3_1_0 = { let mut node = Node::new(Role::Label); - node.set_name("label_3_1_0"); + node.set_value("label_3_1_0"); node }; let button_3_2 = { let mut node = Node::new(Role::Button); - node.set_name("button_3_2"); + node.set_label("button_3_2"); node }; let empty_container_3_3_ignored = Node::new(Role::GenericContainer); diff --git a/consumer/src/node.rs b/consumer/src/node.rs index 05d83e14..35807c61 100644 --- a/consumer/src/node.rs +++ b/consumer/src/node.rs @@ -492,15 +492,25 @@ impl<'a> Node<'a> { } } - pub fn name(&self) -> Option { - if let Some(name) = &self.data().name() { - Some(name.to_string()) + pub fn label_comes_from_value(&self) -> bool { + self.role() == Role::Label + } + + pub fn label(&self) -> Option { + if let Some(label) = &self.data().label() { + Some(label.to_string()) } else { - let names = self + let labels = self .labelled_by() - .filter_map(|node| node.name()) + .filter_map(|node| { + if node.label_comes_from_value() { + node.value() + } else { + node.label() + } + }) .collect::>(); - (!names.is_empty()).then(move || names.join(" ")) + (!labels.is_empty()).then(move || labels.join(" ")) } } @@ -916,7 +926,7 @@ mod tests { } #[test] - fn no_name_or_labelled_by() { + fn no_label_or_labelled_by() { let update = TreeUpdate { nodes: vec![ (NodeId(0), { @@ -930,11 +940,11 @@ mod tests { focus: NodeId(0), }; let tree = crate::Tree::new(update, false); - assert_eq!(None, tree.state().node_by_id(NodeId(1)).unwrap().name()); + assert_eq!(None, tree.state().node_by_id(NodeId(1)).unwrap().label()); } #[test] - fn name_from_labelled_by() { + fn label_from_labelled_by() { // The following mock UI probably isn't very localization-friendly, // but it's good for this test. const LABEL_1: &str = "Check email every"; @@ -954,7 +964,7 @@ mod tests { }), (NodeId(2), { let mut node = Node::new(Role::Label); - node.set_name(LABEL_1); + node.set_value(LABEL_1); node }), (NodeId(3), { @@ -964,7 +974,7 @@ mod tests { }), (NodeId(4), { let mut node = Node::new(Role::Label); - node.set_name(LABEL_2); + node.set_value(LABEL_2); node }), ], @@ -974,16 +984,16 @@ mod tests { let tree = crate::Tree::new(update, false); assert_eq!( Some([LABEL_1, LABEL_2].join(" ")), - tree.state().node_by_id(NodeId(1)).unwrap().name() + tree.state().node_by_id(NodeId(1)).unwrap().label() ); assert_eq!( Some(LABEL_2.into()), - tree.state().node_by_id(NodeId(3)).unwrap().name() + tree.state().node_by_id(NodeId(3)).unwrap().label() ); } #[test] - fn name_from_descendant_label() { + fn label_from_descendant_label() { const ROOT_ID: NodeId = NodeId(0); const DEFAULT_BUTTON_ID: NodeId = NodeId(1); const DEFAULT_BUTTON_LABEL_ID: NodeId = NodeId(2); @@ -1034,7 +1044,7 @@ mod tests { }), (DEFAULT_BUTTON_LABEL_ID, { let mut node = Node::new(Role::Image); - node.set_name(DEFAULT_BUTTON_LABEL); + node.set_label(DEFAULT_BUTTON_LABEL); node }), (LINK_ID, { @@ -1049,7 +1059,7 @@ mod tests { }), (LINK_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(LINK_LABEL); + node.set_value(LINK_LABEL); node }), (CHECKBOX_ID, { @@ -1059,7 +1069,7 @@ mod tests { }), (CHECKBOX_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(CHECKBOX_LABEL); + node.set_value(CHECKBOX_LABEL); node }), (RADIO_BUTTON_ID, { @@ -1069,7 +1079,7 @@ mod tests { }), (RADIO_BUTTON_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(RADIO_BUTTON_LABEL); + node.set_value(RADIO_BUTTON_LABEL); node }), (MENU_BUTTON_ID, { @@ -1079,7 +1089,7 @@ mod tests { }), (MENU_BUTTON_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(MENU_BUTTON_LABEL); + node.set_value(MENU_BUTTON_LABEL); node }), (MENU_ID, { @@ -1094,7 +1104,7 @@ mod tests { }), (MENU_ITEM_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(MENU_ITEM_LABEL); + node.set_value(MENU_ITEM_LABEL); node }), (MENU_ITEM_CHECKBOX_ID, { @@ -1104,7 +1114,7 @@ mod tests { }), (MENU_ITEM_CHECKBOX_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(MENU_ITEM_CHECKBOX_LABEL); + node.set_value(MENU_ITEM_CHECKBOX_LABEL); node }), (MENU_ITEM_RADIO_ID, { @@ -1114,7 +1124,7 @@ mod tests { }), (MENU_ITEM_RADIO_LABEL_ID, { let mut node = Node::new(Role::Label); - node.set_name(MENU_ITEM_RADIO_LABEL); + node.set_value(MENU_ITEM_RADIO_LABEL); node }), ], @@ -1124,38 +1134,38 @@ mod tests { let tree = crate::Tree::new(update, false); assert_eq!( Some(DEFAULT_BUTTON_LABEL.into()), - tree.state().node_by_id(DEFAULT_BUTTON_ID).unwrap().name() + tree.state().node_by_id(DEFAULT_BUTTON_ID).unwrap().label() ); assert_eq!( Some(LINK_LABEL.into()), - tree.state().node_by_id(LINK_ID).unwrap().name() + tree.state().node_by_id(LINK_ID).unwrap().label() ); assert_eq!( Some(CHECKBOX_LABEL.into()), - tree.state().node_by_id(CHECKBOX_ID).unwrap().name() + tree.state().node_by_id(CHECKBOX_ID).unwrap().label() ); assert_eq!( Some(RADIO_BUTTON_LABEL.into()), - tree.state().node_by_id(RADIO_BUTTON_ID).unwrap().name() + tree.state().node_by_id(RADIO_BUTTON_ID).unwrap().label() ); assert_eq!( Some(MENU_BUTTON_LABEL.into()), - tree.state().node_by_id(MENU_BUTTON_ID).unwrap().name() + tree.state().node_by_id(MENU_BUTTON_ID).unwrap().label() ); assert_eq!( Some(MENU_ITEM_LABEL.into()), - tree.state().node_by_id(MENU_ITEM_ID).unwrap().name() + tree.state().node_by_id(MENU_ITEM_ID).unwrap().label() ); assert_eq!( Some(MENU_ITEM_CHECKBOX_LABEL.into()), tree.state() .node_by_id(MENU_ITEM_CHECKBOX_ID) .unwrap() - .name() + .label() ); assert_eq!( Some(MENU_ITEM_RADIO_LABEL.into()), - tree.state().node_by_id(MENU_ITEM_RADIO_ID).unwrap().name() + tree.state().node_by_id(MENU_ITEM_RADIO_ID).unwrap().label() ); } } diff --git a/consumer/src/tree.rs b/consumer/src/tree.rs index 89f2d0be..f175474e 100644 --- a/consumer/src/tree.rs +++ b/consumer/src/tree.rs @@ -647,7 +647,7 @@ mod tests { }), (NodeId(1), { let mut node = child_node.clone(); - node.set_name("foo"); + node.set_label("foo"); node }), ], @@ -657,12 +657,12 @@ mod tests { let mut tree = super::Tree::new(first_update, false); assert_eq!( Some("foo".into()), - tree.state().node_by_id(NodeId(1)).unwrap().name() + tree.state().node_by_id(NodeId(1)).unwrap().label() ); let second_update = TreeUpdate { nodes: vec![(NodeId(1), { let mut node = child_node; - node.set_name("bar"); + node.set_label("bar"); node })], tree: None, @@ -680,8 +680,8 @@ mod tests { } fn node_updated(&mut self, old_node: &crate::Node, new_node: &crate::Node) { if new_node.id() == NodeId(1) - && old_node.name() == Some("foo".into()) - && new_node.name() == Some("bar".into()) + && old_node.label() == Some("foo".into()) + && new_node.label() == Some("bar".into()) { self.got_updated_child_node = true; return; @@ -706,7 +706,7 @@ mod tests { assert!(handler.got_updated_child_node); assert_eq!( Some("bar".into()), - tree.state().node_by_id(NodeId(1)).unwrap().name() + tree.state().node_by_id(NodeId(1)).unwrap().label() ); } @@ -725,7 +725,7 @@ mod tests { }), (NodeId(1), { let mut node = Node::new(Role::Button); - node.set_name("foo"); + node.set_label("foo"); node }), ], diff --git a/platforms/atspi-common/src/node.rs b/platforms/atspi-common/src/node.rs index edd14a3d..31518708 100644 --- a/platforms/atspi-common/src/node.rs +++ b/platforms/atspi-common/src/node.rs @@ -36,7 +36,11 @@ pub(crate) struct NodeWrapper<'a>(pub(crate) &'a Node<'a>); impl<'a> NodeWrapper<'a> { pub(crate) fn name(&self) -> Option { - self.0.name() + if self.0.label_comes_from_value() { + self.0.value() + } else { + self.0.label() + } } pub(crate) fn description(&self) -> Option { diff --git a/platforms/macos/src/event.rs b/platforms/macos/src/event.rs index e1ad7b02..af5cbe7b 100644 --- a/platforms/macos/src/event.rs +++ b/platforms/macos/src/event.rs @@ -29,7 +29,7 @@ pub(crate) enum QueuedEvent { impl QueuedEvent { fn live_region_announcement(node: &Node) -> Self { Self::Announcement { - text: node.name().unwrap(), + text: node.value().unwrap(), priority: if node.live() == Live::Assertive { NSAccessibilityPriorityLevel::NSAccessibilityPriorityHigh } else { @@ -188,7 +188,7 @@ impl TreeChangeHandler for EventGenerator { if filter(node) != FilterResult::Include { return; } - if node.name().is_some() && node.live() != Live::Off { + if node.value().is_some() && node.live() != Live::Off { self.events .push(QueuedEvent::live_region_announcement(node)); } @@ -225,9 +225,9 @@ impl TreeChangeHandler for EventGenerator { notification: unsafe { NSAccessibilitySelectedTextChangedNotification }, }); } - if new_node.name().is_some() + if new_node.value().is_some() && new_node.live() != Live::Off - && (new_node.name() != old_node.name() + && (new_node.value() != old_node.value() || new_node.live() != old_node.live() || filter(old_node) != FilterResult::Include) { diff --git a/platforms/macos/src/node.rs b/platforms/macos/src/node.rs index 98cc6fbf..8bd9209b 100644 --- a/platforms/macos/src/node.rs +++ b/platforms/macos/src/node.rs @@ -250,30 +250,13 @@ impl<'a> NodeWrapper<'a> { self.0.is_root() } - fn name(&self) -> Option { + pub(crate) fn title(&self) -> Option { if self.is_root() && self.0.role() == Role::Window { // If the group element that we expose for the top-level window // includes a title, VoiceOver behavior is broken. return None; } - self.0.name() - } - - fn node_value(&self) -> Option { - self.0.value() - } - - // TODO: implement proper logic for title and value; - // see Chromium's content/browser/accessibility/browser_accessibility_cocoa.mm - // and figure out how this is different in the macOS 10.10+ protocol - - pub(crate) fn title(&self) -> Option { - let state = self.0; - if state.role() == Role::Label && state.raw_value().is_none() { - // In this case, macOS wants the text to be the value, not title. - return None; - } - self.name() + self.0.label() } pub(crate) fn description(&self) -> Option { @@ -285,21 +268,15 @@ impl<'a> NodeWrapper<'a> { } pub(crate) fn value(&self) -> Option { - let state = self.0; - if let Some(toggled) = state.toggled() { + if let Some(toggled) = self.0.toggled() { return Some(Value::Bool(toggled != Toggled::False)); } - if let Some(value) = self.node_value() { + if let Some(value) = self.0.value() { return Some(Value::String(value)); } - if let Some(value) = state.numeric_value() { + if let Some(value) = self.0.numeric_value() { return Some(Value::Number(value)); } - if state.role() == Role::Label { - if let Some(name) = self.name() { - return Some(Value::String(name)); - } - } None } diff --git a/platforms/windows/examples/hello_world.rs b/platforms/windows/examples/hello_world.rs index 330b4bd5..2dc05f99 100644 --- a/platforms/windows/examples/hello_world.rs +++ b/platforms/windows/examples/hello_world.rs @@ -61,7 +61,7 @@ const BUTTON_2_RECT: Rect = Rect { const SET_FOCUS_MSG: u32 = WM_USER; const CLICK_MSG: u32 = WM_USER + 1; -fn build_button(id: NodeId, name: &str) -> Node { +fn build_button(id: NodeId, label: &str) -> Node { let rect = match id { BUTTON_1_ID => BUTTON_1_RECT, BUTTON_2_ID => BUTTON_2_RECT, @@ -70,7 +70,7 @@ fn build_button(id: NodeId, name: &str) -> Node { let mut node = Node::new(Role::Button); node.set_bounds(rect); - node.set_name(name); + node.set_label(label); node.add_action(Action::Focus); node.add_action(Action::Click); node @@ -78,7 +78,7 @@ fn build_button(id: NodeId, name: &str) -> Node { fn build_announcement(text: &str) -> Node { let mut node = Node::new(Role::Label); - node.set_name(text); + node.set_value(text); node.set_live(Live::Polite); node } diff --git a/platforms/windows/src/adapter.rs b/platforms/windows/src/adapter.rs index b3b7f241..b66795cd 100644 --- a/platforms/windows/src/adapter.rs +++ b/platforms/windows/src/adapter.rs @@ -90,7 +90,8 @@ impl TreeChangeHandler for AdapterChangeHandler<'_> { if filter(node) != FilterResult::Include { return; } - if node.name().is_some() && node.live() != Live::Off { + let wrapper = NodeWrapper(node); + if wrapper.name().is_some() && node.live() != Live::Off { let platform_node = PlatformNode::new(self.context, node.id()); let element: IRawElementProviderSimple = platform_node.into(); self.queue.push(QueuedEvent::Simple { @@ -112,9 +113,9 @@ impl TreeChangeHandler for AdapterChangeHandler<'_> { let old_wrapper = NodeWrapper(old_node); let new_wrapper = NodeWrapper(new_node); new_wrapper.enqueue_property_changes(&mut self.queue, &element, &old_wrapper); - if new_node.name().is_some() + if new_wrapper.name().is_some() && new_node.live() != Live::Off - && (new_node.name() != old_node.name() + && (new_wrapper.name() != old_wrapper.name() || new_node.live() != old_node.live() || filter(old_node) != FilterResult::Include) { diff --git a/platforms/windows/src/node.rs b/platforms/windows/src/node.rs index 029296a5..7a44e7c1 100644 --- a/platforms/windows/src/node.rs +++ b/platforms/windows/src/node.rs @@ -258,8 +258,12 @@ impl<'a> NodeWrapper<'a> { self.0.role_description() } - fn name(&self) -> Option { - self.0.name() + pub(crate) fn name(&self) -> Option { + if self.0.label_comes_from_value() { + self.0.value() + } else { + self.0.label() + } } fn description(&self) -> Option { @@ -328,7 +332,7 @@ impl<'a> NodeWrapper<'a> { } fn is_value_pattern_supported(&self) -> bool { - self.0.has_value() + self.0.has_value() && !self.0.label_comes_from_value() } fn is_range_value_pattern_supported(&self) -> bool { diff --git a/platforms/windows/src/tests/simple.rs b/platforms/windows/src/tests/simple.rs index 2b31a62c..382545cb 100644 --- a/platforms/windows/src/tests/simple.rs +++ b/platforms/windows/src/tests/simple.rs @@ -16,9 +16,9 @@ const WINDOW_ID: NodeId = NodeId(0); const BUTTON_1_ID: NodeId = NodeId(1); const BUTTON_2_ID: NodeId = NodeId(2); -fn make_button(name: &str) -> Node { +fn make_button(label: &str) -> Node { let mut node = Node::new(Role::Button); - node.set_name(name); + node.set_label(label); node.add_action(Action::Focus); node } diff --git a/platforms/windows/src/tests/subclassed.rs b/platforms/windows/src/tests/subclassed.rs index d9ca25ff..255f6c9b 100644 --- a/platforms/windows/src/tests/subclassed.rs +++ b/platforms/windows/src/tests/subclassed.rs @@ -25,9 +25,9 @@ const WINDOW_ID: NodeId = NodeId(0); const BUTTON_1_ID: NodeId = NodeId(1); const BUTTON_2_ID: NodeId = NodeId(2); -fn make_button(name: &str) -> Node { +fn make_button(label: &str) -> Node { let mut node = Node::new(Role::Button); - node.set_name(name); + node.set_label(label); node.add_action(Action::Focus); node } diff --git a/platforms/winit/examples/mixed_handlers.rs b/platforms/winit/examples/mixed_handlers.rs index 91cfa6da..affa58b6 100644 --- a/platforms/winit/examples/mixed_handlers.rs +++ b/platforms/winit/examples/mixed_handlers.rs @@ -36,7 +36,7 @@ const BUTTON_2_RECT: Rect = Rect { y1: 100.0, }; -fn build_button(id: NodeId, name: &str) -> Node { +fn build_button(id: NodeId, label: &str) -> Node { let rect = match id { BUTTON_1_ID => BUTTON_1_RECT, BUTTON_2_ID => BUTTON_2_RECT, @@ -45,7 +45,7 @@ fn build_button(id: NodeId, name: &str) -> Node { let mut node = Node::new(Role::Button); node.set_bounds(rect); - node.set_name(name); + node.set_label(label); node.add_action(Action::Focus); node.add_action(Action::Click); node @@ -53,7 +53,7 @@ fn build_button(id: NodeId, name: &str) -> Node { fn build_announcement(text: &str) -> Node { let mut node = Node::new(Role::Label); - node.set_name(text); + node.set_value(text); node.set_live(Live::Polite); node } @@ -77,7 +77,7 @@ impl UiState { if self.announcement.is_some() { node.push_child(ANNOUNCEMENT_ID); } - node.set_name(WINDOW_TITLE); + node.set_label(WINDOW_TITLE); node } diff --git a/platforms/winit/examples/simple.rs b/platforms/winit/examples/simple.rs index 083054d0..ec535d07 100644 --- a/platforms/winit/examples/simple.rs +++ b/platforms/winit/examples/simple.rs @@ -31,7 +31,7 @@ const BUTTON_2_RECT: Rect = Rect { y1: 100.0, }; -fn build_button(id: NodeId, name: &str) -> Node { +fn build_button(id: NodeId, label: &str) -> Node { let rect = match id { BUTTON_1_ID => BUTTON_1_RECT, BUTTON_2_ID => BUTTON_2_RECT, @@ -40,7 +40,7 @@ fn build_button(id: NodeId, name: &str) -> Node { let mut node = Node::new(Role::Button); node.set_bounds(rect); - node.set_name(name); + node.set_label(label); node.add_action(Action::Focus); node.add_action(Action::Click); node @@ -48,7 +48,7 @@ fn build_button(id: NodeId, name: &str) -> Node { fn build_announcement(text: &str) -> Node { let mut node = Node::new(Role::Label); - node.set_name(text); + node.set_value(text); node.set_live(Live::Polite); node } @@ -72,7 +72,7 @@ impl UiState { if self.announcement.is_some() { node.push_child(ANNOUNCEMENT_ID); } - node.set_name(WINDOW_TITLE); + node.set_label(WINDOW_TITLE); node }