From d65fd19095370b1a046f18274ddb1844142d3532 Mon Sep 17 00:00:00 2001 From: Sam Sung <51686767+samwhosung@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:52:46 +0700 Subject: [PATCH] OnClick fires on release while the button is still PUSHED, as in 1.12.1 (#923) A Button release un-pushed the button before `OnMouseUp` and `OnClick` ran, so both read NORMAL. A plain click on a floating chat tab then took the pick-up branch and left the window following the cursor. In the reference, the mouse-up handler fires `OnMouseUp` first, then reads the state: a PUSHED button with the cursor inside it and the Up registration fires `OnDoubleClick` or `OnClick`, and only then is it set NORMAL, unless locked. A failed gate still un-pushes; an `OnMouseUp` that un-pushes cancels the click. The release now runs that order after `OnMouseUp`, with the started-drag skip kept. Tests read the state inside each handler and drive the stock chat tab. Fixes #699 Co-Authored-By: Claude Sonnet 5.5 --- .../src/ui_script/chat_resize_tests.rs | 73 +++++++++- crates/benilla-ui/src/script/pointer.rs | 133 ++++++++++-------- crates/benilla-ui/src/script/tests/capture.rs | 121 ++++++++++++++++ 3 files changed, 268 insertions(+), 59 deletions(-) diff --git a/crates/benilla-app/src/ui_script/chat_resize_tests.rs b/crates/benilla-app/src/ui_script/chat_resize_tests.rs index a383853c..77301637 100644 --- a/crates/benilla-app/src/ui_script/chat_resize_tests.rs +++ b/crates/benilla-app/src/ui_script/chat_resize_tests.rs @@ -79,7 +79,11 @@ fn left_click(s: &mut UiScript, frame: &str) { /// The tabs ship hidden until a stationary hover reveals them (`FCF_OnUpdate`), and a window /// moves by its tab (`OnDragStart` calls `StartMoving`), so every drag starts here. fn reveal(s: &mut UiScript) { - let (cx, cy) = centre(s, "ChatFrame1"); + reveal_window(s, "ChatFrame1"); +} + +fn reveal_window(s: &mut UiScript, window: &str) { + let (cx, cy) = centre(s, window); s.mouse_move(cx, cy); for _ in 0..45 { s.tick(0.016); @@ -517,3 +521,70 @@ fn the_docked_combat_log_cannot_be_moved_or_resized_on_its_own() { assert_eq!(one, two, "the dock moved as one"); assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); } + +/// Drag `window`'s tab by `(dx, dy)`: press, past the drag threshold, move, release. +fn drag_named_tab(s: &mut UiScript, window: &str, dx: f32, dy: f32) { + reveal_window(s, window); + let tab = format!("{window}Tab"); + let tab = tab.as_str(); + let (tx, ty) = centre(s, tab); + assert_eq!( + s.hit_test_name(tx, ty).as_deref(), + Some(tab), + "the tab is hit" + ); + s.mouse_button(tx, ty, "LeftButton", true); + s.mouse_move(tx + 5.0, ty); + s.mouse_move(tx + dx, ty + dy); + s.mouse_button(tx + dx, ty + dy, "LeftButton", false); + s.resolve(); +} + +/// `FCF_Tab_OnClick` reads `GetButtonState()` inside `OnClick`: NORMAL on the press (`0x779285` +/// before the push `0x7792b1`) so an undocked tab picks its window up, PUSHED on the release +/// (`0x7793a9` before `0x7793de`) so it drops it. A tab pulled out of the dock drags again, and a +/// plain click picks the window up and puts it down, never leaving it on the cursor. +#[test] +fn an_undocked_tab_drags_again_and_a_plain_click_drops_the_window() { + let _data = benilla_formats::wow_data_or_skip!(); + let mut s = chat_ui(); + s.run("FCF_SelectDockFrame(ChatFrame2)").unwrap(); + s.resolve(); + drag_named_tab(&mut s, "ChatFrame2", 0.0, -200.0); + assert!( + s.eval::("return not ChatFrame2.isDocked").unwrap(), + "the first drag pulled the window out of the dock" + ); + assert!(s.eval::("return MOVING_CHATFRAME == nil").unwrap()); + + let first = centre(&mut s, "ChatFrame2"); + drag_named_tab(&mut s, "ChatFrame2", 0.0, 120.0); + let second = centre(&mut s, "ChatFrame2"); + assert_eq!( + second.1, + first.1 + 120.0, + "the second drag of the undocked tab moved the window" + ); + + reveal_window(&mut s, "ChatFrame2"); + let (tx, ty) = centre(&mut s, "ChatFrame2Tab"); + s.mouse_button(tx, ty, "LeftButton", true); + assert!( + s.eval::("return MOVING_CHATFRAME == ChatFrame2") + .unwrap(), + "the press picked the window up" + ); + s.mouse_button(tx, ty, "LeftButton", false); + assert!( + s.eval::("return MOVING_CHATFRAME == nil").unwrap(), + "the release put it down" + ); + let before = centre(&mut s, "ChatFrame2"); + s.mouse_move(tx + 90.0, ty + 40.0); + assert_eq!( + centre(&mut s, "ChatFrame2"), + before, + "the window does not follow a cursor with no button held" + ); + assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); +} diff --git a/crates/benilla-ui/src/script/pointer.rs b/crates/benilla-ui/src/script/pointer.rs index d20d99e2..ee687bc5 100644 --- a/crates/benilla-ui/src/script/pointer.rs +++ b/crates/benilla-ui/src/script/pointer.rs @@ -356,7 +356,7 @@ impl UiScript { drag_release, world_dropped, slider_jump, - double_id, + released, abandoned, color_jump, ): ( @@ -364,7 +364,7 @@ impl UiScript { Option, bool, Option<(u32, f32)>, - Option, + Option, Option, Option<(u32, f64, f64, f64)>, ) = { @@ -428,19 +428,6 @@ impl UiScript { } let release = cursor::take_drag(&mut model, button); let started = release.as_ref().is_some_and(|r| r.started); - // The Button release slot `0x7792d0` clicks only a PUSHED button (`0x7792fc`) with - // the cursor inside its own rect (`0x779335`), not "the topmost frame under the - // cursor", read before the release edge below un-pushes it. - let sources = scroll_clip_sources(&model); - let may_click = pressed.filter(|&h| { - button::release_may_click(&model, h) && frame_contains(&model, &sources, h, x, y) - }); - // The release edge, `SetButtonState(NORMAL)` (`0x7793de`), on the pressed frame - // wherever the cursor is, skipped after a started drag (`0x7792df` tests - // `0x76c040`), which already un-pressed it. - if let (Some(h), false) = (pressed, started) { - button::edge(&mut model, h, ButtonState::on_mouse_up); - } // The world drop: a completed left click on the world, never a drag release, which // keeps carrying (`0x495300` runs on the WorldFrame click release). The world is // `over_world`, not "no frame": the stock `WorldFrame` covers the screen. @@ -450,42 +437,9 @@ impl UiScript { && over_world(&model, pressed) && !capture_lost && cursor::world_drop_click(&mut model); - let wants = format!("{button}Up"); - let click = if started { - None - } else { - may_click - .filter(|&h| button::wants_click(&model, h, &wants)) - .and_then(|h| model.frame_to_id.get(&h).copied()) - }; - // The double click, on the release that qualified as a click: `0x77938d` in the - // mouse-up dispatcher `0x7792d0` fires `OnDoubleClick` instead of the second - // `OnClick` (`0x77939d jmp 0x7793b5` skips the single leg), only for a frame with - // an `OnDoubleClick` script (`[+0x4d4] != 0`). What suppresses the click - // suppresses it too. - let target = click.and(may_click); - let double = target.filter(|&h| { - model - .scripts - .get(&h) - .is_some_and(|s| s.contains("OnDoubleClick")) - && model - .last_click - .get(&h) - .is_some_and(|&t| now - t <= DOUBLE_CLICK_SECS) - }); - if let Some(h) = double { - // A double clears the stamp (`0x779393`), so clicks pair: four fast clicks are - // click, double, click, double. - model.last_click.remove(&h); - } else if let Some(h) = target { - // A single stamps the detector (`0x7793af`) whether or not an `OnClick` script - // is bound. - model.last_click.insert(h, now); - } - let double_id = double.and_then(|h| model.frame_to_id.get(&h).copied()); - let click = if double_id.is_some() { None } else { click }; - (click, release, dropped, None, double_id, None, None) + // The Button release slot `0x7792d0` (below, after `OnMouseUp`) does nothing on a + // started drag (`0x7792df` tests `0x76c040`), which already un-pressed the button. + (None, release, dropped, None, pressed.filter(|_| !started), None, None) } }; // A track press's value jump fires `OnValueChanged` first, outside the borrow. @@ -590,13 +544,8 @@ impl UiScript { if let Some(h) = press_art { button::edge(&mut self.model_mut(), h, ButtonState::on_mouse_down); } - // `OnDoubleClick(self, button)` in place of `OnClick` (`0x77938d call [edx+0x98]`), with - // the same click sound: its firer `0x779650` is the twin of `OnClick`'s `0x779540` and - // passes only the button name of the completing release. - if let Some(id) = double_id { - if let Err(e) = event::fire_widget_handler(&self.lua, id, "OnDoubleClick", vec![btn]) { - self.push_error(e); - } + if let Some(h) = released { + self.release_slot(h, button, (x, y), now, &btn); } // A world-frame hit is not the UI consuming the event: the reference runs its // `OnMouseDown` without consuming (`0x483c40`), then the `BUTTON1`/`BUTTON2` binding, the @@ -604,6 +553,74 @@ impl UiScript { hit_id.is_some_and(|id| !self.is_world_frame(id)) || world_dropped } + /// The Button release slot `0x7792d0` past its base (`0x76c040`: `OnMouseUp`, which the caller + /// has fired), on the frame that took the press: a PUSHED button (`0x7792fc`) with the cursor + /// inside its own rect (`0x779335`), not "the topmost frame under the cursor", and registered + /// for this button's Up (`0x77934e`) fires `OnDoubleClick` (`0x77938d`) or `OnClick` + /// (`0x7793a9`); only then does the tail set NORMAL (`0x7793de`) if the button is unlocked and + /// not disabled (`0x7793c2`..`0x7793d4`), on every path that got past the pushed gate and + /// wherever the cursor is. So a handler reads PUSHED, and what it sets (a lock, a disable) + /// decides the tail. The state is read here, after `OnMouseUp`, so a script that un-pushes the + /// button there cancels the click. + fn release_slot( + &mut self, + h: crate::widget::FrameHandle, + button: &str, + (x, y): (f32, f32), + now: f64, + btn: &Value, + ) { + let (click, double) = { + let model = self.model_ref(); + if model.arena.frame(h).is_none() { + return; + } + let sources = scroll_clip_sources(&model); + let clicks = button::release_may_click(&model, h) + && frame_contains(&model, &sources, h, x, y) + && button::wants_click(&model, h, &format!("{button}Up")); + // The double click: `0x77938d` fires `OnDoubleClick` instead of the second `OnClick` + // (`0x77939d jmp 0x7793b5` skips the single leg), only for a frame with an + // `OnDoubleClick` script (`[+0x4d4] != 0`) whose last click was within the interval. + // What suppresses the click suppresses it too. + let double = clicks + && model + .scripts + .get(&h) + .is_some_and(|s| s.contains("OnDoubleClick")) + && model + .last_click + .get(&h) + .is_some_and(|&t| now - t <= DOUBLE_CLICK_SECS); + (clicks, double) + }; + let id = self.model_ref().frame_to_id.get(&h).copied(); + if let (true, Some(id)) = (click, id) { + if double { + if let Err(e) = + event::fire_widget_handler(&self.lua, id, "OnDoubleClick", vec![btn.clone()]) + { + self.push_error(e); + } + } else { + button::click_button(&self.lua, id, button, false, false); + } + // A double clears the stamp (`0x779393`), so clicks pair: four fast clicks are click, + // double, click, double. A single stamps the detector (`0x7793af`) whether or not an + // `OnClick` script is bound. + let mut model = self.model_mut(); + if model.arena.frame(h).is_some() { + if double { + model.last_click.remove(&h); + } else { + model.last_click.insert(h, now); + } + } + } + // The release edge, `SetButtonState(NORMAL)` (`0x7793de`). + button::edge(&mut self.model_mut(), h, ButtonState::on_mouse_up); + } + /// Fire `OnDragStop` on a gesture's source, the one place a started drag ends; a source that /// died since the press fires nothing. pub(super) fn fire_drag_stop(&mut self, source: crate::widget::FrameHandle) { diff --git a/crates/benilla-ui/src/script/tests/capture.rs b/crates/benilla-ui/src/script/tests/capture.rs index 10b5ce7f..b39f2d56 100644 --- a/crates/benilla-ui/src/script/tests/capture.rs +++ b/crates/benilla-ui/src/script/tests/capture.rs @@ -363,3 +363,124 @@ fn the_press_art_comes_after_on_mouse_down_and_the_down_click() { "PUSHED" ); } + +// ── The release slot's order: OnMouseUp, then the click, then NORMAL (`0x7792d0`) ─────────────── + +/// A Button `A` that logs the state its `OnClick` and `OnDoubleClick` read. +fn click_state_probe(s: &mut UiScript, body: &str) { + s.run(&format!( + r#" + A:SetScript("OnClick", function() table.insert(log, "click:"..A:GetButtonState()) {body} end) + A:SetScript("OnDoubleClick", function() table.insert(log, "double:"..A:GetButtonState()) end) + "# + )) + .unwrap(); + s.mouse_move(200.0, 300.0); +} + +fn press_release(s: &mut UiScript) { + s.mouse_button(200.0, 300.0, "LeftButton", true); + s.mouse_button(200.0, 300.0, "LeftButton", false); +} + +fn state_of_a(s: &mut UiScript) -> String { + s.eval::("return A:GetButtonState()").unwrap() +} + +/// The mouse-up handler fires `OnClick` (`0x7793a9`) and only then sets NORMAL (`0x7793de`), so a +/// click registered for the release reads PUSHED; one registered for the press reads NORMAL +/// (`0x779285` before `0x7792b1`). +#[test] +fn on_click_reads_pushed_on_a_release_click_and_normal_on_a_press_click() { + let mut s = two_frames("Button"); + click_state_probe(&mut s, ""); + press_release(&mut s); + assert_eq!( + log_of(&mut s), + "A:enter A:down:LeftButton A:up:LeftButton click:PUSHED" + ); + assert_eq!( + state_of_a(&mut s), + "NORMAL", + "…and the release then un-pushes it" + ); + + let mut s = two_frames("Button"); + s.run(r#"A:RegisterForClicks("LeftButtonDown")"#).unwrap(); + click_state_probe(&mut s, ""); + press_release(&mut s); + assert!( + log_of(&mut s).contains("click:NORMAL"), + "{}", + log_of(&mut s) + ); + assert!( + !log_of(&mut s).contains("click:PUSHED"), + "{}", + log_of(&mut s) + ); + + let mut s = two_frames("Button"); + s.run(r#"A:RegisterForClicks("LeftButtonDown", "LeftButtonUp")"#) + .unwrap(); + click_state_probe(&mut s, ""); + press_release(&mut s); + assert!( + log_of(&mut s).ends_with("click:NORMAL A:up:LeftButton click:PUSHED"), + "{}", + log_of(&mut s) + ); +} + +/// `OnDoubleClick` replaces the second `OnClick` (`0x77938d`) and runs before the same NORMAL +/// write, so it reads PUSHED too. +#[test] +fn on_double_click_reads_pushed() { + let mut s = two_frames("Button"); + click_state_probe(&mut s, ""); + press_release(&mut s); + press_release(&mut s); + assert!( + log_of(&mut s).ends_with("A:up:LeftButton double:PUSHED"), + "{}", + log_of(&mut s) + ); + assert_eq!(state_of_a(&mut s), "NORMAL"); +} + +/// After the click the handler sets NORMAL only if `[+0x32c]` (the lock) is 0 and `[+0x328]` (the +/// state) is not DISABLED (`0x7793c2`..`0x7793de`): an `OnClick` that pushes the button unlocked is +/// released by the tail, one that locks it keeps what it set. +#[test] +fn the_tail_after_the_click_honours_what_the_click_set() { + let mut s = two_frames("Button"); + click_state_probe(&mut s, r#"A:SetButtonState("PUSHED")"#); + press_release(&mut s); + assert_eq!(state_of_a(&mut s), "NORMAL", "unlocked: the tail un-pushes"); + + let mut s = two_frames("Button"); + click_state_probe(&mut s, r#"A:SetButtonState("PUSHED", 1)"#); + press_release(&mut s); + assert_eq!(state_of_a(&mut s), "PUSHED", "locked: the tail is skipped"); + + let mut s = two_frames("Button"); + click_state_probe(&mut s, "A:Disable()"); + press_release(&mut s); + assert_eq!( + state_of_a(&mut s), + "DISABLED", + "disabled: the tail is skipped" + ); +} + +/// The state is tested after the base slot's `OnMouseUp` script has run (`0x76c099`, then +/// `0x7792fc`): an `OnMouseUp` that un-pushes the button cancels the click. +#[test] +fn an_on_mouse_up_that_unpushes_the_button_cancels_the_click() { + let mut s = two_frames("Button"); + click_state_probe(&mut s, ""); + s.run(r#"A:SetScript("OnMouseUp", function() table.insert(log, "up:"..A:GetButtonState()) A:SetButtonState("NORMAL") end)"#) + .unwrap(); + press_release(&mut s); + assert_eq!(log_of(&mut s), "A:enter A:down:LeftButton up:PUSHED"); +}