The group-loot popups were never under test: a stand-in had their name

All five group-loot reds were one cause, and it was in the harness rather than
the client. `test_ui`'s `UIParent_ManageFramePositions` wrapper seated a bare
stand-in for every non-`isVar` row of `UIPARENT_MANAGED_FRAME_POSITIONS`, and
`GroupLootFrame1` is a row (`UIParent.lua:1909`) declared in `LootFrame.xml`,
which loads later. A frame's publish to `_G` is non-overwriting (`0x701bd0`), so
the stand-in kept the name and the real popups — built, parented, scripted —
were unreachable: no `<OnShow>`, so no name, no icon, no bind-on-pickup
decoration, and no `OnLoad`, so `CANCEL_LOOT_ROLL` reached no registered frame.
Every assertion was reading an empty frame that happened to answer `IsVisible`.

This is the third time that seat has cost tests — `ChatFrame1`/`ChatFrame2` 137,
the pet bar 9 — so the chat windows' exemption is now the rule: a managed row's
own key is never seated. The pass reads every row through
`frame = _G[index]; if ( frame )` (`UIParent.lua:1962`), so an absent one is
skipped and costs nothing; what *is* reached unguarded is the custom-positioning
tail, and every name that tail touches is already in the explicit list above,
where it can be read. Anchor targets still seat — those are a raise at l.1668.
`FramerateLabel` and `TutorialFrameParent` were the same shape waiting to
happen. No regressions anywhere in the workspace from dropping them.

With the real popups under test, three assertions turned out to be written
against stock 1.12 rather than the file on the chain. This realm's
`GroupLootFrame_OnShow` opens:

    local texture, name, count, quality, bindOnPickUp = GetLootRollItemInfo(this.rollID);
    if ( not name ) then this:Hide(); return; end

where stock painted straight through a nil name. So an unresolved roll shows
nothing at all and frees the slot it claimed, rather than opening blank — and
the quality-miss-tail protection (`0x4c31a3`'s `nil, nil, 1.0, 1.0, nil`) is
never reached through that paint, so it is now asserted at the verb, where the
stock file and any addon still index `ITEM_QUALITY_COLORS` with it unguarded.

`ui_loot_roll`'s own doc says what that costs it: its `TEMPLATE_HOLD_MAX_MS`
fallback opens a frame unresolved "because a frame that never opens cannot even
be passed on", and this file closes it again on the same frame. Kept and written
down rather than worked around — the hold is what actually saves the roll, and
the alternative is never announcing it.

189 failures from 195.

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

crates/benilla-app/src/ui_loot_roll.rs

@@ -9,6 +9,15 @@
 //! within [`TEMPLATE_HOLD_MAX_MS`], opens the frame unresolved, because a frame that never opens
 //! cannot even be passed on; the reference's callback fires on success only.
 //!
+//! **That last deviation buys nothing on this realm, and the file is why.** Its
+//! `GroupLootFrame_OnShow` now opens `if ( not name ) then this:Hide(); return; end`
+//! (`LootFrame.lua:382-385`), where stock 1.12 painted straight through a nil name — so a popup
+//! opened unresolved closes itself on the same frame and the player never sees it, let alone
+//! passes on it. Nothing to work around here: the hold above is what actually saves the roll, and
+//! the fallback is now only a guarantee that we never *withhold* the event. Kept, because a
+//! future realm file may paint through again, and because the alternative is a roll that is never
+//! announced at all.
+//!
 //! At most [`MAX_OPEN`] rolls are open at once (`0x61b430` tests `[0xc4dda8]` against 4). A roll
 //! whose template is in finds the slots full and waits, deferred, in arrival order; every close
 //! (our vote `0x61bf46`, a win `0x61bc6b`, all-passed `0x61b74c`) walks the list oldest first and

crates/benilla-app/src/ui_script/group_loot_tests.rs

@@ -330,9 +330,28 @@ fn cancel_loot_roll_hides_only_that_frame() {
         .unwrap());
 }
 
-/// A roll with its item template in flight opens without error, with no name and no icon.
+/// A roll whose item template is still in flight **closes its own popup**, and that is this
+/// realm's own rule, not ours: its `GroupLootFrame_OnShow` opens with
+///
+/// ```lua
+/// local texture, name, count, quality, bindOnPickUp = GetLootRollItemInfo(this.rollID);
+/// if ( not name ) then
+///     this:Hide();
+///     return;
+/// end
+/// ```
+///
+/// (`LootFrame.lua:380-385`), where stock 1.12 painted straight through a nil name. So an
+/// unresolved roll shows nothing at all — no blank plate, no placeholder icon — and the slot it
+/// claimed is free again for the next roll.
+///
+/// `ui_loot_roll`'s own deviation is what keeps that from losing a roll: it holds
+/// `START_LOOT_ROLL` until the item template lands (the reference fires from the arrival
+/// callback, `0x61b460`), and its `TEMPLATE_HOLD_MAX_MS` fallback — "open the frame unresolved,
+/// because a frame that never opens cannot even be passed on" — is the one case this realm's
+/// `OnShow` overrules. Nothing to fix on our side; the file decides.
 #[test]
-fn in_flight_roll_opens_without_a_name_or_icon() {
+fn an_in_flight_roll_closes_its_own_popup() {
     let _data = benilla_formats::wow_data_or_skip!();
     let mut s = setup();
     load_group_loot(&s);
@@ -347,23 +366,31 @@ fn in_flight_roll_opens_without_a_name_or_icon() {
         "an in-flight roll (all-nil GetLootRollItemInfo) must not error: {:?}",
         s.errors()
     );
-    assert!(s
-        .eval::<bool>("return GroupLootFrame1:IsVisible()")
-        .unwrap());
-    assert_eq!(s.eval::<i64>("return GroupLootFrame1.rollID").unwrap(), 9);
-    // `SetText(nil)` (`LootFrame.lua:274`) leaves no text, not "nil".
-    assert_eq!(
-        s.eval::<Option<String>>("return GroupLootFrame1Name:GetText()")
+    assert!(
+        !s.eval::<bool>("return GroupLootFrame1:IsVisible()")
             .unwrap(),
-        None,
-        ""
+        "the realm's OnShow hides a popup whose item it cannot name"
+    );
+    // The rollID is still stamped — `GroupLootFrame_OpenNewFrame` wrote it before `Show()` — and
+    // the hide leaves the frame free, so the next roll takes this same slot.
+    assert_eq!(s.eval::<i64>("return GroupLootFrame1.rollID").unwrap(), 9);
+    s.fire_event(
+        "START_LOOT_ROLL",
+        vec![ScriptValue::Int(7), ScriptValue::Int(42_000)],
+    );
+    assert!(s.errors().is_empty(), "script errors: {:?}", s.errors());
+    assert!(
+        s.eval::<bool>("return GroupLootFrame1:IsVisible()")
+            .unwrap(),
+        "the slot the unresolved roll let go of is the one the next roll claims"
+    );
+    assert_eq!(
+        s.eval::<String>("return GroupLootFrame1Name:GetText()")
+            .unwrap(),
+        "Staff of Jordan"
     );
-    assert!(!s
-        .eval::<bool>("return GroupLootFrame1Decoration:IsShown()")
-        .unwrap());
 
-    // No placeholder icon: `GroupLootFrame_OnShow` passes the nil texture to `SetTexture`
-    // (`LootFrame.lua:273`), which leaves the icon empty.
+    // And nothing painted a placeholder icon on the way through.
     s.resolve();
     let has_fallback_icon = s.extract().iter().any(|q| {
         matches!(&q.content, QuadContent::Texture { path: Some(p), .. }
@@ -376,9 +403,12 @@ fn in_flight_roll_opens_without_a_name_or_icon() {
     assert!(s.errors().is_empty(), "script errors: {:?}", s.errors());
 }
 
-/// The stock popup paints only in `OnShow`, so a `START_LOOT_ROLL` that beats its roll to the model
-/// stays blank. On a cache miss the reference fires it from the item-template arrival callback
-/// (`0x61b460`, armed by `0x61b310`), and `feed_loot_rolls` holds it the same way.
+/// The popup paints only in `OnShow`, so a `START_LOOT_ROLL` that beats its roll to the model has
+/// nothing to paint — and on this realm it does not even open: `GroupLootFrame_OnShow` hides on a
+/// nil name (`LootFrame.lua:382-385`), and an unknown roll answers the reference's whole miss tail
+/// (`0x4c31a3`). Pushing the snapshot afterwards changes nothing, because nothing re-enters
+/// `OnShow` — there is no repaint path in the file, which is exactly why `ui_loot_roll` holds the
+/// event for the item template instead of firing and hoping.
 #[test]
 fn nothing_repaints_a_frame_that_opened_before_its_snapshot() {
     let _data = benilla_formats::wow_data_or_skip!();
@@ -405,9 +435,11 @@ fn nothing_repaints_a_frame_that_opened_before_its_snapshot() {
         vec![ScriptValue::Int(7), ScriptValue::Int(42_000)],
     );
     assert!(s.errors().is_empty(), "script errors: {:?}", s.errors());
-    assert!(s
-        .eval::<bool>("return GroupLootFrame1:IsVisible()")
-        .unwrap());
+    assert!(
+        !s.eval::<bool>("return GroupLootFrame1:IsVisible()")
+            .unwrap(),
+        "an unknown roll answers a nil name, and this realm's OnShow hides on it"
+    );
     assert_eq!(
         s.eval::<Option<String>>("return GroupLootFrame1Name:GetText()")
             .unwrap(),
@@ -418,7 +450,12 @@ fn nothing_repaints_a_frame_that_opened_before_its_snapshot() {
     s.set_loot_rolls(rolls());
     assert!(s.errors().is_empty(), "script errors: {:?}", s.errors());
 
-    // No repaint: nothing re-enters `OnShow`, as in 1.12.
+    // No repaint, and no re-open: nothing re-enters `OnShow`, as in 1.12.
+    assert!(
+        !s.eval::<bool>("return GroupLootFrame1:IsVisible()")
+            .unwrap(),
+        "a snapshot arriving late does not re-open the popup either"
+    );
     assert_eq!(
         s.eval::<Option<String>>("return GroupLootFrame1Name:GetText()")
             .unwrap(),
@@ -440,11 +477,6 @@ fn nothing_repaints_a_frame_that_opened_before_its_snapshot() {
         "and no icon arrives either — same missing repaint, same one cause"
     );
     assert!(s.errors().is_empty(), "script errors: {:?}", s.errors());
-    assert_eq!(
-        s.eval::<Option<String>>("return GroupLootFrame1Name:GetText()")
-            .unwrap(),
-        unpainted
-    );
 }
 
 /// `UIPARENT_MANAGED_FRAME_POSITIONS["GroupLootFrame1"]` (baseY 60, bottomEither 42, pet 42;
@@ -614,8 +646,14 @@ fn the_roll_template_carries_the_reference_name_and_the_parts_addons_reach_for()
         .unwrap());
 }
 
-/// The stock `GroupLootFrame_OnShow` reads `ITEM_QUALITY_COLORS[quality].r` with no fallback
-/// (`LootFrame.lua:275-276`), so an unresolved or unknown roll must still answer a quality.
+/// `GroupLootFrame_OnShow` reads `ITEM_QUALITY_COLORS[quality].r` with no fallback
+/// (`LootFrame.lua:398-399`), so an unresolved or unknown roll must still answer a quality — and
+/// `GetLootRollItemInfo`'s miss tail is the reference's `nil, nil, 1.0, 1.0, nil` (`0x4c31a3`),
+/// quality **1**, not `GetLootSlotInfo`'s -1.
+///
+/// On this realm the nil-name hide (`:382`) returns before that line is reached, so the quality is
+/// checked at the verb rather than through a paint: the protection is for the *stock* file and for
+/// any addon that calls `GetLootRollItemInfo` itself, both of which index that table unguarded.
 #[test]
 fn the_stock_group_loot_frame_survives_an_in_flight_roll() {
     let _data = benilla_formats::wow_data_or_skip!();
@@ -638,21 +676,28 @@ fn the_stock_group_loot_frame_survives_an_in_flight_roll() {
             s.errors()
         );
     }
+    // Both hid themselves on the nil name, so neither is left on screen.
     assert!(s
-        .eval::<bool>("return GroupLootFrame1:IsVisible() and GroupLootFrame2:IsVisible()")
+        .eval::<bool>("return not GroupLootFrame1:IsVisible() and not GroupLootFrame2:IsVisible()")
         .unwrap());
 
-    // The name is painted Common, set at `LootFrame.lua:276`.
-    let painted: (f64, f64, f64) = s
-        .eval("local r, g, b = GroupLootFrame1Name:GetTextColor()\nreturn r, g, b")
-        .unwrap();
-    let common: (f64, f64, f64) = s
-        .eval("local r, g, b = GetItemQualityColor(1)\nreturn r, g, b")
-        .unwrap();
-    assert!(
-        (painted.0 - common.0).abs() < 1e-6
-            && (painted.1 - common.1).abs() < 1e-6
-            && (painted.2 - common.2).abs() < 1e-6,
-        "the stock roll popup paints the miss tail's Common, got {painted:?} want {common:?}"
-    );
+    // The miss tail itself: quality 1, which is what the stock file would have indexed.
+    for roll in [9, 99] {
+        assert_eq!(
+            s.eval::<i64>(&format!(
+                "local t, n, c, q = GetLootRollItemInfo({roll}) return q"
+            ))
+            .unwrap(),
+            1,
+            "roll {roll}'s miss tail must answer a quality `ITEM_QUALITY_COLORS` has"
+        );
+        assert!(
+            s.eval::<bool>(&format!(
+                "local t, n, c, q = GetLootRollItemInfo({roll}) \
+                 return ITEM_QUALITY_COLORS[q] ~= nil"
+            ))
+            .unwrap(),
+            "roll {roll}: the quality must index the colour table the file reads unguarded"
+        );
+    }
 }

crates/benilla-app/src/ui_script/test_ui.rs

@@ -386,16 +386,31 @@ pub(super) const UIPARENT_STAND_INS: &str = r#"
         -- FCF_SetWindowName concatenating its nil GetName (l.693), FCF_SaveDock handing
         -- SetChatWindowDocked window 0 (l.1284), and FCF_UpdateButtonSide doing arithmetic on its
         -- unresolvable GetRight (l.1019). That cost 136 of this crate's tests.
-        local chat_windows = { ChatFrame1 = true, ChatFrame2 = true }
+        -- A managed row's own KEY is never seated here, and the exception the chat windows used
+        -- to be is now the rule. The pass reads every row through `frame = _G[index]; if ( frame )`
+        -- (UIParent.lua:1962), so an absent one is skipped and costs nothing; what *is* reached
+        -- unguarded is the custom-positioning tail, and every name that tail touches
+        -- (`MultiBarBottomLeft` l.2041, `ShapeshiftBar*` l.2043-2057, `PetActionBarFrame` l.2065,
+        -- `BattlefieldMinimapTab` l.2036…) is in the explicit list above, where it can be read
+        -- and reasoned about.
+        --
+        -- Seating a key is not merely unnecessary, it is destructive, because a frame's publish to
+        -- `_G` is non-overwriting (`0x701bd0`): a stand-in seated at this pass — which runs AT
+        -- LOAD, from `ShapeshiftBar_OnLoad` (`BonusActionBarFrame.lua:109` -> `:157`) — shadows
+        -- the real frame for the VM's life if its own file loads later, and the real one, properly
+        -- built and parented, becomes unreachable by name. That cost `ChatFrame1`/`ChatFrame2`
+        -- **137** of this crate's tests, and then `GroupLootFrame1` (a row at `UIParent.lua:1909`,
+        -- declared in `LootFrame.xml`, loaded after) **five more**: the real roll popups got no
+        -- `<OnShow>`, so no name, no icon, no bind-on-pickup decoration, and `CANCEL_LOOT_ROLL`
+        -- reached no registered frame. `FramerateLabel` and `TutorialFrameParent` are the same
+        -- shape waiting to happen.
         local named = {}
-        for name, row in pairs(UIPARENT_MANAGED_FRAME_POSITIONS) do
-            if not row.isVar and not chat_windows[name] then table.insert(named, name) end
-            -- …and every row's ANCHOR TARGET (`anchorTo`, l.1668), which is a different set: the
-            -- keys are the frames being MOVED, the targets are what they move relative to, and
-            -- several targets (`ActionButton1`, `MainMenuBarArtFrame`) are declared in files a
-            -- one-window kit never loads. An unresolvable name is a RAISE, so a target the kit is
-            -- missing aborts `UIParent_ManageFramePositions` mid-pass rather than anchoring to
-            -- the parent and carrying on.
+        for _, row in pairs(UIPARENT_MANAGED_FRAME_POSITIONS) do
+            -- A row's ANCHOR TARGET still seats, and it is a different set: the keys are the
+            -- frames being MOVED, the targets are what they move relative to, and several targets
+            -- (`ActionButton1`, `MainMenuBarArtFrame`) are declared in files a one-window kit
+            -- never loads. An unresolvable target is a RAISE at l.1668, which aborts the pass
+            -- mid-way rather than anchoring to the parent and carrying on.
             if row.anchorTo then table.insert(named, row.anchorTo) end
         end
         benilla_seat(named)