diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c6d4f0..44816e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,9 @@ Entry format and the per-PR fragment workflow are in [`changelog.d/README.md`](c - Accessible text for data views: `DataGrid::cell_text(|row, column_id| ..)` labels each GridCell, and each Row reads its cells joined with ", "; `List::row_label(|row| ..)` labels each option. AccessKit derives no name from these nodes' children, so Orca was silent as focus moved through a grid or list. `Column::id()` returns a column's id. Used in the gallery DataGrid and List pages and in `examples/a11y_walk.rs` (WI-1848). - `SpinButton::label` sets the field's accessible name (AccessKit label), like `Textarea::label`. It is not drawn. Found by the rdpapp port (WI-1815) (WI-1918). +### Changed +- Slider rail, thumb and inner-dot sizes come from `Theme.slider` (`SliderMetrics`). A disabled slider paints the disabled alias colours for the rail, fill and thumb (issue #151). + ### Fixed - Tooltip text was only painted, so screen readers never read it. `fluent_tooltip` now sets it as the widget's AccessKit description (Fluent's `aria-describedby`), with no hover needed. A tooltip that only repeats the label adds nothing, and ribbon tooltips ("Paste\nCtrl+V") describe the control by its shortcut alone. Found by the 2026-10-20 audit (WI-1931). - KeyTips: once KeyTips are on, letters typed with Alt still held were dropped. They now continue the sequence, as in Office, and releasing Alt afterwards no longer toggles KeyTips off. Found in the 2026-10-20 upstream review (gridcraft 4ed93c3b) (WI-1927). @@ -39,6 +42,8 @@ Entry format and the per-PR fragment workflow are in [`changelog.d/README.md`](c - Dialog bodies taller than the screen scroll inside the surface. The title and actions stay visible, every control stays inside the surface (so clicks no longer land on the backdrop and close a Modal), and a control that gains focus is scrolled into view. Found by the rdpapp port (WI-1815) (WI-1912). ### Changed +- Dialogs are always modal. `DialogKind` only chooses what dismisses the surface: `Modal` closes on a backdrop click and on Escape, `Dismissible` on Escape only, `Alert` on neither. `DialogKind::NonModal` remains as a deprecated alias of `Dismissible`; it never let background input through (gh:TheDancingDeveloper-org/fluent2-egui#148). + - Dark theme: two Fluent tokens failed WCAG AA for text. `Theme::dark` now sets `status_danger_foreground_1` to `status_danger_foreground_3` (danger text was 4.16:1; Field errors, the danger ribbon tab, danger badges and message bars) and `neutral_foreground_2_brand_hover` to `neutral_foreground_2_hover` (subtle button hover label was 4.18:1). Light and high contrast keep Fluent's values (WI-1844). ## [0.2.0] - 2026-10-19 diff --git a/changelog.d/gh-147-dialog-action-ids.md b/changelog.d/gh-147-dialog-action-ids.md new file mode 100644 index 0000000..b86620b --- /dev/null +++ b/changelog.d/gh-147-dialog-action-ids.md @@ -0,0 +1,2 @@ +### Changed +- Dialog actions take a stable id and a display text (`DialogActions::primary(id, text)` and `secondary`). `CloseReason::Button` carries that id, and the button's widget id is derived from it, so two actions may share a label and a translated label still closes as the same action. This is a breaking change: call sites that passed only a label now pass an id as well, and `CloseReason` is no longer `Copy` (gh:TheDancingDeveloper-org/fluent2-egui#147). diff --git a/changelog.d/gh-151-slider-tokens.md b/changelog.d/gh-151-slider-tokens.md new file mode 100644 index 0000000..898e36d --- /dev/null +++ b/changelog.d/gh-151-slider-tokens.md @@ -0,0 +1,2 @@ +### Changed +- Slider rail, thumb and inner-dot sizes come from `Theme.slider` (`SliderMetrics`). A disabled slider paints the disabled alias colours for the rail, fill and thumb instead of the resting brand colours (issue #151). diff --git a/crates/fluent2-egui/src/controls/combobox.rs b/crates/fluent2-egui/src/controls/combobox.rs index 062d0b4..bfee4b6 100644 --- a/crates/fluent2-egui/src/controls/combobox.rs +++ b/crates/fluent2-egui/src/controls/combobox.rs @@ -4,12 +4,13 @@ //! [`super::Dropdown`]. Metrics are the WI-1549 brief: 32 px rows, a check on the selected //! option, and [`egui::ScrollArea::show_rows`] past 50 options. +use std::sync::atomic::{AtomicU64, Ordering}; + use egui::{Id, Rect, Response, Sense, TextEdit, Ui, Vec2, Widget, WidgetInfo, WidgetText, WidgetType}; use super::ButtonSize; use super::dropdown::{ - PreviewFn, field_height, filter_options, finite, icon_points, icon_size, paint_outline, rounded_u8, - show_listbox, step, + PreviewFn, field_height, finite, icon_points, icon_size, paint_outline, rounded_u8, show_listbox, step, }; use crate::foundation::focus::{focus_visible, paint_focus_ring}; use crate::foundation::icon::{IconName, IconStyle, paint_icon}; @@ -18,6 +19,71 @@ use egui::CornerRadius; use fluent2_tokens::generated::global::spacing_horizontal; +/// Options the combobox filters without copying the list. +/// +/// `revision` changes when the list changes. The filtered result is cached against it and the +/// query, so a stable list is not scanned again while the popup stays open on the same text. +pub trait ComboboxOptions { + /// How many options the list holds. + fn len(&self) -> usize; + + /// Whether the list holds no options. + fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// The option at `index`. + fn get(&self, index: usize) -> &str; + + /// A value that changes when the list changes. + fn revision(&self) -> u64; +} + +/// A borrowed option list with an explicit revision. +/// +/// Bump `revision` when the list changes so the filter cache rebuilds. [`Combobox::new`] borrows +/// a slice at revision 0 and does not copy the strings. [`ModelCombobox`] takes a model whose +/// revision can change. +pub struct SliceOptions<'a, T: AsRef> { + options: &'a [T], + revision: u64, +} + +impl<'a, T: AsRef> SliceOptions<'a, T> { + /// Borrow `options`. `revision` is the cache key for this list. + pub fn new(options: &'a [T], revision: u64) -> Self { + Self { options, revision } + } +} + +impl> ComboboxOptions for SliceOptions<'_, T> { + fn len(&self) -> usize { + self.options.len() + } + + fn get(&self, index: usize) -> &str { + self.options[index].as_ref() + } + + fn revision(&self) -> u64 { + self.revision + } +} + +/// Counts how often the combobox actually filters. Tests read it; production code leaves it +/// unset and pays nothing. +#[derive(Clone, Copy)] +pub struct FilterProbe(pub &'static AtomicU64); + +/// Filtered indexes stored on the widget id. Rebuilt only while the popup is open and the +/// query or model revision changed. +#[derive(Clone)] +struct FilterCache { + query: String, + revision: u64, + indexes: Vec, +} + /// An editable field that filters a list of options. /// /// ``` @@ -26,43 +92,68 @@ use fluent2_tokens::generated::global::spacing_horizontal; /// let _ = ui.add(Combobox::new("city", text, &["Perth", "Paris"]).freeform(true)); /// # } /// ``` -pub struct Combobox<'a> { +pub struct Combobox<'a, T: AsRef = &'a str> { id_salt: Id, text: &'a mut String, - options: Vec, + options: &'a [T], + model: Option<&'a dyn ComboboxOptions>, placeholder: Option, + label: Option, size: ButtonSize, width: Option, freeform: bool, preview: Option>, + probe: Option<&'static AtomicU64>, } -impl<'a> Combobox<'a> { +impl<'a, T: AsRef> Combobox<'a, T> { /// A combobox bound to `text`. `id_salt` names the widget. + /// + /// Borrows `options` for this frame. The strings are not copied into the widget. The cache + /// revision stays 0; use [`ModelCombobox`] with [`SliceOptions`] when the list can change + /// without the query changing. pub fn new( id_salt: impl std::hash::Hash + std::fmt::Debug, text: &'a mut String, - options: &[impl AsRef], + options: &'a [T], ) -> Self { Self { id_salt: Id::new(id_salt), text, - options: options.iter().map(|option| option.as_ref().to_owned()).collect(), + options, + model: None, placeholder: None, + label: None, size: ButtonSize::Medium, width: None, freeform: false, preview: None, + probe: None, } } - /// Grey hint while `text` is empty. + /// Count real filter passes. For tests, not application code. + #[must_use] + pub fn filter_probe(mut self, probe: FilterProbe) -> Self { + self.probe = Some(probe.0); + self + } + + /// Grey hint while `text` is empty. Not the accessible name. #[must_use] pub fn placeholder(mut self, placeholder: impl Into) -> Self { self.placeholder = Some(placeholder.into()); self } + /// Accessible name, read by screen readers. Not drawn. Inside a [`super::Field`] the + /// Field's label is the default; otherwise the placeholder, then "Combobox". + #[must_use] + pub fn label(mut self, label: impl Into) -> Self { + self.label = Some(label.into()); + self + } + /// Field height. Defaults to [`ButtonSize::Medium`]. #[must_use] pub fn size(mut self, size: ButtonSize) -> Self { @@ -77,8 +168,8 @@ impl<'a> Combobox<'a> { self } - /// When set, Enter on a typed string that matches no active option keeps the typed text. - /// Otherwise Enter only accepts an option. + /// When set, Enter keeps the typed text unless the user has moved the highlight with the + /// arrow keys since the text last changed. Otherwise Enter only accepts an option. #[must_use] pub fn freeform(mut self, freeform: bool) -> Self { self.freeform = freeform; @@ -93,7 +184,76 @@ impl<'a> Combobox<'a> { } } -impl Widget for Combobox<'_> { +/// Combobox over a reusable [`ComboboxOptions`] model. +pub struct ModelCombobox<'a> { + id_salt: Id, + text: &'a mut String, + model: &'a dyn ComboboxOptions, + placeholder: Option, + label: Option, + size: ButtonSize, + width: Option, + freeform: bool, + preview: Option>, + probe: Option<&'static AtomicU64>, +} + +impl<'a> ModelCombobox<'a> { + /// A combobox over a reusable model. The model is not copied. + pub fn new( + id_salt: impl std::hash::Hash + std::fmt::Debug, + text: &'a mut String, + model: &'a dyn ComboboxOptions, + ) -> Self { + Self { + id_salt: Id::new(id_salt), + text, + model, + placeholder: None, + label: None, + size: ButtonSize::Medium, + width: None, + freeform: false, + preview: None, + probe: None, + } + } + + /// Count real filter passes. For tests, not application code. + #[must_use] + pub fn filter_probe(mut self, probe: FilterProbe) -> Self { + self.probe = Some(probe.0); + self + } + + /// Grey hint while `text` is empty. Not the accessible name. + #[must_use] + pub fn placeholder(mut self, placeholder: impl Into) -> Self { + self.placeholder = Some(placeholder.into()); + self + } +} + +impl Widget for ModelCombobox<'_> { + fn ui(self, ui: &mut Ui) -> Response { + let combobox = Combobox { + id_salt: self.id_salt, + text: self.text, + options: &[] as &[&str], + model: Some(self.model), + placeholder: self.placeholder, + label: self.label, + size: self.size, + width: self.width, + freeform: self.freeform, + preview: self.preview, + probe: self.probe, + }; + combobox.ui(ui) + } +} + +impl> Widget for Combobox<'_, T> { fn ui(self, ui: &mut Ui) -> Response { let theme = Theme::get(ui.ctx()); let enabled = ui.is_enabled(); @@ -102,6 +262,7 @@ impl Widget for Combobox<'_> { let id = ui.make_persistent_id(self.id_salt); let open_id = id.with("open"); let active_id = id.with("active"); + let navigated_id = id.with("navigated"); let rect = ui.allocate_exact_size(Vec2::new(width, height), Sense::hover()).0; let icon_px = icon_points(self.size); @@ -140,17 +301,34 @@ impl Widget for Combobox<'_> { let mut open = ui .ctx() .data(|data| data.get_temp::(open_id).unwrap_or(false)); - let filtered = filter_options(&self.options, self.text); + let filter_id = id.with("filter"); + let view = OptionView { + slice: self.options, + model: self.model, + }; + let filtered = if open { + cached_filter(ui, filter_id, &view, self.text, self.probe) + } else { + Vec::new() + }; let mut active = ui .ctx() .data(|data| data.get_temp::(active_id)) .unwrap_or(0) .min(filtered.len().saturating_sub(1)); + // Set only by arrow keys, and cleared when the text changes, so a freeform Enter can + // tell a keyboard selection from the default highlight on the first filtered option. + let mut navigated = ui + .ctx() + .data(|data| data.get_temp::(navigated_id)) + .unwrap_or(false) + && !filtered.is_empty(); let focused = response.has_focus() && enabled; if response.changed() && enabled && !self.text.is_empty() { open = true; active = 0; + navigated = false; } if enabled && focused { let input = ui.input(|i| i.clone()); @@ -158,28 +336,39 @@ impl Widget for Combobox<'_> { open = true; } if open && input.key_pressed(egui::Key::ArrowDown) { - active = step(Some(active), filtered.len(), 1); + // The first arrow selects the highlighted option; later arrows move it. + if navigated { + active = step(Some(active), filtered.len(), 1); + } + navigated = !filtered.is_empty(); } if open && input.key_pressed(egui::Key::ArrowUp) { - active = step(Some(active), filtered.len(), -1); + if navigated { + active = step(Some(active), filtered.len(), -1); + } + navigated = !filtered.is_empty(); } if input.key_pressed(egui::Key::Escape) { open = false; } if enter { if open { - if self.freeform { - // The typed text is already in `self.text`. Keep it. - response.mark_changed(); - } else if let Some((_, label)) = filtered.get(active) { - *self.text = (*label).to_owned(); - response.mark_changed(); + let commit_active = if self.freeform { navigated } else { true }; + if commit_active { + if let Some((_, label)) = filtered.get(active) { + *self.text = (*label).to_owned(); + response.mark_changed(); + } else if !self.freeform { + self.text.clear(); + response.mark_changed(); + } } else { - self.text.clear(); + // The typed text is already in `self.text`. Keep it. response.mark_changed(); } } open = false; + navigated = false; } } @@ -195,13 +384,15 @@ impl Widget for Combobox<'_> { response.request_focus(); } if open && !response.has_focus() && !chevron_response.clicked() { - let over_popup = ui.input(|input| { - input - .pointer - .interact_pos() - .is_some_and(|pos| pos.y >= rect.bottom()) + // A key press moves the pointer's interact position off the field. Close only on + // a press that is not over the popup (WI-145 / #145). + let pressed_outside = ui.input(|input| { + let pointer = &input.pointer; + let pressed = pointer.any_pressed() || pointer.any_click(); + let over_popup = pointer.interact_pos().is_some_and(|pos| pos.y >= rect.bottom()); + pressed && !over_popup }); - if !over_popup { + if pressed_outside { open = false; } } @@ -236,12 +427,28 @@ impl Widget for Combobox<'_> { ); } - let accessible_name = if hint.is_empty() { - "Combobox".to_owned() - } else { - hint - }; + // Name, placeholder and value stay separate. TextEdit already wrote the AccessKit + // node (the placeholder as hint_text); a later widget_info does not replace it, so + // the name is set on that node. The value is the typed text, not the name. + let accessible_name = self + .label + .clone() + .filter(|label| !label.is_empty()) + .or_else(|| super::field::enclosing_field_label(ui.ctx()).filter(|label| !label.is_empty())) + .or_else(|| { + let placeholder = hint.trim(); + if placeholder.is_empty() { + None + } else { + Some(placeholder.to_owned()) + } + }) + .unwrap_or_else(|| "Combobox".to_owned()); let value_text = self.text.clone(); + ui.ctx().accesskit_node_builder(response.id, |builder| { + builder.set_label(accessible_name.clone()); + builder.set_value(value_text.clone()); + }); response.widget_info(|| { let mut info = WidgetInfo::labeled(WidgetType::ComboBox, enabled, accessible_name.clone()); info.current_text_value = Some(value_text.clone()); @@ -277,7 +484,338 @@ impl Widget for Combobox<'_> { ui.ctx().data_mut(|data| { data.insert_temp(open_id, open); data.insert_temp(active_id, active); + data.insert_temp(navigated_id, navigated); + if !open { + data.remove::(filter_id); + } }); response } } + +struct OptionView<'a, T: AsRef> { + slice: &'a [T], + model: Option<&'a dyn ComboboxOptions>, +} + +impl> ComboboxOptions for OptionView<'_, T> { + fn len(&self) -> usize { + if let Some(model) = self.model { + model.len() + } else { + self.slice.len() + } + } + + fn get(&self, index: usize) -> &str { + if let Some(model) = self.model { + model.get(index) + } else { + self.slice[index].as_ref() + } + } + + fn revision(&self) -> u64 { + self.model.map(|model| model.revision()).unwrap_or(0) + } +} + +fn cached_filter<'b>( + ui: &Ui, + filter_id: Id, + options: &'b dyn ComboboxOptions, + query: &str, + probe: Option<&'static AtomicU64>, +) -> Vec<(usize, &'b str)> { + let cached = ui.ctx().data(|data| data.get_temp::(filter_id)); + let indexes = if let Some(cache) = + cached.filter(|cache| cache.query == query && cache.revision == options.revision()) + { + cache.indexes + } else { + if let Some(probe) = probe { + probe.fetch_add(1, Ordering::Relaxed); + } + let indexes = filter_indexes(options, query); + ui.ctx().data_mut(|data| { + data.insert_temp( + filter_id, + FilterCache { + query: query.to_owned(), + revision: options.revision(), + indexes: indexes.clone(), + }, + ); + }); + indexes + }; + indexes + .into_iter() + .map(|index| (index, options.get(index))) + .collect() +} + +fn filter_indexes(options: &dyn ComboboxOptions, query: &str) -> Vec { + let count = options.len(); + if query.is_empty() { + return (0..count).collect(); + } + let query = query.to_lowercase(); + let mut prefix = Vec::new(); + let mut rest = Vec::new(); + for index in 0..count { + let label = options.get(index); + let lower = label.to_lowercase(); + if lower.starts_with(&query) { + prefix.push(index); + } else if lower.contains(&query) { + rest.push(index); + } + } + prefix.append(&mut rest); + prefix +} + +#[cfg(test)] +mod tests { + use std::cell::RefCell; + use std::rc::Rc; + + use egui_kittest::Harness; + use egui_kittest::kittest::Queryable; + + use super::Combobox; + use crate::controls::Field; + use crate::theme::{self, Theme}; + use fluent2_tokens::FLUENT; + + fn install_egui_theme(ui: &egui::Ui) { + let theme = match ui.ctx().theme() { + egui::Theme::Light => Theme::light(FLUENT), + egui::Theme::Dark => Theme::dark(FLUENT), + }; + theme::install(ui.ctx(), &theme); + } + + fn freeform_city() -> (Rc>, Harness<'static, ()>) { + let text = Rc::new(RefCell::new(String::new())); + let shown = Rc::clone(&text); + let harness = Harness::new_ui(move |ui| { + install_egui_theme(ui); + let mut value = shown.borrow().clone(); + let response = ui.add( + Combobox::new("city", &mut value, &["Perth", "Paris", "Padua"]) + .placeholder("City") + .freeform(true) + .width(160.0), + ); + response.request_focus(); + *shown.borrow_mut() = value; + }); + (text, harness) + } + + #[test] + fn freeform_enter_commits_the_arrow_key_option() { + let (text, mut harness) = freeform_city(); + harness.run(); + harness.run(); + harness.get_by_label("City").type_text("pa"); + harness.run(); + harness.key_press(egui::Key::ArrowDown); + harness.run(); + harness.key_press(egui::Key::Enter); + harness.run(); + assert_eq!(text.borrow().as_str(), "Paris"); + } + + #[test] + fn freeform_second_arrow_commits_the_next_option() { + let (text, mut harness) = freeform_city(); + harness.run(); + harness.run(); + harness.get_by_label("City").type_text("pa"); + harness.run(); + harness.key_press(egui::Key::ArrowDown); + harness.run(); + harness.key_press(egui::Key::ArrowDown); + harness.run(); + harness.key_press(egui::Key::Enter); + harness.run(); + assert_eq!(text.borrow().as_str(), "Padua"); + } + + #[test] + fn freeform_typing_after_navigation_keeps_the_text() { + let (text, mut harness) = freeform_city(); + harness.run(); + harness.run(); + harness.get_by_label("City").type_text("pa"); + harness.run(); + harness.key_press(egui::Key::ArrowDown); + harness.run(); + harness.get_by_label("City").type_text("d"); + harness.run(); + harness.key_press(egui::Key::Enter); + harness.run(); + assert_eq!(text.borrow().as_str(), "pad"); + } + + #[test] + fn field_label_names_a_combobox_without_a_placeholder() { + let mut harness = Harness::new_ui(|ui| { + install_egui_theme(ui); + let mut value = String::new(); + Field::new("Home city").show(ui, |ui| { + ui.add(Combobox::new("city", &mut value, &["Perth", "Paris"]).width(160.0)); + }); + }); + harness.run(); + harness.get_by_role_and_label(egui::accesskit::Role::ComboBox, "Home city"); + } + + #[test] + fn explicit_label_wins_over_the_field() { + let mut harness = Harness::new_ui(|ui| { + install_egui_theme(ui); + let mut value = String::new(); + Field::new("Home city").show(ui, |ui| { + ui.add( + Combobox::new("city", &mut value, &["Perth", "Paris"]) + .label("Destination") + .width(160.0), + ); + }); + }); + harness.run(); + harness.get_by_role_and_label(egui::accesskit::Role::ComboBox, "Destination"); + harness.get_by_role_and_label(egui::accesskit::Role::Label, "Home city"); + } + + use std::sync::atomic::AtomicU64; + + use super::{ComboboxOptions, FilterProbe, ModelCombobox, SliceOptions}; + + struct Counting<'a> { + options: &'a [&'a str], + revision: u64, + } + + impl ComboboxOptions for Counting<'_> { + fn len(&self) -> usize { + self.options.len() + } + + fn get(&self, index: usize) -> &str { + self.options[index] + } + + fn revision(&self) -> u64 { + self.revision + } + } + + struct ProbeState { + text: String, + options: &'static [&'static str], + revision: u64, + open: bool, + probe: &'static AtomicU64, + } + + #[test] + fn closed_popup_does_not_filter() { + static PROBE: AtomicU64 = AtomicU64::new(0); + let mut harness = Harness::new_ui_state( + |ui, state: &mut ProbeState| { + install_egui_theme(ui); + let model = Counting { + options: state.options, + revision: state.revision, + }; + ui.add( + ModelCombobox::new("city", &mut state.text, &model) + .filter_probe(FilterProbe(state.probe)), + ); + }, + ProbeState { + text: "P".to_owned(), + options: &["Perth", "Paris"], + revision: 1, + open: false, + probe: &PROBE, + }, + ); + harness.run(); + harness.run(); + assert_eq!(PROBE.load(std::sync::atomic::Ordering::Relaxed), 0); + } + + #[test] + fn filter_reruns_when_the_query_or_model_changes_and_stays_cached() { + static PROBE: AtomicU64 = AtomicU64::new(0); + let mut harness = Harness::new_ui_state( + |ui, state: &mut ProbeState| { + install_egui_theme(ui); + let model = Counting { + options: state.options, + revision: state.revision, + }; + let response = ui.add( + ModelCombobox::new("city", &mut state.text, &model) + .filter_probe(FilterProbe(state.probe)) + .placeholder("City"), + ); + if state.open { + ui.ctx() + .data_mut(|data| data.insert_temp(response.id.with("open"), true)); + } + }, + ProbeState { + text: "P".to_owned(), + options: &["Perth", "Paris", "Rome"], + revision: 1, + open: true, + probe: &PROBE, + }, + ); + harness.run(); + assert_eq!( + PROBE.load(std::sync::atomic::Ordering::Relaxed), + 1, + "opening filters once" + ); + harness.run(); + assert_eq!( + PROBE.load(std::sync::atomic::Ordering::Relaxed), + 1, + "same query stays cached" + ); + harness.state_mut().text = "Pa".to_owned(); + harness.run(); + assert_eq!( + PROBE.load(std::sync::atomic::Ordering::Relaxed), + 2, + "a new query filters again" + ); + harness.state_mut().revision = 2; + harness.state_mut().options = &["Paris", "Padua", "Rome"]; + harness.run(); + assert_eq!( + PROBE.load(std::sync::atomic::Ordering::Relaxed), + 3, + "a new revision filters again" + ); + let options = harness.state().options; + let shown = SliceOptions::new(options, harness.state().revision); + assert_eq!(shown.get(0), "Paris"); + harness.state_mut().open = false; + let before = PROBE.load(std::sync::atomic::Ordering::Relaxed); + harness.run(); + assert_eq!( + PROBE.load(std::sync::atomic::Ordering::Relaxed), + before, + "closed does not filter" + ); + } +} diff --git a/crates/fluent2-egui/src/controls/dropdown.rs b/crates/fluent2-egui/src/controls/dropdown.rs index c6308fe..5455f87 100644 --- a/crates/fluent2-egui/src/controls/dropdown.rs +++ b/crates/fluent2-egui/src/controls/dropdown.rs @@ -500,6 +500,7 @@ pub(super) fn paint_outline( } /// Options whose text contains `query`, prefix matches first. An empty query returns every option. +#[cfg(test)] pub(super) fn filter_options<'a>(options: &'a [String], query: &str) -> Vec<(usize, &'a str)> { if query.is_empty() { return options diff --git a/crates/fluent2-egui/src/controls/mod.rs b/crates/fluent2-egui/src/controls/mod.rs index 0a8ff57..22ca16b 100644 --- a/crates/fluent2-egui/src/controls/mod.rs +++ b/crates/fluent2-egui/src/controls/mod.rs @@ -109,7 +109,7 @@ pub use checkbox::{ CheckState, Checkbox, }; -pub use combobox::Combobox; +pub use combobox::{Combobox, ComboboxOptions, FilterProbe, ModelCombobox, SliceOptions}; pub use compound_button::CompoundButton; #[rustfmt::skip] pub use date_picker::{ diff --git a/crates/fluent2-egui/src/controls/slider.rs b/crates/fluent2-egui/src/controls/slider.rs index 58797cb..bed9d67 100644 --- a/crates/fluent2-egui/src/controls/slider.rs +++ b/crates/fluent2-egui/src/controls/slider.rs @@ -8,10 +8,6 @@ use egui::{Key, Pos2, Rect, Response, Sense, Stroke, Ui, Vec2, Widget, WidgetInf use crate::foundation::focus::paint_focus_ring; use crate::theme::{self, Theme}; -const RAIL: f32 = 4.0; -const THUMB: f32 = 20.0; -const DOT: f32 = 12.0; - /// A continuous or stepped slider. /// /// ``` @@ -93,16 +89,15 @@ impl Widget for Slider<'_> { let label = label .or_else(|| super::field::enclosing_field_label(ui.ctx())) .unwrap_or_else(|| "Slider".to_owned()); - let current = snap(clamp(*value, min, max), min, max, step); - *value = current; - + let metrics = theme.slider; let length = if vertical { ui.available_height().clamp(80.0, 160.0) } else { ui.available_width().clamp(120.0, 240.0) }; - // Density grows the cross axis only (44 in Touch); the thumb stays 20. - let cross = crate::theme::density_of(ui).height(THUMB); + // Density grows the cross axis only (44 in Touch); the thumb diameter stays a + // component token and is not scaled. + let cross = crate::theme::density_of(ui).height(metrics.thumb); let size = if vertical { Vec2::new(cross, length) } else { @@ -112,14 +107,17 @@ impl Widget for Slider<'_> { let enabled = ui.is_enabled(); let span = (max - min).max(f64::EPSILON); + // Snap the incoming value, then apply this frame's drag or keys before paint + // and AccessKit see it. Reading the value before input paints and reports the + // previous frame's thumb (issue #143). + let before = *value; + let mut current = snap(clamp(*value, min, max), min, max, step); + if enabled && response.dragged() { let pointer = response.interact_pointer_pos().unwrap_or(rect.center()); - let t = fraction_at(pointer, rect, vertical); - *value = snap(min + t * span, min, max, step); - response.mark_changed(); - } - - if enabled && response.has_focus() { + let t = fraction_at(pointer, rect, vertical, metrics.thumb); + current = snap(value_at(t, min, max), min, max, step); + } else if enabled && response.has_focus() { let increment = step.unwrap_or(span * 0.01); let (home, end, left, right, up, down) = ui.input(|input| { ( @@ -143,16 +141,23 @@ impl Widget for Slider<'_> { None }; if let Some(next) = next { - *value = snap(clamp(next, min, max), min, max, step); - response.mark_changed(); + current = snap(clamp(next, min, max), min, max, step); } } + *value = current; + // Changed only when the value the caller holds actually moved, including a snap + // of an out-of-range or off-step value they wrote in. A drag or key that lands + // on the same value is not a change. + if current != before { + response.mark_changed(); + } + paint( ui, &theme, rect, - fraction(current, min, span), + fraction(current, min, max), vertical, enabled, response.has_focus(), @@ -174,12 +179,21 @@ impl Widget for Slider<'_> { fn paint(ui: &Ui, theme: &Theme, rect: Rect, fraction: f32, vertical: bool, enabled: bool, focused: bool) { let color = &theme.color; let painter = ui.painter(); + let metrics = theme.slider; + let rail_size = metrics.rail; + let thumb_size = metrics.thumb; + let dot_size = metrics.dot; let rail = if vertical { - Rect::from_center_size(rect.center(), Vec2::new(RAIL, rect.height() - THUMB)) + Rect::from_center_size(rect.center(), Vec2::new(rail_size, rect.height() - thumb_size)) + } else { + Rect::from_center_size(rect.center(), Vec2::new(rect.width() - thumb_size, rail_size)) + }; + let rail_fill = if enabled { + color.neutral_stroke_accessible } else { - Rect::from_center_size(rect.center(), Vec2::new(rect.width() - THUMB, RAIL)) + color.neutral_stroke_disabled }; - painter.rect_filled(rail, RAIL / 2.0, theme::c(color.neutral_stroke_accessible)); + painter.rect_filled(rail, rail_size / 2.0, theme::c(rail_fill)); let filled = if vertical { Rect::from_min_max( Pos2::new(rail.left(), rail.bottom() - rail.height() * fraction), @@ -191,38 +205,48 @@ fn paint(ui: &Ui, theme: &Theme, rect: Rect, fraction: f32, vertical: bool, enab Pos2::new(rail.left() + rail.width() * fraction, rail.bottom()), ) }; + let brand = if enabled { + color.compound_brand_background + } else { + color.neutral_foreground_disabled + }; if fraction > 0.0 { - painter.rect_filled(filled, RAIL / 2.0, theme::c(color.compound_brand_background)); + painter.rect_filled(filled, rail_size / 2.0, theme::c(brand)); } let thumb_center = if vertical { Pos2::new(rect.center().x, rail.bottom() - rail.height() * fraction) } else { Pos2::new(rail.left() + rail.width() * fraction, rect.center().y) }; - let thumb = Rect::from_center_size(thumb_center, Vec2::splat(THUMB)); - painter.circle_filled(thumb.center(), THUMB / 2.0, theme::c(color.neutral_background_1)); + let thumb = Rect::from_center_size(thumb_center, Vec2::splat(thumb_size)); + let thumb_fill = if enabled { + color.neutral_background_1 + } else { + color.neutral_background_disabled + }; + let thumb_stroke = if enabled { + color.neutral_stroke_1 + } else { + color.neutral_stroke_disabled + }; + painter.circle_filled(thumb.center(), thumb_size / 2.0, theme::c(thumb_fill)); painter.circle_stroke( thumb.center(), - THUMB / 2.0 - 0.5, - Stroke::new(1.0, theme::c(color.neutral_stroke_1)), - ); - painter.circle_filled( - thumb.center(), - DOT / 2.0, - theme::c(color.compound_brand_background), + thumb_size / 2.0 - 0.5, + Stroke::new(1.0, theme::c(thumb_stroke)), ); + painter.circle_filled(thumb.center(), dot_size / 2.0, theme::c(brand)); if focused { - paint_focus_ring(ui, thumb, egui::CornerRadius::same((THUMB / 2.0) as u8)); + paint_focus_ring(ui, thumb, egui::CornerRadius::same((thumb_size / 2.0) as u8)); } - let _ = enabled; } -fn fraction_at(pointer: Pos2, rect: Rect, vertical: bool) -> f64 { +fn fraction_at(pointer: Pos2, rect: Rect, vertical: bool, thumb: f32) -> f64 { // The rail is inset by half a thumb on both ends, matching `paint`. let (along, length) = if vertical { - (rect.bottom() - THUMB / 2.0 - pointer.y, rect.height() - THUMB) + (rect.bottom() - thumb / 2.0 - pointer.y, rect.height() - thumb) } else { - (pointer.x - rect.left() - THUMB / 2.0, rect.width() - THUMB) + (pointer.x - rect.left() - thumb / 2.0, rect.width() - thumb) }; if length <= f32::EPSILON { return 0.0; @@ -230,8 +254,66 @@ fn fraction_at(pointer: Pos2, rect: Rect, vertical: bool) -> f64 { (along as f64 / length as f64).clamp(0.0, 1.0) } -fn fraction(value: f64, min: f64, span: f64) -> f32 { - ((value - min) / span).clamp(0.0, 1.0) as f32 +fn fraction(value: f64, min: f64, max: f64) -> f32 { + unit_position(value, min, max) as f32 +} + +/// Place `t` in `[0, 1]` onto `[min, max]` without forming `max - min`. +/// +/// `max - min` overflows to infinity for extreme finite endpoints such as +/// `-f64::MAX..=f64::MAX`, and a later division then becomes NaN. +fn value_at(t: f64, min: f64, max: f64) -> f64 { + let t = t.clamp(0.0, 1.0); + if !min.is_finite() || !max.is_finite() || min == max { + return min; + } + let mixed = min * (1.0 - t) + max * t; + if mixed.is_finite() { + return mixed.clamp(min, max); + } + // Opposite-sign extremes overflow the weighted sum. Split at zero. + if min < 0.0 && max > 0.0 { + let zero = (-min) / (max + (-min)); + if zero.is_finite() { + let placed = if t <= zero { + min * (1.0 - t / zero) + } else { + max * ((t - zero) / (1.0 - zero)) + }; + if placed.is_finite() { + return placed.clamp(min, max); + } + } + } + min +} + +/// `(value - min) / (max - min)` without a subtraction that can overflow. +fn unit_position(value: f64, min: f64, max: f64) -> f64 { + if !value.is_finite() || !min.is_finite() || !max.is_finite() || min == max { + return 0.0; + } + let value = value.clamp(min, max); + let direct = (value - min) / (max - min); + if direct.is_finite() { + return direct.clamp(0.0, 1.0); + } + if min < 0.0 && max > 0.0 { + if value <= 0.0 { + let to_zero = (-min) / (max + (-min)); + let along = if value == min { 0.0 } else { 1.0 - value / min }; + if to_zero.is_finite() && along.is_finite() { + return (to_zero * along).clamp(0.0, 1.0); + } + } else { + let from_zero = max / (max + (-min)); + let along = value / max; + if from_zero.is_finite() && along.is_finite() { + return (1.0 - from_zero + from_zero * along).clamp(0.0, 1.0); + } + } + } + 0.0 } fn clamp(value: f64, min: f64, max: f64) -> f64 { @@ -242,9 +324,121 @@ fn clamp(value: f64, min: f64, max: f64) -> f64 { } fn snap(value: f64, min: f64, max: f64, step: Option) -> f64 { - let Some(step) = step else { + let Some(step) = step.filter(|step| step.is_finite() && *step > 0.0) else { return clamp(value, min, max); }; + let value = clamp(value, min, max); let steps = ((value - min) / step).round(); - clamp(min + steps * step, min, max) + if !steps.is_finite() { + return value; + } + let snapped = min + steps * step; + if !snapped.is_finite() { + return value; + } + clamp(snapped, min, max) +} + +#[cfg(test)] +mod tests { + use super::Slider; + use egui_kittest::Harness; + use egui_kittest::kittest::Queryable; + + struct Frame { + value: f64, + reported: f64, + changed: bool, + } + + fn slider_frame(start: f64, step: Option, drive: impl FnOnce(&Harness)) -> Frame { + let mut harness = Harness::builder().build_ui_state( + |ui, frame: &mut Frame| { + let response = ui.add(Slider::new(&mut frame.value, 0.0..=100.0).step(step)); + let mut reported = f64::NAN; + ui.ctx().accesskit_node_builder(response.id, |node| { + if let Some(value) = node.numeric_value() { + reported = value; + } + }); + frame.reported = reported; + frame.changed = response.changed(); + }, + Frame { + value: start, + reported: f64::NAN, + changed: false, + }, + ); + drive(&harness); + harness.step(); + Frame { + value: harness.state().value, + reported: harness.state().reported, + changed: harness.state().changed, + } + } + + #[test] + fn arrow_key_updates_value_and_a11y_in_the_same_frame() { + let frame = slider_frame(40.0, Some(10.0), |harness| { + harness.get_by_label("Slider").focus(); + harness.key_down(egui::Key::ArrowRight); + }); + assert_eq!(frame.value, 50.0); + assert_eq!(frame.reported, 50.0); + assert!(frame.changed); + } + + #[test] + fn drag_updates_value_and_a11y_in_the_same_frame() { + let frame = slider_frame(0.0, None, |harness| { + let node = harness.get_by_label("Slider"); + let rect = node.rect(); + // Press at the rail start, then move to the far end in the same queued step. + let start = rect.left_center(); + let end = rect.right_center(); + harness.event(egui::Event::PointerMoved(start)); + harness.event(egui::Event::PointerButton { + pos: start, + button: egui::PointerButton::Primary, + pressed: true, + modifiers: egui::Modifiers::default(), + }); + harness.event(egui::Event::PointerMoved(end)); + }); + assert!(frame.value > 90.0, "dragged value {}", frame.value); + assert!((frame.reported - frame.value).abs() < 1e-9); + assert!(frame.changed); + } + + #[test] + fn unchanged_drag_does_not_mark_changed() { + // A press that does not move the pointer stays at the snapped start. + let frame = slider_frame(0.0, Some(10.0), |harness| { + let rect = harness.get_by_label("Slider").rect(); + let pos = rect.left_center(); + harness.event(egui::Event::PointerMoved(pos)); + harness.event(egui::Event::PointerButton { + pos, + button: egui::PointerButton::Primary, + pressed: true, + modifiers: egui::Modifiers::default(), + }); + }); + assert_eq!(frame.value, 0.0); + assert_eq!(frame.reported, 0.0); + assert!(!frame.changed); + } + + #[test] + fn key_that_does_not_move_does_not_mark_changed() { + let frame = slider_frame(0.0, Some(10.0), |harness| { + harness.get_by_label("Slider").focus(); + harness.key_down(egui::Key::ArrowLeft); + }); + assert_eq!(frame.value, 0.0); + assert_eq!(frame.reported, 0.0); + assert!(!frame.changed); + } } diff --git a/crates/fluent2-egui/src/data/html/data_uri.rs b/crates/fluent2-egui/src/data/html/data_uri.rs index 5403d8e..ea697a8 100644 --- a/crates/fluent2-egui/src/data/html/data_uri.rs +++ b/crates/fluent2-egui/src/data/html/data_uri.rs @@ -53,10 +53,24 @@ fn extension(mime: &str) -> &'static str { } /// Standard or URL-safe base64, ignoring whitespace (mail wraps long lines) and padding. +/// +/// The encoded length is checked before any allocation. Four encoded characters produce at most +/// three bytes, so an input whose decoded size must exceed [`MAX_BYTES`] is rejected with no +/// buffer. Whitespace is skipped, so a whitespace-heavy payload under the decoded limit still +/// decodes. The initial buffer is capped at [`MAX_BYTES`]; that cap is the decoded-byte limit, +/// not an image-pixel limit. pub(crate) fn decode_base64(input: &str) -> Option> { - let mut out = Vec::with_capacity(input.len() * 3 / 4); + // Four significant characters decode to three bytes. Reject before allocating when even a + // payload with no whitespace would exceed the decoded-byte limit. + let max_significant = MAX_BYTES.checked_mul(4)?.checked_div(3)?.saturating_add(2); + if input.len() > max_significant { + return None; + } + let capacity = input.len().checked_mul(3)?.checked_div(4)?.min(MAX_BYTES); + let mut out = Vec::with_capacity(capacity); let mut buffer: u32 = 0; let mut bits = 0; + let mut saw_padding = false; for byte in input.bytes() { let value = match byte { b'A'..=b'Z' => byte - b'A', @@ -64,7 +78,10 @@ pub(crate) fn decode_base64(input: &str) -> Option> { b'0'..=b'9' => byte - b'0' + 52, b'+' | b'-' => 62, b'/' | b'_' => 63, - b'=' => break, + b'=' => { + saw_padding = true; + break; + } b' ' | b'\n' | b'\r' | b'\t' => continue, _ => return None, }; @@ -78,6 +95,17 @@ pub(crate) fn decode_base64(input: &str) -> Option> { } } } + if saw_padding { + let mut after = input.split('=').nth(1).unwrap_or("").bytes(); + if after.any(|byte| !matches!(byte, b'=' | b' ' | b'\n' | b'\r' | b'\t')) { + return None; + } + } + // One significant character cannot complete a byte: truncated (`A`) or bad padding (`A===`). + // '=' alone never enters the sextet loop, so it stays an empty payload. + if bits == 6 { + return None; + } Some(out) } @@ -92,6 +120,52 @@ mod tests { assert_eq!(decode_base64("a$b"), None); } + #[test] + fn oversized_encoded_input_is_rejected_without_a_decoded_buffer() { + // Longer than any payload that can decode to MAX_BYTES, including padding slack. + let input = "A".repeat(MAX_BYTES.saturating_mul(4) / 3 + 3); + assert!(decode_base64(&input).is_none()); + } + + #[test] + fn whitespace_heavy_input_under_the_limit_decodes() { + let mut input = String::new(); + for (index, byte) in b"aGVsbG8=".iter().enumerate() { + if index > 0 { + input.push_str(" \n\r\t"); + } + input.push(*byte as char); + } + assert!(input.len() > 8); + assert_eq!(decode_base64(&input).as_deref(), Some(b"hello".as_slice())); + } + + #[test] + fn malformed_padding_outcomes() { + // Extra '=' stops decoding and does not change a complete payload. + assert_eq!(decode_base64("aGVsbG8===").as_deref(), Some(b"hello".as_slice())); + // Non-alphabet after padding, or a single significant character, is an error. + assert!(decode_base64("aGVsbG8=!!!!").is_none()); + assert_eq!(decode_base64("=").as_deref(), Some(b"".as_slice())); + assert!(decode_base64("A").is_none()); + assert!(decode_base64("A===").is_none()); + } + + #[test] + fn decoded_length_at_and_one_byte_over_the_limit() { + // 'A' decodes to 0x00. The largest multiple of 3 that fits in MAX_BYTES is the exact + // boundary the quartet decoder can land on. + let exact = MAX_BYTES / 3 * 3; + let at_limit = "A".repeat(exact / 3 * 4); + let decoded = decode_base64(&at_limit); + let len = decoded.as_ref().map(Vec::len); + assert_eq!(len, Some(exact)); + assert!(len.is_some_and(|len| len <= MAX_BYTES && MAX_BYTES - len < 3)); + // One more quartet is three more bytes, which crosses MAX_BYTES. + let one_over = format!("{at_limit}AAAA"); + assert!(decode_base64(&one_over).is_none()); + } + #[test] fn data_uris() { let (mime, bytes) = parse("data:image/png;base64,iVBORw0KGgo=").unwrap_or_default(); diff --git a/crates/fluent2-egui/src/lib.rs b/crates/fluent2-egui/src/lib.rs index 61e213f..79a5932 100644 --- a/crates/fluent2-egui/src/lib.rs +++ b/crates/fluent2-egui/src/lib.rs @@ -136,6 +136,7 @@ pub use surfaces::{ }; #[rustfmt::skip] pub use theme::{ + SliderMetrics, Theme, ThemeKind, }; diff --git a/crates/fluent2-egui/src/surfaces/dialog.rs b/crates/fluent2-egui/src/surfaces/dialog.rs index b8d5598..45facf1 100644 --- a/crates/fluent2-egui/src/surfaces/dialog.rs +++ b/crates/fluent2-egui/src/surfaces/dialog.rs @@ -26,32 +26,67 @@ const CHROME_ESTIMATE: f32 = 96.0; const MIN_BODY_HEIGHT: f32 = 48.0; /// How the dialog reacts to the backdrop and to Escape. +/// +/// Every dialog is modal: while one is open the backdrop blocks input to the UI behind it, +/// whatever this variant is. The variant only chooses what dismisses the dialog. `Modal` is the +/// default and is the only kind a backdrop click closes. #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub enum DialogKind { /// A backdrop click and Escape both close the dialog. The default. + /// + /// The dialog still blocks the UI behind it, as every kind does. #[default] Modal, /// Escape closes the dialog. A backdrop click does not. - NonModal, + /// + /// The dialog is still modal: the backdrop blocks input to the UI behind it. The name is the + /// dismissal policy, not a non-modal window. [`DialogKind::NonModal`] is the old name. + Dismissible, /// Only a button closes the dialog. Backdrop clicks and Escape are ignored. + /// + /// The dialog is still modal: the backdrop blocks input to the UI behind it. Alert, } +#[allow(non_upper_case_globals)] +impl DialogKind { + /// The previous name of [`DialogKind::Dismissible`]. + /// + /// It never meant the dialog let background input through. Prefer [`DialogKind::Dismissible`]. + #[deprecated( + since = "0.3.0", + note = "renamed to DialogKind::Dismissible; every dialog is modal" + )] + pub const NonModal: Self = Self::Dismissible; + + /// Whether a backdrop click closes a dialog of this kind. Only [`DialogKind::Modal`] does. + /// + /// Every kind blocks background input. This is the dismissal policy, not modality. + #[must_use] + pub const fn backdrop_dismiss(self) -> bool { + matches!(self, Self::Modal) + } +} + /// Why a dialog closed. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +/// +/// [`CloseReason::Button`] carries the action's stable id, not its display text, so two actions +/// can share a label and a translated label still closes as the same action. +#[derive(Clone, Debug, Eq, PartialEq)] pub enum CloseReason { - /// Escape, on a [`DialogKind::Modal`] or [`DialogKind::NonModal`] with no popup open. + /// Escape, on a [`DialogKind::Modal`] or [`DialogKind::Dismissible`] with no popup open. Escape, /// A click on the backdrop of a [`DialogKind::Modal`]. Backdrop, - /// An action button. The label is the button's text. - Button(&'static str), + /// An action button. The string is the id passed to [`DialogActions::primary`] or + /// [`DialogActions::secondary`], never the label. + Button(String), /// `open` was cleared by the app, or [`DialogActions::close`] was called. Programmatic, } /// Whether the dialog is still open. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum DialogResult { /// The body ran. `R` is what the body closure returned. Open(R), @@ -62,10 +97,16 @@ pub enum DialogResult { /// The action declaration closure accepted by [`Dialog::actions`]. type ActionBuild<'a> = Box) + 'a>; +/// One footer button: a stable id and the text drawn on it. +struct DialogAction { + id: String, + text: WidgetText, +} + /// Footer buttons declared by [`Dialog::actions`]. pub struct DialogActions<'a> { - primary: Option<&'static str>, - secondary: Vec<&'static str>, + primary: Option, + secondary: Vec, close_requested: bool, dialog: Id, _lifetime: std::marker::PhantomData<&'a mut Ui>, @@ -74,13 +115,23 @@ pub struct DialogActions<'a> { impl DialogActions<'_> { /// The primary action. Enter activates it unless focus is in a multiline field or on another /// button. Drawn first. - pub fn primary(&mut self, label: &'static str) { - self.primary = Some(label); + /// + /// `id` is stable across frames and languages. It is what [`CloseReason::Button`] returns and + /// what the button's widget id is derived from. `text` is only what the button shows, and may + /// be borrowed or owned. + pub fn primary(&mut self, id: impl Into, text: impl Into) { + self.primary = Some(DialogAction { + id: id.into(), + text: text.into(), + }); } - /// A secondary action, drawn after the primary. - pub fn secondary(&mut self, label: &'static str) { - self.secondary.push(label); + /// A secondary action, drawn after the primary. `id` and `text` are as for [`Self::primary`]. + pub fn secondary(&mut self, id: impl Into, text: impl Into) { + self.secondary.push(DialogAction { + id: id.into(), + text: text.into(), + }); } /// Close without a button, as [`CloseReason::Programmatic`]. @@ -89,9 +140,11 @@ impl DialogActions<'_> { } /// The egui id of a button in this dialog, for example to focus it or drive it in a test. + /// + /// `id` is the action id passed to [`Self::primary`] or [`Self::secondary`], not the label. #[must_use] - pub fn button_id(&self, label: &'static str) -> egui::Id { - self.dialog.with("action").with(label) + pub fn button_id(&self, id: impl std::hash::Hash + std::fmt::Debug) -> egui::Id { + self.dialog.with("action").with(id) } } @@ -104,8 +157,8 @@ impl DialogActions<'_> { /// .title("Save") /// .kind(DialogKind::Modal) /// .actions(|actions| { -/// actions.primary("Save"); -/// actions.secondary("Cancel"); +/// actions.primary("save", "Save"); +/// actions.secondary("cancel", "Cancel"); /// }) /// .show(ctx, open, |ui| { /// ui.label("Save this document?"); @@ -141,7 +194,10 @@ impl<'a> Dialog<'a> { self } - /// Modality. The default is [`DialogKind::Modal`]. + /// Dismissal policy. The default is [`DialogKind::Modal`]. + /// + /// Every dialog is modal and blocks background input. The kind only chooses whether a + /// backdrop click or Escape closes it. #[must_use] pub fn kind(mut self, kind: DialogKind) -> Self { self.kind = kind; @@ -259,7 +315,7 @@ impl<'a> Dialog<'a> { if let Some(build) = self.actions { build(&mut declared); } - let primary = declared.primary; + let primary_id = declared.primary.as_ref().map(|action| action.id.clone()); let shown_actions = show_actions(ui, declared, stack_actions, opening); // Measured from the layout cursor, not `min_rect`: the surface keeps last // frame's size, so its rect would feed the old height back in and never shrink. @@ -274,13 +330,13 @@ impl<'a> Dialog<'a> { // A focusable surface, so Tab has a stop even when the body has no widgets. // It is allocated last, which makes it the last stop; the title is not a stop. let surface = ui.interact(ui.min_rect(), surface_id, Sense::focusable_noninteractive()); - if opening && primary.is_none() { + if opening && primary_id.is_none() { surface.request_focus(); } let role = match self.kind { DialogKind::Alert => egui::accesskit::Role::AlertDialog, - DialogKind::Modal | DialogKind::NonModal => egui::accesskit::Role::Dialog, + DialogKind::Modal | DialogKind::Dismissible => egui::accesskit::Role::Dialog, }; ui.ctx().accesskit_node_builder(surface_id, |node| { node.set_role(role); @@ -295,13 +351,13 @@ impl<'a> Dialog<'a> { } }); - (value, primary, shown_actions) + (value, primary_id, shown_actions) }); - let (value, primary, action_state) = shown.inner; + let (value, primary_id, action_state) = shown.inner; let mut close = None; - if let Some(label) = action_state.clicked { - close = Some(CloseReason::Button(label)); + if let Some(id) = action_state.clicked { + close = Some(CloseReason::Button(id)); } if action_state.close_requested { close = Some(CloseReason::Programmatic); @@ -339,9 +395,9 @@ impl<'a> Dialog<'a> { if enter && !on_other && !ctx.text_edit_focused() - && let Some(label) = primary + && let Some(id) = primary_id { - close = Some(CloseReason::Button(label)); + close = Some(CloseReason::Button(id)); } } @@ -375,7 +431,7 @@ fn reveal_new_focus(ui: &Ui, key: Id) { } struct ActionState { - clicked: Option<&'static str>, + clicked: Option, close_requested: bool, } @@ -450,44 +506,65 @@ fn show_actions(ui: &mut Ui, actions: DialogActions<'_>, stack: bool, opening: b // Recomputed every frame: `note_button` sets it again if focus is on a non-primary button. ui.ctx() .data_mut(|data| data.insert_temp(dialog_scope.with("focus_other_button"), false)); - let labels: Vec<(&'static str, Appearance)> = actions + let buttons: Vec<(DialogAction, Appearance)> = actions .primary .into_iter() - .map(|label| (label, Appearance::Primary)) + .map(|action| (action, Appearance::Primary)) .chain( actions .secondary - .iter() - .copied() - .map(|label| (label, Appearance::Secondary)), + .into_iter() + .map(|action| (action, Appearance::Secondary)), ) .collect(); let mut clicked = None; - let primary_label = actions.primary; + let primary_id = buttons + .first() + .and_then(|(action, appearance)| (*appearance == Appearance::Primary).then(|| action.id.clone())); if stack { ui.vertical(|ui| { - for (label, appearance) in &labels { + for (action, appearance) in &buttons { let response = ui.add_sized( Vec2::new(ui.available_width(), 32.0), - Button::new(*label).appearance(*appearance).id_salt(*label), + Button::new(action.text.clone()) + .appearance(*appearance) + .id_salt(&action.id), + ); + note_button( + ui, + dialog_scope, + response.id, + &action.id, + primary_id.as_deref(), + opening, ); - note_button(ui, dialog_scope, response.id, label, primary_label, opening); if response.clicked() { - clicked = Some(*label); + clicked = Some(action.id.clone()); } } }); - } else if !labels.is_empty() { + } else if !buttons.is_empty() { // `Align::Min`, not `Center`: a centred cross axis spans all the height left in the // surface, which is last frame's height, so the row (and the surface) never shrank. // The buttons share one height, so top alignment looks the same (WI-1912). ui.with_layout(Layout::right_to_left(Align::Min), |ui| { ui.spacing_mut().item_spacing.x = spacing_horizontal::S; - for (label, appearance) in labels.iter().rev() { - let response = ui.add(Button::new(*label).appearance(*appearance).id_salt(*label)); - note_button(ui, dialog_scope, response.id, label, primary_label, opening); + for (action, appearance) in buttons.iter().rev() { + let response = ui.add( + Button::new(action.text.clone()) + .appearance(*appearance) + .id_salt(&action.id), + ); + note_button( + ui, + dialog_scope, + response.id, + &action.id, + primary_id.as_deref(), + opening, + ); if response.clicked() { - clicked = Some(*label); + clicked = Some(action.id.clone()); } } }); @@ -499,16 +576,23 @@ fn show_actions(ui: &mut Ui, actions: DialogActions<'_>, stack: bool, opening: b } /// Remember which button is focused so Enter can tell the primary from another button. -fn note_button(ui: &Ui, dialog_scope: Id, button_id: Id, label: &str, primary: Option<&str>, opening: bool) { +fn note_button( + ui: &Ui, + dialog_scope: Id, + button_id: Id, + action_id: &str, + primary: Option<&str>, + opening: bool, +) { let ctx = ui.ctx(); - if Some(label) == primary { + if Some(action_id) == primary { ctx.data_mut(|data| data.insert_temp(dialog_scope.with("primary_focus"), button_id)); if opening { ui.memory_mut(|memory| memory.request_focus(button_id)); } } let focused = ui.memory(|memory| memory.focused()) == Some(button_id); - if focused && Some(label) != primary { + if focused && Some(action_id) != primary { ctx.data_mut(|data| data.insert_temp(dialog_scope.with("focus_other_button"), true)); } } diff --git a/crates/fluent2-egui/src/surfaces/drawer.rs b/crates/fluent2-egui/src/surfaces/drawer.rs index 450b913..0f360e2 100644 --- a/crates/fluent2-egui/src/surfaces/drawer.rs +++ b/crates/fluent2-egui/src/surfaces/drawer.rs @@ -218,7 +218,7 @@ impl Drawer { if header_closed { *open = false; ctx.data_mut(|data| data.insert_temp(seen_key, false)); - return DialogResult::Closed(CloseReason::Button(CLOSE_LABEL)); + return DialogResult::Closed(CloseReason::Button(CLOSE_LABEL.to_owned())); } DialogResult::Open(value) } @@ -327,7 +327,7 @@ pub(crate) fn show_overlay( } let role = match overlay.kind { DialogKind::Alert => egui::accesskit::Role::AlertDialog, - DialogKind::Modal | DialogKind::NonModal => egui::accesskit::Role::Dialog, + DialogKind::Modal | DialogKind::Dismissible => egui::accesskit::Role::Dialog, }; ui.ctx().accesskit_node_builder(surface_id, |node| { node.set_role(role); @@ -341,7 +341,7 @@ pub(crate) fn show_overlay( let (value, header_closed) = shown.inner; let mut close = None; if header_closed { - close = Some(CloseReason::Button(CLOSE_LABEL)); + close = Some(CloseReason::Button(CLOSE_LABEL.to_owned())); } if close.is_none() && shown.backdrop_response.clicked() && overlay.kind == DialogKind::Modal { close = Some(CloseReason::Backdrop); diff --git a/crates/fluent2-egui/src/theme/mod.rs b/crates/fluent2-egui/src/theme/mod.rs index 0969072..afd1347 100644 --- a/crates/fluent2-egui/src/theme/mod.rs +++ b/crates/fluent2-egui/src/theme/mod.rs @@ -42,6 +42,35 @@ pub struct Theme { /// Compact, medium or touch sizing (`docs/specs/density.md`). Medium by default. Change it /// with [`Self::with_density`], which also resizes [`Self::data`]. pub density: Density, + /// Slider rail, thumb and inner-dot sizes. Component metrics, not Fluent global tokens. + pub slider: SliderMetrics, +} + +/// Slider sizes, in points. Defaults match the Fluent 2 web slider (4 px rail, 20 px thumb, +/// 12 px inner dot). They are component metrics: Fluent publishes no global token for them. +/// +/// ``` +/// let metrics = fluent2_egui::theme::SliderMetrics::default(); +/// assert_eq!(metrics.thumb, 20.0); +/// ``` +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct SliderMetrics { + /// Rail thickness. + pub rail: f32, + /// Thumb diameter. + pub thumb: f32, + /// Inner brand dot diameter. + pub dot: f32, +} + +impl Default for SliderMetrics { + fn default() -> Self { + Self { + rail: 4.0, + thumb: 20.0, + dot: 12.0, + } + } } /// Sizes of the Office chrome, in points. Fixed by `docs/specs/office-chrome.md` §10 (D2). @@ -207,6 +236,7 @@ impl Theme { chrome: ChromeMetrics::default(), data: DataMetrics::default(), density: Density::Medium, + slider: SliderMetrics::default(), } } @@ -221,6 +251,7 @@ impl Theme { chrome: ChromeMetrics::default(), data: DataMetrics::default(), density: Density::Medium, + slider: SliderMetrics::default(), } } @@ -235,6 +266,7 @@ impl Theme { chrome: ChromeMetrics::default(), data: DataMetrics::default(), density: Density::Medium, + slider: SliderMetrics::default(), } } diff --git a/crates/fluent2-egui/tests/a11y_keyboard.rs b/crates/fluent2-egui/tests/a11y_keyboard.rs index 26cb6d3..009c0e7 100644 --- a/crates/fluent2-egui/tests/a11y_keyboard.rs +++ b/crates/fluent2-egui/tests/a11y_keyboard.rs @@ -296,7 +296,7 @@ fn escape_returns_focus_to_a_keyboard_opener() { } let _ = Dialog::new("walk") .title("Save changes?") - .actions(|actions| actions.secondary("Cancel")) + .actions(|actions| actions.secondary("cancel", "Cancel")) .show(ui.ctx(), open, |ui| { ui.label("Body"); }); @@ -381,7 +381,7 @@ fn dialog_is_described_by_its_body_on_open() { let _ = Dialog::new("described") .title("Save changes?") .description("Your document has unsaved changes.") - .actions(|actions| actions.primary("Save")) + .actions(|actions| actions.primary("save", "Save")) .show(ui.ctx(), &mut open, |ui| { ui.label("Your document has unsaved changes."); }); diff --git a/crates/fluent2-egui/tests/dialog.rs b/crates/fluent2-egui/tests/dialog.rs index 52849b2..5167d0a 100644 --- a/crates/fluent2-egui/tests/dialog.rs +++ b/crates/fluent2-egui/tests/dialog.rs @@ -7,7 +7,7 @@ use std::rc::Rc; use egui::accesskit::Role; use egui_kittest::Harness; -use egui_kittest::kittest::Queryable; +use egui_kittest::kittest::{NodeT, Queryable}; use fluent2_egui::controls::{Button, Dropdown}; use fluent2_egui::surfaces::{CloseReason, Dialog, DialogKind, DialogResult}; use fluent2_egui::{Theme, install}; @@ -17,6 +17,8 @@ use fluent2_egui::{Theme, install}; struct Probe { open: bool, closed_with: Option, + /// Clicks that reached a control on the page behind the dialog. + behind_clicks: u32, } /// Store the dialog's open flag. The close reason is kept only on the frame that closed it: @@ -46,8 +48,8 @@ fn dialog_harness(probe: Rc>, kind: DialogKind) -> Harness<'stati .kind(kind) .width(600.0) .actions(|actions| { - actions.primary("Save"); - actions.secondary("Cancel"); + actions.primary("save", "Save"); + actions.secondary("cancel", "Cancel"); }) .show(ui.ctx(), &mut open, |ui| { ui.label("Body"); @@ -61,6 +63,7 @@ fn open_probe() -> Rc> { Rc::new(RefCell::new(Probe { open: true, closed_with: None, + behind_clicks: 0, })) } @@ -107,7 +110,58 @@ fn enter_triggers_primary() { harness.key_press(egui::Key::Enter); harness.run(); assert!(!probe.borrow().open, "Enter should trigger the primary action"); - assert_eq!(probe.borrow().closed_with, Some(CloseReason::Button("Save"))); + assert_eq!( + probe.borrow().closed_with, + Some(CloseReason::Button("save".to_owned())) + ); +} + +#[test] +fn same_label_actions_are_distinct_and_survive_a_language_switch() { + let label = Rc::new(RefCell::new("Save".to_owned())); + let probe = open_probe(); + let label_for_ui = label.clone(); + let probe_for_ui = probe.clone(); + let mut harness = Harness::new_ui(move |ui| { + install_light(ui); + let text = label_for_ui.borrow().clone(); + let mut open = probe_for_ui.borrow().open; + let was_open = open; + let result = Dialog::new("duplicate_labels") + .title("Save?") + .actions(|actions| { + actions.primary("save", text.clone()); + actions.secondary("discard", text); + }) + .show(ui.ctx(), &mut open, |_ui| {}); + record(&probe_for_ui, was_open, open, result); + }); + harness.run(); + let english: Vec<_> = harness + .get_all_by_label("Save") + .map(|node| node.accesskit_node().id()) + .collect(); + assert_eq!(english.len(), 2, "both actions show the same label"); + assert_ne!( + english[0], english[1], + "widget ids come from the action id, not the label" + ); + + *label.borrow_mut() = "Speichern".to_owned(); + harness.run(); + let translated: Vec<_> = harness + .get_all_by_label("Speichern") + .map(|node| node.accesskit_node().id()) + .collect(); + assert_eq!(translated.len(), 2, "both buttons show the translated label"); + assert_eq!(translated, english, "a language switch keeps the widget ids"); + harness.key_press(egui::Key::Enter); + harness.run(); + assert_eq!( + probe.borrow().closed_with, + Some(CloseReason::Button("save".to_owned())), + "the close result is the action id, not the translated label" + ); } #[test] @@ -130,7 +184,10 @@ fn enter_on_another_button_does_not_trigger_primary() { harness.key_press(egui::Key::Enter); harness.run(); assert!(!probe.borrow().open, "Cancel should close the dialog"); - assert_eq!(probe.borrow().closed_with, Some(CloseReason::Button("Cancel"))); + assert_eq!( + probe.borrow().closed_with, + Some(CloseReason::Button("cancel".to_owned())) + ); } /// Which focus stop of the dialog has focus: one of Save, Cancel, Inside, or the surface. @@ -215,7 +272,7 @@ fn focus_returns_to_opener() { } let result = Dialog::new("dialog") .title("Dialog title") - .actions(|actions| actions.secondary("Cancel")) + .actions(|actions| actions.secondary("cancel", "Cancel")) .show(ui.ctx(), &mut open, |ui| { ui.label("Body"); }); @@ -259,8 +316,8 @@ fn dropdown_inside_dialog_escapes_first() { let result = Dialog::new("dialog") .title("Dialog title") .actions(|actions| { - actions.primary("Save"); - actions.secondary("Cancel"); + actions.primary("save", "Save"); + actions.secondary("cancel", "Cancel"); }) .show(ui.ctx(), &mut open, |ui| { let mut choice = *selected.borrow(); @@ -308,8 +365,8 @@ fn tall_harness(probe: Rc>, rows: usize) -> Harness<'static> { .title("Tall dialog") .width(400.0) .actions(|actions| { - actions.primary("Save"); - actions.secondary("Cancel"); + actions.primary("save", "Save"); + actions.secondary("cancel", "Cancel"); }) .show(ui.ctx(), &mut open, |ui| { for row in 0..rows { @@ -366,7 +423,7 @@ fn focusing_a_lower_control_scrolls_it_into_view_and_clicks_reach_it() { let result = Dialog::new("tall") .title("Tall dialog") .width(400.0) - .actions(|actions| actions.primary("Save")) + .actions(|actions| actions.primary("save", "Save")) .show(ui.ctx(), &mut open, |ui| { for row in 0..rows { let button = ui.add(Button::new(format!("Row {row}")).id_salt(("row", row))); @@ -423,3 +480,73 @@ fn short_body_does_not_grow_the_surface() { surface.height() ); } + +/// Page content under the dialog. `behind_clicks` counts clicks that got through the backdrop. +fn page_behind(probe: Rc>, kind: DialogKind) -> Harness<'static> { + Harness::new_ui(move |ui| { + install_light(ui); + if ui.add(Button::new("Behind").id_salt("behind")).clicked() { + probe.borrow_mut().behind_clicks += 1; + } + let mut open = probe.borrow().open; + let was_open = open; + let result = Dialog::new("dialog") + .title("Dialog title") + .kind(kind) + .actions(|actions| { + actions.primary("save", "Save"); + }) + .show(ui.ctx(), &mut open, |ui| { + ui.label("Body"); + }); + record(&probe, was_open, open, result); + }) +} + +fn click_at(harness: &mut Harness<'_>, pos: egui::Pos2) { + harness.event(egui::Event::PointerMoved(pos)); + for pressed in [true, false] { + harness.event(egui::Event::PointerButton { + pos, + button: egui::PointerButton::Primary, + pressed, + modifiers: egui::Modifiers::default(), + }); + } + harness.run(); +} + +#[test] +fn every_kind_blocks_the_page_and_backdrop_dismiss_follows_the_kind() { + for kind in [DialogKind::Modal, DialogKind::Dismissible, DialogKind::Alert] { + let probe = open_probe(); + let mut harness = page_behind(probe.clone(), kind); + harness.run(); + let pos = harness.get_by_label("Behind").rect().center(); + click_at(&mut harness, pos); + assert_eq!( + probe.borrow().behind_clicks, + 0, + "{kind:?} let a click through to the page behind the dialog" + ); + match kind { + DialogKind::Modal => { + assert!(!probe.borrow().open, "Modal closes on a backdrop click"); + assert_eq!(probe.borrow().closed_with, Some(CloseReason::Backdrop)); + } + DialogKind::Dismissible | DialogKind::Alert => { + assert!(probe.borrow().open, "{kind:?} stays open on a backdrop click"); + assert_eq!(probe.borrow().closed_with, None, "{kind:?}"); + } + } + } +} + +#[test] +fn deprecated_non_modal_alias_is_dismissible() { + #![allow(deprecated)] + assert_eq!(DialogKind::NonModal, DialogKind::Dismissible); + assert!(!DialogKind::NonModal.backdrop_dismiss()); + assert!(DialogKind::Modal.backdrop_dismiss()); + assert!(!DialogKind::Alert.backdrop_dismiss()); +} diff --git a/crates/fluent2-egui/tests/drawer.rs b/crates/fluent2-egui/tests/drawer.rs index 3cedc42..65bd5d9 100644 --- a/crates/fluent2-egui/tests/drawer.rs +++ b/crates/fluent2-egui/tests/drawer.rs @@ -128,7 +128,7 @@ fn renders_in_every_theme() { #[test] fn escape_closes_modal_and_non_modal_but_not_alert() { for surface in SURFACES { - for kind in [DialogKind::Modal, DialogKind::NonModal] { + for kind in [DialogKind::Modal, DialogKind::Dismissible] { let probe = open_probe(); let mut harness = harness(probe.clone(), surface, kind, true); harness.run(); @@ -155,7 +155,10 @@ fn close_button_closes() { harness.get_by_label(CLOSE_LABEL).click(); harness.run(); assert!(!probe.borrow().open); - assert_eq!(probe.borrow().closed_with, Some(CloseReason::Button(CLOSE_LABEL))); + assert_eq!( + probe.borrow().closed_with, + Some(CloseReason::Button(CLOSE_LABEL.to_owned())) + ); } } @@ -163,7 +166,7 @@ fn close_button_closes() { fn backdrop_click_closes_only_modal() { // The Start drawer is 320 wide in an 800 wide window: (700, 300) is on the backdrop. let backdrop = egui::pos2(700.0, 300.0); - for (kind, closes) in [(DialogKind::Modal, true), (DialogKind::NonModal, false)] { + for (kind, closes) in [(DialogKind::Modal, true), (DialogKind::Dismissible, false)] { let probe = open_probe(); let mut harness = harness(probe.clone(), Surface::Drawer(DrawerPosition::Start), kind, true); harness.run(); diff --git a/crates/fluent2-egui/tests/readme_index.rs b/crates/fluent2-egui/tests/readme_index.rs index 3f499b8..6fe005c 100644 --- a/crates/fluent2-egui/tests/readme_index.rs +++ b/crates/fluent2-egui/tests/readme_index.rs @@ -21,6 +21,10 @@ const SUPPORTING: &[&str] = &[ "CalendarResponse", "CalendarStrings", "CheckState", + "ComboboxOptions", + "FilterProbe", + "ModelCombobox", + "SliceOptions", "DayContent", "FileCardResponse", "InputAppearance", diff --git a/crates/fluent2-egui/tests/slider.rs b/crates/fluent2-egui/tests/slider.rs index fc1dbbd..0ca7eeb 100644 --- a/crates/fluent2-egui/tests/slider.rs +++ b/crates/fluent2-egui/tests/slider.rs @@ -2,10 +2,11 @@ use std::cell::Cell; +use egui::{Shape, Vec2}; use egui_kittest::Harness; use egui_kittest::kittest::Queryable; use fluent2_egui::controls::Slider; -use fluent2_egui::theme::{self, Theme}; +use fluent2_egui::theme::{self, SliderMetrics, Theme, c}; fn install(ui: &egui::Ui) { theme::install(ui.ctx(), &Theme::default()); @@ -73,14 +74,66 @@ fn step_snaps() { assert_eq!(value.get(), 10.0); } -#[test] -fn inverted_range_does_not_panic() { +fn finite_slider( + start: f64, + range: std::ops::RangeInclusive, + step: Option, + press: Option, +) -> f64 { + let value = Cell::new(start); let mut harness = Harness::new_ui(|ui| { install(ui); - let mut value = f64::NAN; - ui.add(Slider::new(&mut value, 8.0..=f64::NAN).vertical()); + let mut current = value.get(); + let response = ui.add(Slider::new(&mut current, range.clone()).step(step)); + if press.is_some() { + response.request_focus(); + } + value.set(current); }); harness.run(); + if let Some(key) = press { + harness.key_press(key); + harness.run(); + } + value.get() +} + +#[test] +fn inverted_range_does_not_panic() { + let value = finite_slider(f64::NAN, 80.0..=10.0, None, None); + assert!(value.is_finite()); + assert!((10.0..=80.0).contains(&value)); +} + +#[test] +fn nan_bound_does_not_panic() { + let value = finite_slider(f64::NAN, 8.0..=f64::NAN, None, None); + assert!(value.is_finite()); +} + +#[test] +fn extreme_finite_range_stays_finite() { + let value = finite_slider(-f64::MAX, -f64::MAX..=f64::MAX, None, None); + assert!(value.is_finite(), "value {value}"); + assert_eq!(value, -f64::MAX); +} + +#[test] +fn degenerate_range_stays_finite() { + let value = finite_slider(4.0, 7.0..=7.0, None, None); + assert_eq!(value, 7.0); +} + +#[test] +fn tiny_step_stays_finite() { + let value = finite_slider( + 0.0, + 0.0..=1.0, + Some(f64::MIN_POSITIVE), + Some(egui::Key::ArrowRight), + ); + assert!(value.is_finite(), "value {value}"); + assert!((0.0..=1.0).contains(&value)); } #[test] @@ -94,3 +147,137 @@ fn renders_in_dark_and_high_contrast() { harness.run(); } } + +/// Run `build` for two frames and return the second frame's shapes, with every +/// `Shape::Vec` expanded. +fn shapes(theme: &Theme, build: impl Fn(&mut egui::Ui)) -> Vec { + let ctx = egui::Context::default(); + theme::install(&ctx, theme); + let mut last = Vec::new(); + for _ in 0..2 { + let raw = egui::RawInput { + screen_rect: Some(egui::Rect::from_min_size( + egui::Pos2::ZERO, + Vec2::new(400.0, 200.0), + )), + ..Default::default() + }; + let mut output = ctx.run_ui(raw, |ui| build(ui)); + last = flatten(output.shapes.into_iter().map(|clipped| clipped.shape)); + output.textures_delta.clear(); + } + last +} + +fn flatten(shapes: impl Iterator) -> Vec { + shapes + .flat_map(|shape| match shape { + Shape::Vec(inner) => flatten(inner.into_iter()), + other => vec![other], + }) + .collect() +} + +#[test] +fn custom_slider_tokens_change_painted_geometry() { + let theme = Theme { + slider: SliderMetrics { + rail: 10.0, + thumb: 32.0, + dot: 8.0, + }, + ..Theme::default() + }; + let painted = shapes(&theme, |ui| { + let mut value = 50.0; + ui.add(Slider::new(&mut value, 0.0..=100.0)); + }); + let rails: Vec<_> = painted + .iter() + .filter_map(|shape| match shape { + Shape::Rect(rect) if (rect.rect.height() - 10.0).abs() < 0.01 => Some(rect.rect), + _ => None, + }) + .collect(); + assert!( + !rails.is_empty(), + "custom rail thickness 10 must appear in painted rects" + ); + let thumbs: Vec<_> = painted + .iter() + .filter_map(|shape| match shape { + Shape::Circle(circle) if (circle.radius - 16.0).abs() < 0.01 => Some(circle.radius), + _ => None, + }) + .collect(); + assert!( + !thumbs.is_empty(), + "custom thumb diameter 32 must paint a 16 radius circle" + ); + assert!( + painted.iter().any(|shape| matches!( + shape, + Shape::Circle(circle) if (circle.radius - 4.0).abs() < 0.01 + )), + "custom inner dot diameter 8 must paint a 4 radius circle" + ); +} + +#[test] +fn disabled_slider_uses_disabled_colours() { + let theme = Theme::default(); + let rgb = |color: egui::Color32| (color.r(), color.g(), color.b()); + let paint = |enabled: bool| { + shapes(&theme, |ui| { + ui.add_enabled_ui(enabled, |ui| { + let mut value = 50.0; + ui.add(Slider::new(&mut value, 0.0..=100.0)); + }); + }) + }; + let rects = |shapes: &[Shape]| { + shapes + .iter() + .filter_map(|shape| match shape { + Shape::Rect(rect) => Some(rgb(rect.fill)), + _ => None, + }) + .collect::>() + }; + let circles = |shapes: &[Shape]| { + shapes + .iter() + .filter_map(|shape| match shape { + Shape::Circle(circle) => Some(rgb(circle.fill)), + _ => None, + }) + .collect::>() + }; + let on = paint(true); + let off = paint(false); + // egui fades a disabled `Ui` after the widget chooses colours, so the pixels are not the + // raw alias. The outcome is that rail, fill and thumb no longer match the enabled frame. + assert_ne!( + rects(&on), + rects(&off), + "disabled rail and fill must not use the enabled colours" + ); + assert_ne!( + circles(&on), + circles(&off), + "disabled thumb and dot must not use the enabled colours" + ); + let enabled_rail = rgb(c(theme.color.neutral_stroke_accessible)); + let enabled_brand = rgb(c(theme.color.compound_brand_background)); + let enabled_thumb = rgb(c(theme.color.neutral_background_1)); + assert!( + !rects(&off).contains(&enabled_rail) && !rects(&off).contains(&enabled_brand), + "disabled rail and fill are not the resting stroke or brand, got {:?}", + rects(&off) + ); + assert!( + !circles(&off).contains(&enabled_thumb) && !circles(&off).contains(&enabled_brand), + "disabled thumb and dot are not the resting background or brand, got {:?}", + circles(&off) + ); +} diff --git a/crates/gallery/examples/a11y_walk.rs b/crates/gallery/examples/a11y_walk.rs index 0ec3cab..9903708 100644 --- a/crates/gallery/examples/a11y_walk.rs +++ b/crates/gallery/examples/a11y_walk.rs @@ -57,8 +57,8 @@ impl eframe::App for Walk { let _ = Dialog::new("walk-dialog") .title("Save changes?") .actions(|actions| { - actions.primary("Save"); - actions.secondary("Cancel"); + actions.primary("save", "Save"); + actions.secondary("cancel", "Cancel"); }) .show(ctx, &mut self.dialog, |ui| { ui.label("Your document has unsaved changes."); diff --git a/crates/gallery/src/pages/surfaces.rs b/crates/gallery/src/pages/surfaces.rs index 9e60766..6a22a32 100644 --- a/crates/gallery/src/pages/surfaces.rs +++ b/crates/gallery/src/pages/surfaces.rs @@ -276,9 +276,9 @@ impl SurfacesPage { .title("Save changes?") .description("Your document has unsaved changes.") .actions(|actions| { - actions.primary("Save"); - actions.secondary("Don't save"); - actions.secondary("Cancel"); + actions.primary("save", "Save"); + actions.secondary("dont-save", "Don't save"); + actions.secondary("cancel", "Cancel"); }) .show(ctx, &mut self.modal, |ui| { ui.label("Your document has unsaved changes."); @@ -290,10 +290,12 @@ impl SurfacesPage { let was = self.non_modal; let result = Dialog::new("surfaces_non_modal") .title("Find") - .kind(DialogKind::NonModal) - .actions(|actions| actions.secondary("Close")) + .kind(DialogKind::Dismissible) + .actions(|actions| actions.secondary("close", "Close")) .show(ctx, &mut self.non_modal, |ui| { - ui.label("Escape closes this dialog; the backdrop does not."); + ui.label( + "Escape closes this dialog; a backdrop click does not. The page behind stays blocked.", + ); }); if was { record(&mut self.status, "Non-modal", result); @@ -304,8 +306,8 @@ impl SurfacesPage { .title("Delete 3 files?") .kind(DialogKind::Alert) .actions(|actions| { - actions.primary("Delete"); - actions.secondary("Cancel"); + actions.primary("delete", "Delete"); + actions.secondary("cancel", "Cancel"); }) .show(ctx, &mut self.alert, |ui| { ui.label("Only a button closes an alert dialog."); @@ -319,8 +321,8 @@ impl SurfacesPage { let result = Dialog::new("surfaces_tall") .title("Connection settings") .actions(|actions| { - actions.primary("OK"); - actions.secondary("Cancel"); + actions.primary("ok", "OK"); + actions.secondary("cancel", "Cancel"); }) .show(ctx, &mut self.tall, |ui| { ui.label("The body scrolls when it is taller than the window."); diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index aa12d81..0e66316 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -84,6 +84,17 @@ one test file. Do not reorganise folders as part of a component item. selects a row. `tests/text_selection.rs` covers it. - Field names mirror Fluent token names in snake_case (`colorNeutralForeground1` becomes `color.neutral_foreground_1`), so a token can be found in Fluent's documentation by name. +- Slider sizes live on `Theme.slider` (`SliderMetrics`: rail, thumb, inner dot). They are + **component tokens**, not Fluent global tokens: Fluent web hard-codes the same 4 / 20 / 12 + px values in the slider styles. Density grows only the allocated cross-axis hit target + (44 in Touch); the thumb diameter is not scaled. Layout clamps on the long axis + (120–240 horizontal, 80–160 vertical) are **fixed constraints**, not tokens. Hover, + pressed and focus fills are **not** customisable on the slider: the painter uses resting + alias colours (`neutral_stroke_accessible`, `compound_brand_background`, + `neutral_background_1`, `neutral_stroke_1`) and the disabled aliases when `ui` is + disabled (`neutral_stroke_disabled`, `neutral_foreground_disabled`, + `neutral_background_disabled`). High contrast uses the same aliases from + `Theme::high_contrast()`. ## Fonts diff --git a/docs/specs/dialog.md b/docs/specs/dialog.md index 44ac0b4..a9ea8cc 100644 --- a/docs/specs/dialog.md +++ b/docs/specs/dialog.md @@ -1,18 +1,26 @@ # Dialog -A modal or non-modal surface built on [`egui::Modal`](https://docs.rs/egui/0.36.2/egui/containers/struct.Modal.html). -The egui modal already paints a full-screen backdrop, blocks input to the UI behind it, tracks the -topmost modal layer, and reports whether any popup was open at the start of the frame. Fluent -chrome, focus, keyboard policy and accessibility sit on top of that. A custom area is not used. +A modal surface built on [`egui::Modal`](https://docs.rs/egui/0.36.2/egui/containers/struct.Modal.html). +Every dialog is modal. The egui modal paints a full-screen backdrop, blocks input to the UI behind +it, tracks the topmost modal layer, and reports whether any popup was open at the start of the +frame. That blocking does not depend on [`DialogKind`]: a click, hover or key meant for the page +does not reach it while any dialog is open. Fluent chrome, focus, keyboard policy and +accessibility sit on top of that. A custom area is not used. ## Kinds +`DialogKind` is the dismissal policy, not modality. All three kinds block background input. + | Kind | Backdrop click | Escape | Use | |---|---|---|---| | `Modal` (default) | closes (`CloseReason::Backdrop`) | closes (`CloseReason::Escape`) when this dialog is the top modal and no popup is open | ordinary dialog | -| `NonModal` | does not close | same as Modal | a surface that stays open while the user clicks the dimmed page | +| `Dismissible` | does not close | same as Modal | a modal surface that stays open when the dimmed page is clicked | | `Alert` | ignored | ignored | a choice the user must make with a button | +`DialogKind::NonModal` is a deprecated alias of `Dismissible`. The old name described the +dismissal policy as if the dialog were not modal. It was not: the backdrop always blocked the +page. New code uses `Dismissible`. + `Dialog::show` returns `DialogResult::Closed` only on the frame the dialog closes. While it stays open the body closure runs and its value is `DialogResult::Open`. Setting `open` to `false` from outside, or calling `DialogActions::close`, is `CloseReason::Programmatic`. @@ -40,7 +48,7 @@ Enter activates the primary action, unless: Escape follows the kind table above and is also swallowed when a popup is open, so a Dropdown inside the dialog can close itself first. The next Escape, with no popup open, closes a Modal or -NonModal dialog. Alert never closes on Escape. +Dismissible dialog. Alert never closes on Escape. ## Nested popups diff --git a/docs/specs/drawer.md b/docs/specs/drawer.md index ee3657a..d59141b 100644 --- a/docs/specs/drawer.md +++ b/docs/specs/drawer.md @@ -12,9 +12,10 @@ copied. built on [`egui::Modal`](https://docs.rs/egui/0.36.2/egui/containers/struct.Modal.html), like [Dialog](dialog.md), and follow the same rules: -- `DialogKind` sets the policy: `Modal` closes on Escape and on a backdrop click, `NonModal` on +- `DialogKind` sets the dismissal policy, not modality. Every overlay blocks the page behind it. + `Modal` closes on Escape and on a backdrop click, `Dismissible` (the old name `NonModal`) on Escape only, `Alert` on neither. The header's Close button (`CLOSE_LABEL`) always closes, as - `CloseReason::Button("Close")`. + `CloseReason::Button("Close".to_owned())`. - A popup open before the body owns Escape. - Focus trap: on opening, focus moves to the surface. Tab and Shift+Tab cycle the Close button, the body's widgets and the surface, and never reach the page.