messageframe: a scroll that moves the view fires OnMessageScrollChanged

WIM sets this script on every window it builds — the history frame at load, then
one per correspondent (six whisper windows in one session of his log) — and we
refused the name by raise, which killed the rest of the builder function each
time, not just the handler.

It was on the deliberate "real 1.12 kinds nothing fires raise rather than
silently accept" list. That rule is right and this was the wrong side of it: the
slot now fires, from the ScrollingMessageFrame's own scroll verbs when a scroll
actually moves the view, which is what a chat addon hangs its scroll-down arrow
on. A scroll refused at either end moves nothing and says nothing; whether the
reference fires one there is untraced.

The raise list keeps its rule and its test — `OnInputLanguageChanged` and the
movie frame's are still raises, and a name leaves that list only when something
fires it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn

crates/benilla-ui/src/script/messageframe/scrolling.rs

@@ -59,19 +59,32 @@ fn scroll(
     op: impl FnOnce(&mut ScrollingMessageState, usize),
 ) -> mlua::Result<()> {
     let h = frame_handle_of(lua, this)?;
-    let mut model = lua.app_data_mut::<Model>().expect("model app_data");
-    let viewport_rows = UiScript::message_viewport_rows(&model, h);
-    let frame = model
-        .arena
-        .frame_mut(h)
-        .ok_or_else(|| mlua::Error::runtime("stale frame handle"))?;
-    match &mut frame.kind_state {
-        KindState::ScrollingMessage(smf) => {
-            op(smf, viewport_rows);
-            Ok(())
+    let id = crate::script::object::decode_id(this)?;
+    let moved = {
+        let mut model = lua.app_data_mut::<Model>().expect("model app_data");
+        let viewport_rows = UiScript::message_viewport_rows(&model, h);
+        let frame = model
+            .arena
+            .frame_mut(h)
+            .ok_or_else(|| mlua::Error::runtime("stale frame handle"))?;
+        match &mut frame.kind_state {
+            KindState::ScrollingMessage(smf) => {
+                let before = smf.scroll_offset;
+                op(smf, viewport_rows);
+                smf.scroll_offset != before
+            }
+            _ => return Err(mlua::Error::runtime("not a ScrollingMessageFrame")),
         }
-        _ => Err(mlua::Error::runtime("not a ScrollingMessageFrame")),
+    };
+    // `OnMessageScrollChanged`, with no model borrow alive: the handler re-enters Lua and may call
+    // straight back in. The scroll frame's own slot, the one a chat addon hangs its scroll-down
+    // arrow on (`WIM_HistoryFrameMessageListScrollingMessageFrame` and every whisper window it
+    // builds). Only a scroll that *moved* fires; whether the reference also fires one refused at
+    // an end is untraced.
+    if moved {
+        crate::script::event::fire_widget_handler(lua, id, "OnMessageScrollChanged", Vec::new())?;
     }
+    Ok(())
 }
 
 pub(super) fn install(lua: &Lua) -> mlua::Result<()> {
@@ -890,6 +903,45 @@ mod tests {
         .unwrap();
     }
 
+    /// WIM's mechanism: it hangs its scroll-down arrow on the frame's own
+    /// `OnMessageScrollChanged` and sets it on every window it builds, so the name has to be
+    /// accepted *and* fired. A scroll refused at an end moved nothing and says nothing.
+    #[test]
+    fn a_scroll_that_moves_the_view_fires_the_frames_scroll_script() {
+        let mut s = UiScript::new().unwrap();
+        s.set_screen_size(800.0, 600.0);
+        s.run(
+            "local f = CreateFrame('ScrollingMessageFrame', 'CF')\n\
+             f:SetPoint('BOTTOMLEFT', 32, 90)\n\
+             f:SetWidth(430)\n\
+             f:SetHeight(34)\n\
+             scrolls = 0\n\
+             f:SetScript('OnMessageScrollChanged', function() scrolls = scrolls + 1 end)",
+        )
+        .unwrap();
+        for i in 1..=8 {
+            s.run(&format!("CF:AddMessage('line {i}')")).unwrap();
+        }
+        assert_eq!(
+            s.eval::<i64>("return scrolls").unwrap(),
+            0,
+            "nothing scrolled yet"
+        );
+
+        s.run("CF:ScrollUp()").unwrap();
+        assert_eq!(s.eval::<i64>("return scrolls").unwrap(), 1);
+        s.run("CF:ScrollToBottom()").unwrap();
+        assert_eq!(s.eval::<i64>("return scrolls").unwrap(), 2);
+        // Already at the bottom: refused, so no edge.
+        s.run("CF:ScrollDown()").unwrap();
+        assert_eq!(
+            s.eval::<i64>("return scrolls").unwrap(),
+            2,
+            "a scroll that cannot move fires nothing"
+        );
+        assert!(s.take_errors().is_empty());
+    }
+
     #[test]
     fn lua_addmessage_and_scroll_surface() {
         let s = UiScript::new().unwrap();

crates/benilla-ui/src/script/mod.rs

@@ -419,7 +419,7 @@ pub const SCREEN: crate::layout::Handle = 0;
 /// ScrollFrame's three scroll kinds are `+0x32c`/`+0x334`/`+0x33c` (script-name map `0x786c40`).
 /// The EditBox's vtable (`0x81c910`) replaces the key and char slots: an EditBox never fires
 /// `OnKeyDown`, and fires `OnChar` only from `Insert`, with the inserted text (`0x77c13c`).
-const SCRIPT_KINDS: [&str; 41] = [
+const SCRIPT_KINDS: [&str; 42] = [
     "OnLoad",
     "OnEvent",
     "OnUpdate",
@@ -463,6 +463,10 @@ const SCRIPT_KINDS: [&str; 41] = [
     // `pointer::update_hyperlink_hover` off the same rasterized spans the click path reads.
     "OnHyperlinkEnter",
     "OnHyperlinkLeave",
+    // The ScrollingMessageFrame's own scroll slot, fired by [`messageframe::scrolling`] when a
+    // scroll verb moves the view. WIM sets it on every window it builds, so refusing the name
+    // aborted the builder mid-function and left the window half-made.
+    "OnMessageScrollChanged",
     // The GameTooltip's engine-fired scripts: money render, money clear and world-hover default
     // placement, all three wired by the stock template (`GameTooltipTemplate.xml:617-625`).
     "OnTooltipAddMoney",

crates/benilla-ui/src/script/object/events_regions.rs

@@ -211,10 +211,11 @@ pub(super) fn install(lua: &Lua, m: &Table) -> mlua::Result<()> {
 }
 
 /// `SetScript(name, func)`: stores the closure under one of [`SCRIPT_KINDS`], else raises.
-/// Deviation: real 1.12 kinds nothing fires yet raise too (`OnMessageScrollChanged`,
-/// `OnInputLanguageChanged`, the movie frame's), because an accepted handler that never runs
-/// fails silently. `OnAttributeChanged` is 2.0's, with no 1.12 slot. `OnHyperlinkEnter`/`Leave`
-/// were on that list until `pointer::update_hyperlink_hover` started firing them.
+/// Deviation: real 1.12 kinds nothing fires yet raise too (`OnInputLanguageChanged`, the movie
+/// frame's), because an accepted handler that never runs fails silently. `OnAttributeChanged` is
+/// 2.0's, with no 1.12 slot. `OnHyperlinkEnter`/`Leave` were on that list until
+/// `pointer::update_hyperlink_hover` started firing them, and `OnMessageScrollChanged` until the
+/// scroll verbs did.
 fn set_script(lua: &Lua, this: &Table, name: &str, func: Option<Function>) -> mlua::Result<()> {
     let kind = SCRIPT_KINDS
         .iter()

crates/benilla-ui/src/script/tests/input.rs

@@ -570,8 +570,8 @@ fn the_unfired_script_kinds_still_raise_rather_than_silently_accepting() {
         "OnAttributeChanged",
         // Real 1.12 slots that benilla does not fire. `OnHyperlinkEnter`/`Leave` left this list
         // when `pointer::update_hyperlink_hover` started firing them off the rasterized link
-        // spans — which is the rule this test states, not an exception to it.
-        "OnMessageScrollChanged",
+        // spans, and `OnMessageScrollChanged` when the scroll verbs did — which is the rule this
+        // test states, not an exception to it.
         "OnInputLanguageChanged",
     ] {
         let err = s

docs/ADDON_COMPAT.md

@@ -416,3 +416,4 @@ with the load checked after each.
 | An item tooltip's rebuild was silent, so an addon's lines lasted one refresh | **fixed** | Reported from play with a video: hovering a bag item draws the enhanced tooltip — MasterTradeSkills' recipe block, AdvancedTradeSkillWindow2's `Bags/Bank/Total` — for about nine frames of sixty, and then the plate reverts to the bare one and stays bare while the cursor sits still. The loop behind it is stock: `ContainerFrameItemButton_OnUpdate` re-enters `..._OnEnter` every `TOOLTIP_UPDATE_TIME` (`ContainerFrame.lua:693` in the player's own `patch-4.mpq`, and pfUI's bag slots are `ContainerFrameItemButtonTemplate`, so they run it too), and each turn is `SetOwner` + `SetBagItem` — a clear that wipes every line an addon appended. What puts them back in the reference is a **hide/show pair**: the `SetOwner` binding hides the plate before it even reads the owner argument (`0x531179` zeroes the content gate at `+0xd0`, `0x531183` calls `[vtable+0x84]` = `0x530a60`, which is the `SetOwner(NULL)` core plus the base hide `0x76ad50`), and the content `Set*` re-shows it through the tooltip's own Show override (`0x530a80`), so every rebuild raises `OnHide` then `OnShow`. Ours cleared in place, and the base show early-outs on an already-visible frame (`0x76ae3b`), so the plate never hid and the `OnShow` never came. `LibItemTooltip-1.0` — the tooltip contract of MasterTradeSkills and Gromissingcrafts — hangs entirely on a scratch child frame of `GameTooltip` listening for `OnShow`, which is why exactly those two addons' lines vanished while pfUI's `ItemID` and sell-price lines, hooked on the `Set*` methods themselves, survived every refresh. All addresses read off the player's own `WoW.exe` |
 | The object sparkle was armed but buried inside the object | **fixed** — the placement is ours | *"ore vein is still not sparkling"*, after the switch fix. Reproduced offline in the `fxview` GameObject lane (`WOW_CAPTURE=fxview WOW_FX_GO=310 WOW_FX_GO_TYPE=3`, a copper mining node): the gate fires, `attach_spell_fx` spawns the instance (`kit spawn … Particles\LootFX.mdl`), and `WOW_PARTICLE_CENSUS` lists all four of `LootFX`'s emitters at the node's own position, live and ungated — while the captured frame is **byte-identical** to one taken with the sparkle gated off. The attach cascade's last rung is the host's origin, which for a unit is right (`0x13` is attachment 19, Base, the ground point under its feet, and this is a ground twinkle); a 1.12 **GameObject** model authors no attachment points at all (`copper_Miningnode_01.m2`: 0), so the effect sat at the node's pivot, inside the rock — that model's own vertices run y ∈ [-3.83, +2.73]. A host with no attachment points now seats the effect at the top centre of its own box, and the capture differs; the same sparkle on a peacebloom, whose model is small, already showed at the pivot. The placement is ours and no more: SuperWoW publishes no source and the reference has no object arm to copy. It is also **subtle by design** — `SpellVisualEffectName` row 14 scales 1.0 and `Particles\LootFX.m2` is four emitters of 0.06–0.14 yd particles with ~0.9 yd of reach — so it reads as a few twinkles over the node rather than a beacon |
 | A frame left in saved variables came back as a bag of its fields | **fixed** | `attempt to call method 'AddMessage' (a nil value)` at `Decursive.lua:1393`, every login. Decursive keeps its output window in its own saved table (`Dcr_OutputWindow = DEFAULT_CHAT_FRAME`) and guards the reload with `if Dcr_Saved.Dcr_OutputWindow == nil or not ...` (`:1245`) — which a non-nil **data** table passes. Our writer walked the frame like any other table and wrote ChatFrame1's own fields (`buttonSide`, `channelList`, `editBox`, …), doubling the file (2217 bytes against his real client's 1108) and losing every method. The reference tests the value's `T[0]` identity first — `lua_rawgeti(v, 0)` then `lua_touserdata` at `0x704613`-`0x704628`, the lightuserdata `0x701bd0` wrote — and never walks a widget: a **named** one writes as its bare global name, a Lua expression, so the reloaded chunk's own lookup hands the live object back, and an anonymous one as `nil --[[ unnamed object ]]` (`0x872e0c`). His own real-client file is the proof: `["Dcr_OutputWindow"] = ChatFrame1,`. Deviation: a name that is no Lua identifier writes as an anonymous object, where the reference writes it raw (`%s`) and can leave a file that will not parse. Tests: `a_saved_frame_comes_back_as_the_frame_not_a_copy_of_its_fields`, `an_anonymous_frame_saves_as_the_references_unnamed_object` |
+| `SetScript("OnMessageScrollChanged")` raised, aborting the builder around it | **fixed** | WIM sets it on every window it builds — the history frame at load and then one per correspondent (6 whisper windows in one session of his log, each `WIM_msgFrame<Name>ScrollingMessageFrame`) — so refusing the name by raise killed the rest of that builder function every time, not just the handler. It was on the deliberate "real 1.12 kinds nothing fires, so raise rather than silently accept" list (`object/events_regions.rs`), which is the right rule and the wrong side of it here: the slot now **fires**, from the ScrollingMessageFrame's own scroll verbs when a scroll actually moves the view (`messageframe::scrolling::scroll`), which is what a chat addon hangs its scroll-down arrow on. A scroll refused at either end moves nothing and says nothing; whether the reference also fires one there is untraced. Test: `a_scroll_that_moves_the_view_fires_the_frames_scroll_script`, and the raise list's own test keeps its rule (`the_unfired_script_kinds_still_raise_rather_than_silently_accepting`) |