fix(actionbars): keep press-and-hold working with Single Button Assist - #2014
Open
dfrisone wants to merge 1 commit into
Open
fix(actionbars): keep press-and-hold working with Single Button Assist#2014dfrisone wants to merge 1 commit into
dfrisone wants to merge 1 commit into
Conversation
Reported: with Single Button Assist bound to a press-and-hold key, casts are delayed from the first press. Disabling EllesmereUI Action Bars fixes it, on a clean install with no other addons, across bars and classes. Press-and-hold casts on key DOWN, and that is a secure attribute Blizzard sets in UpdatePressAndHoldAction. The only event reaching it is ACTIONBAR_SLOT_CHANGED. We were unregistering Blizzard's whole ActionBarButtonEventsFrame and re-registering a subset, which made US the registrant, which made that SetAttribute a tainted write the client blocks in combat. The workaround was to withhold the event whenever InCombatLockdown, so for the whole pull the attribute went stale against an assist slot that re-picks ~11x/sec, and the button fell back to casting on release. The wholesale wipe is now a targeted one: every event Blizzard registers on that frame is unregistered by name EXCEPT ACTIONBAR_SLOT_CHANGED, which is left on Blizzard's own registration and never taken over. Untainted dispatch, so the attribute keeps updating under lockdown. All three kill sites go through the same helper, including the redundant-kill safety net, which would otherwise strand it again -- nothing re-registers that event any more. The performance reason for the original wipe is untouched: ACTIONBAR_UPDATE_COOLDOWN and the rest are still suppressed, and that was the ~1500/sec path. ACTIONBAR_SLOT_CHANGED is gated per button by Blizzard on "arg1 == 0 or arg1 == self.action", so it wakes only the button whose slot moved. Helper is ns-scoped rather than a file local: this file is at Lua's 200-local main-chunk cap and a local pushed it over.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported by @PeanutZ0906 (Action Bars, 9.1.6): with Single Button Assist bound
to a press and hold key, casts are delayed. Disabling EllesmereUI Action
Bars fixes it. He isolated it on a clean install with no other addons, and
reproduced across different bars and different classes.
What is actually happening
Press-and-hold means cast on key down, and that is a secure attribute
Blizzard maintains in
UpdatePressAndHoldAction. The only event that reaches itis
ACTIONBAR_SLOT_CHANGED.We were calling
UnregisterAllEvents()on Blizzard'sActionBarButtonEventsFrameand re-registering a chosen subset. That made us the registrant, so the
dispatch ran under our taint and Blizzard's own
SetAttributebecame a taintedwrite the client blocks in combat. The existing workaround was to withhold the
event entirely while
InCombatLockdown().So for the whole pull the attribute went stale against an assist slot that
re-picks roughly 11 times a second, and the button fell back to casting on
release. That is why it feels delayed from the very first press rather than
degrading over time.
The change
The wipe is now targeted. Every event Blizzard registers on that frame is
unregistered by name except
ACTIONBAR_SLOT_CHANGED, which is left onBlizzard's own registration and never taken over. The dispatch stays untainted,
so the attribute keeps updating under lockdown and the in-combat gate is no
longer needed for it.
All three kill sites now go through one helper, including the redundant-kill
safety net further down. That one matters: it previously wiped the frame
wholesale, and since nothing re-registers this event any more, leaving it would
have stranded press-and-hold exactly the way the comment there already warns
about.
The performance reason for the original wipe is untouched.
ACTIONBAR_UPDATE_COOLDOWNand the rest are still suppressed, and that was thepath measured at ~1500 dispatches/sec through ~140 mixins.
ACTIONBAR_SLOT_CHANGEDis gated per button by Blizzard onarg1 == 0 or arg1 == self.action, so it only wakes the button whose slot moved.The helper is
ns-scoped rather than a file local because this file is at Lua's200-local main-chunk cap and a local pushed it over.
What needs verifying, and why I am not confident alone
The whole fix rests on one assumption I cannot prove from source: that
selectively unregistering leaves Blizzard's remaining registration untainted.
If that is wrong, we get the blocked-action spam this workaround existed to
prevent.
So testing has two halves:
a full fight.
ADDON_ACTION_BLOCKEDnaming EllesmereUI, especially on a raid pull.This area has history: a slot spamming at 11/sec caused a named incident, and
press-and-hold once ended up silently inert.
luac -pclean, house style check clean.Both halves confirmed in game, the second one under forced restrictions.
Half 1: press-and-hold with Single Button Assist responds on key down, no
delay.
Half 2: tested with
/euidevactive, which forcesaddonCombatRestrictionsForced,addonEncounterRestrictionsForced,addonChallengeModeRestrictionsForcedand the rest on -- the restricted,secret-value environment. That is exactly the regime where a tainted
SetAttributeis blocked, and no blocked-action errors appeared. This is theassumption the fix rested on, so it is the test that mattered.
A live raid pull would still be the final word, since forced restrictions plus
an assist slot re-picking ~11x/sec is the original incident's shape, but the
mechanism is confirmed.