Stanzilla / WoWUIBugs

World of Warcraft UI Bug Tracker
153 stars 7 forks source link

SecureActionButtonTemplate no longer supports "macrotext" execution #552

Open Meorawr opened 2 months ago

Meorawr commented 2 months ago

In the initial alpha build of 11.0.0, the "macrotext" secure attribute appears to have been removed from the macro action available to secure action buttons.

The removal of this functionality has a wide-ranging impact that will negatively impact users quite significantly, primarily over concerns with the limitations of the built-in macro system.

This ticket is intended to collect a few examples of use cases that will be broken to see if improvements can be made for end-users, or for certain functionalities to be added to the API to restore parts of what has been broken.

Macro limits are too low

One common point of feedback so far is that both the length limit of individual macros (255 characters) and the number of available macro slots is considered to be extremely limiting. Addons such as Clique and MacroToolkit which extensively make use of the "macrotext" attribute to allow macros to be bound directly to keypresses or unitframe clicks without consuming precious macro slots.

Increasing both the number of available character and account-wide macro slots, as well as bumping macros to natively support 1023 characters matching what RunMacroText actually supports would be helpful and may mitigate some common uses of the macrotext attribute.

Item processing and other out-of-combat usages

A number of addons use macrotext for tasks such as item processing. Typically this takes the form of a /cast <spell> followed by a /use <item>. In some cases these can be replaced by proper use of the "target-bag"/"target-slot"/"target-item" attributes, but there's allegedly a few use-cases where these item-targetting attributes aren't suitable.

Additionally, for roleplayers addons such as Total RP 3 Extended provide the ability to create custom items that may execute macro texts on a click, which makes it very easy for users to have an item that performs an emote for example. Our use case here is strictly out-of-combat, and we're not keen on the alternative idea of allowing unrestricted loadstring access to users as a replacement.

All of these cases could be made to continue working in 11.0 unaltered if the "macrotext" attribute were re-added, but limited to only ever working while out-of-combat. Conceptually this is a bit similar to InsecureActionButtonTemplate, which allows delegated execution of secure actions on click but only while out-of-combat.

Some slash commands have no secure action equivalents

A few common examples that I've seen so far are /clearfocus and /targetexact - neither of these seems to be directly possible with the existing attribute system available to SecureActionButtonTemplate.

There's also cases of command sequences that aren't trivially replicated without the use of macrotext - for example, /cleartarget followed by /target somethingelse, or casting/using multiple spells or items back-to-back where one doesn't trigger a global cooldown.

In general - it would be wise to do a pass over SecureCmdList and see what makes sense to bring over to secure actions.

Built-in macro conditions are extremely limited

The client doesn't support a lot of macro conditions natively, and the availability of certain conditions like [known:spell] isn't consistent between client flavors. A general resolution for this use case is proposed in #554.

Concerns over macro slot reservations

The removal of this functionality may incentivize addons to "reserve" proper macro slots for their own functions, which would be a significant annoyance for users.

It wouldn't be surprising if the macro APIs need restricting to prevent this from happening, but that would then also wipe out any form of macro management addons that for example save and load macro sets based upon factors such as what instance you're in, or what class you're playing.

cont1nuity commented 2 months ago

The addon "New Openables - Continued" heavily relies on macro text execution to use items (reputation, mounts, cosmetics,...), pick + open lockboxes and disenchant items on a single button click. Those macros are basic /use macros with spells and item targets.

mbattersby commented 2 months ago

This is hugely concerning, LiteMount uses this extensively to do mounting actions.

Meorawr commented 2 months ago

Concrete examples of macrotext strings would be quite useful; as your use case is mount related though I assume you wouldn't be too impacted if macrotext continued to function out of combat?

mbattersby commented 2 months ago

Sure, here are some concrete examples. I mean these first two are showstoppers by themself.

/dismount
/cancelform

These next kinds of things work because journal mounts can summon directly after a non-cast-time spell or item, ignoring the GCD.

/use Prismatic Bauble
/cast Polly Roger
/cast Crusader Aura
/cast Polly Roger

This is a transition from Travel Form to Bear Form ("try to restore druid forms" LiteMount option). The /cancelform is required because Travel Form in flight won't cancel if "Auto Dismount in Flight" is not checked.

/cancelform
/cast Bear Form

I also allow the possibility for users to define two macrotexts that will run (a) out of combat if no mount can be activated and (b) in combat as a default set of combat actions.

Generally speaking the 'macrotext' option is critical to a lot of the useful flexiblity I have written. I don't see any good reason for Blizzard to remove it, and am very angry about the whole idea.

mbattersby commented 2 months ago

Also as far as combat goes, since Blizzard saw fit to add dragon riding mounting in combat for a recent raid encounter (Tindral Sageswift), I've also been able to detect being on the right map and set a dragonriding mount as the combat action so it will all work nicely. That will be totally impossible.

Edit: last but not least (sorry for the spam) this is the default set of combat actions LiteMount will do.

/dismount [mounted]
/stopmacro [mounted]
# If you are a druid cancel travel/mount form if you are in it. There is some runtime calculation
# done for the actual commands used since the shapeshift form indexes needed for [form:x] are not
# fixed.
/leavevehicle
Duugu commented 2 months ago

I use macrotext a lot for "/click" macros in my blind accessibility addon (https://github.com/Duugu/Sku) to do mouse clicks on default ui widgets via the keyboard. Like rolling on stuff, using bag items, accepting quests, accepting group invites, etc in combat. These and more are required all the time in combat. Not having macrotext for secure buttons will basically break most ic (non-default ui) actions for blind players. The complete lack of macrotext, even ooc, will most likely break a large part of my blind accessibility addon and many other accessibility addons.

I can't think of a way to replace the ic clicks without macrotext. A lot of the ooc stuff can probably be changed to not use /click on default ui widgets. But this will almost certainly lead to taint issues (like with calling Click() handlers or such of the default ui widgets instead of doing /click on the widget), and/or it will increase the effort a lot. The addon is basically a full ui replacement/conversion. So it covers every single part of the game/default ui features. Not being able to do /click on default ui widgets will push me into a scenario where I would have to rebuild the full default ui for my accessible audio ui. An example: instead of just hooking into OnShow of the quest details frame and doing a single /click mouse click on Accept for a shared quest, I would have to rebuild the entire 'stack' of code behind Blizzard's quest details frame. I think it's easy to see that this would double, triple, or more the effort of building a blind accessible UI. :/

Again: /click macros are pretty essential for my accessibility addon.

seblindfors commented 2 months ago

I use /click macrotext to execute two clicks on my custom game menu. The first to close the game menu /click GameMenuButtonContinue and the second to execute some action afterwards, usually clicking a microbutton. Calling HideUIPanel is not an option because it does not work in combat, and GameMenuFrame is not explicitly protected so the frame ref is also not available in combat. The replacement menu is fully secure and uses these proxy buttons to avoid spreading taint.

image

My other use cases are pretty simple things:

/cleartarget
/clearfocus
/targetenemy

and finally:

/cast Disenchant
/use bagID slotID

All these use cases can be solved by writing this logic into the macro namespace, but that would just be bloat for my users, not to mention contending with other addons for the same limited space.

Another niche thing I use macrotext for is button proxies, because there's currently no way to use type: click and clickbutton: button from the restricted environment when you're iterating over children of a secure handler. However, macrotext can dynamically map /click buttonName. The button has to be named for this approach to work, I use this convention to provide this accessibility wheel for the spell flyout.

This could be fixed by adding the spell IDs to each flyout button through attributes, so that they can be accessed from the RE without extreme book-keeping of all the flyouts, since flyout information is not accessible in the RE. image

TimothyLuke commented 2 months ago

This change absolutely breaks GSE. GSE's user base and primary supporters are returning veterans with hand injuries (loss of function and digit amputation) and other people with accessibility issues who wanted a better alternative than the broken castsequence implementation. GSE allows them to produce between 65-70% of their character's simmed DPS, allowing them to play reasonably and not get kicked from end-game content. They aren't running MDI or Mythic Raids but are doing Heroic and KSM. GSE follows all the current rules of one GCD action per hardware event as the execution is not done by itself but rather by the current macrotext implementation.

Cast sequence itself is fine for simple things, but its limitation is that you can't mix and match modifiers. For example, if I want to perform an AoE action at my cursor, cast a couple of spells and then use an AoE action at myself. I can't swap modifiers between @player and @cursor. The macro author also can't swap in non-GCD abilities and commands for step 3 but not steps 1 and 2. The average GSE template creates a macro with 7 individual actions that combine a mix of 2-3 non-GCD abilities and commands with 1 GCD ability.

One of Castsequence's greatest strengths is that if someone doesn't have the resources to do the next action or the previous action wasn't completed successfully, it doesn't move on to the next spell and sits, requiring manual intervention. As the author of GSE my main criticism of my tool is that the macrotext implementation currently doesn't have the same limitation. If you "spam" the button, GSE can attempt to do something and fail and, based on failure, move on to something that succeeds. As an alternative to what is being proposed in reducing and restricting the use of macrotext in combat, forcing mods like GSE into the same restriction in that it can't move onto the next action without success allows it to operate as a replacement for castsequence that allows for the swapping of conditionals but removing the "try till things succeed" that is currently available. The ability to fail and move on gives GSE the appearance of automation

GSE's other function is to build and compile a macro out of combat based on the results of WoW API functions that are not possible currently in macro conditionals; however, this has been covered in #554.

cont1nuity commented 2 months ago

The addon "New Openables - Continued" heavily relies on macro text execution to use items (reputation, mounts, cosmetics,...), pick + open lockboxes and disenchant items on a single button click. Those macros are basic /use macros with spells and item targets.

To add some context:

Troubl156 commented 2 months ago

As others have stated, this will break accessibility addons (for both in and out of combat situations) for disabled people. As a blind player, not having working addons (because Blizzard has refused to comply with federal civil rights legislation and make them a part of the base game) means that, if this is done, we can no longer play the game that, some of us, have been playing for 10+ years. As at least some of the dev team know, because they've been told by a blind member of the community counsel, blindslash and other adaptive addons are essential to the ability to do everything from move about Azeroth, to running dungeons, participating in raids, pvp, etc. Let me be clear, because disabled people have been playing this game for quite some time via the use of addons, the removal of the ability to use addons without the implementation of Blizzard made reasonable accommodations could be construed as discriminatory processes by the State. Personally, I'd prefer to not go that route but it's Blizzard's call.

mbattersby commented 2 months ago

By way of comment, what is special about the macro system/macrotext is allowing multiple casts per user interaction, which as far as I know is the only way the game allows that.

If they are removing "macrotext" we have to assume Blizzard are deliberately trying to remove that ability from addons[1], and we have to assume it is to stop some current action they don't like. Since we don't know why and they won't say, we are in the sad (but normal) situation of trying to guess at solutions that might be acceptable.

If an addon only wants to do one cast per user interaction, it can mostly[2] be done already without "macrotext", even dynamically. More secure attributes could be added if it were needed to support the rest. Pre-combat state saving can be done with SetAttribute and SecureHandlerWrapScript.

The question is do Blizzard want to stop addons performing multiple casts per user interaction? If they do there's no solution possible, and our benign helpful uses are a casualty of something we aren't privy to the reasons for. If they don't, and the "macrotext" removal is for some other reason, we need a replacement way of performing multiple casts per user interaction that is Blizzard-sanctioned. Preferably as some improvement to SecureActionButton.

[1] Or have just made a dumb blunder without understanding. [2] Sad to say my /dismount and /cancelform are part of the won't-work set which is a big problem for me.

TimothyLuke commented 2 months ago

What this ticket has missed or skipped is the intent of this change. It goes from this feature has been removed to possible solutions without the thought that this change potentially shouldn’t have happened. This change seems to affect a greater bunch of users than I would have initially thought. The questions that aren’t addressed by this are?

Meorawr commented 2 months ago

It's an intentional change. There are currently no mitigating changes to replace this functionality.

Examples of real usage of macrotext that would be impacted by this change would help, as these can be used to drive targeted improvements in the event that this change isn't ultimately reverted wholesale solely on negative feedback - an outcome I'd personally expect to be unlikely.

Regarding what this change is addressing, it's broad and not entirely clear - however we can make a few observations;

If the issue were simply macro length then the answer would just be to limit RunMacroText to only accept the standard 255 characters and suppressing /click chaining. That such an answer wasn't taken implies to me that the problem more likely involves the RE and dynamic macrotext.

The only other thing I can think of as reasoning is that they want macros to exist in real macro slots, but then this whole problem just shifts to "who has the best macro auto-management add-on" until those APIs get invariably locked down and we're all back here screaming again having gained nothing and lost everything.

seblindfors commented 2 months ago

Looking at my own use cases, I just do two things that the secure templates currently cannot do without macrotext:

If macrotext is truly being removed outright, I would wish for SABT to get some of the tools provided by SecureCmdList. The SECURE_ACTIONS list is extremely limited, comparatively, and probably remains so because macrotext covered all the things.

kemayo commented 2 months ago

SilverDragon uses it for a few things:

  1. A "scan" macro that does macro-chaining to /target a long list of mobs in the current zone. (This being ruled illegitimate would be fair. 😂)
  2. To fake a unit-popup for a mob once it's seen that can be clicked on to target it via a /targetexact [name] macro.
  3. To proxy right-clicks (macrotext2) on that unit-popup to a close button so it can be hidden in combat. (Which I think I could refactor to avoid needing, if need be. It's a few years since I wrote that.)

Adding a new secure command that let me target a unit by name / ID would be enough to not break things.

TimothyLuke commented 2 months ago

Looking at the API’s available I think I can do everything I need to do without RunMacroText directly.

From a localisation point of view, access to CastSpellById would be significantly more useful.

The other thing that MacroText handles that I haven’t experimented with in CastSpellByName is Base Spells and Override Spells. In Legion’s specialisation overhaul each class has a base spec and the other specs extend and change it. Some spells are simple x replaces y. Talents work the same way. A very simple example is Paladin’s base ability of Avenging Wrath and the talent Crusade. As these don’t change in combat they are pretty stable however you can /cast base spell and get the current version. In MacroText you simply /cast Avenging Wrath and if Crusade has replaced AW, Crusade casts. Procs use this same approach where a spell is temporarily changed in combat. Where this is important is within the RE. You cannot tell if a proc has happened so you don’t know at a mod/macro level if you should use a different version of a spell - you just continue to cast the base spell.

Unfortunately all I see coming from this change is addons that manipulate and rewrite what’s in /macro every time you change talents and target different types of things. Those slots then become super valuable and people will just keep whining about the 255 character limit and the lack of slots. What is currently one macro slot gets replaced by 8 slots. That’s half that characters allotment and now that you changed talents we need to delete those 8 and replace them with these 10. All the macros get written out to slots but you have a button that is simply RunMacro(n) instead of RunMacroText(x)

woodrikj commented 2 months ago

Welcome to Alpha this is pretty par for the course. The functionality is likely not fully enabled or not at all. If this was a PTR build it would mean something but in alpha it means nothing .

RealityWinner commented 1 month ago

To add to this thread I use Grounded style casts for both accessibility and QOL. This change will completely break the functionality due to the lack of ability to perform any sort of chaining. Below is a sample.

OnKeyDown

/cast Rain of Fire

OnKeyUp

/stopspelltarget
/cast [@cursor] Rain of Fire

As RE LUA

if down then
    self:SetAttribute("macrotext", "/cast "..spell)
else
    self:SetAttribute("macrotext", "/stopspelltarget\n/cast [@cursor] "..spell)
end

Ground targeted spells will not cast at cursor if you currently have one selected. Requiring the stopspelltarget which will now be impossible. Requiring a second mouse click confirmation or praying you are not casting on the ceiling.

Edit: It's actually impossible without macro/macrotext to cast @​cursor because of the UnitExists check.

RealityWinner commented 1 month ago

Additionally some aspects of the base macro conditional system does not operate as one would expect leading to inconsistent behavior. In Season of Discovery Runes work by overriding the base X Slot Rune spell. The known macro conditional does not handle these types of spells via spell ID requiring using an addon that can such as M6 or including the long name taking up precious macro space. Some runes such as Improved Frenzied Regeneration are 30 characters long.

This example works

/stopmacro [noknown:Warbringer,combat]
/cast [noknown:Warbringer] Battle Stance
/cast Charge

This example doesn't

/stopmacro [noknown:425421,combat]
/cast [noknown:425421] Battle Stance
/cast Charge

FindSpellBookSlotBySpellID which is available in the RE correctly returns a value for these spell IDs when learnt whereas IsPlayerSpell does not.

Spyroh commented 1 month ago

I use this functionality in KeyMacros to bind long macros (up to 1023 characters) directly to keys, saved per spec (so I can have different macros for Feral/Balance/Restoration, etc). KeyMacros I think Blizzard should change the macro system to be entirely spec-specific like they already do with actionbars, and of course increase the max macro length to at least 511 characters. 255 is ridiculously short, specially for locales where the ability names are super long, which gives an advantage to players on locales where the ability names are shorter.

TimothyLuke commented 3 weeks ago

There appears to be no alternative for @cursor in SecureActionButtonTemplate

In a macro you could go /cast [@cursor] Final Reckoning Putting cursor as the unit in SATB appears to cause the spell not to be fired

RealityWinner commented 3 weeks ago

@TimothyLuke It's impossible without macro/macrotext to cast @​cursor because of the UnitExists check.. UnitExists("cursor") always returns nil failing that check. I don't have beta access and am unable to see if that has changed.

kemayo commented 3 weeks ago

I'll just quickly confirm that right now on beta UnitExists("cursor") is alwaysfalse`.

2072 commented 3 weeks ago

@TimothyLuke @kemayo @RealityWinner it works with the mouseover unit I never used the cursor one, is there a difference?

To answer the OP on use cases: In Decusrive, I use it to click-cast spells (or /use items - is this even possible without macros?) using /stopcasting before so that it can interrupts any ongoing spell casting...

I implemented a workaround creating macros in the global macro namespace (I only need 5 or so depending on how many spells the user has configured within Decursive...). Decursive deletes them on logout (on PLAYER_LEAVING_WORLD).

(I added a comment in each macro to tell why I need these macro telling the user to ask Blizzard to re-add macrotext support if they don't like seeing this macro...)

RealityWinner commented 3 weeks ago

@2072 @​cursor is used for ground targeted effects like Rain of Fire, Blizzard, Earthquake, etc. It allows you to cast them like normal where your cursor is positioned but without having to perform a second mouse click to confirm. Alternatively you can cast them @​player to auto cast them on the ground where your feet are but that's used less often. This is used quite extensively by Elemental Shamans in their rotations and players across the game. You can read my comment above here for more info.

zaxxted commented 1 week ago

NPCsMarkers makes an intensive use of macrotext to place and remove ground markers - as ground markers can only be placed with macros. Will I have to create 8 macros to place markers plus 8 macros to remove markers?

ui_dm

rowaasr13 commented 1 week ago

From changelog of Plumber (https://github.com/Peterodox/Plumber/) version 1.3.0:

Handy Lockpick: This module can no longer function due to API changes to SecureActionButton and macrotext.

Meorawr commented 6 days ago

Alright - so I've been permitted to share the following update on the fate of macrotext. The changes below will take effect in probably the next beta build, but do allow for usual variances.

kemayo commented 6 days ago

Not too bad. Sounds like I'll lose my "try to target every rare mob in the zone" macro, but I get to keep the (more important) fake unit-popups. Losing the former is a fair cop, since it always felt a little bit on the deliberately-dodging-limits side.

RealityWinner commented 5 days ago

Great update to hear! I still would love if real macros could perform /click to allow for easy creation of things like different round robin mount buttons without having to do custom keybinding for every little thing. If the /click consumed the hardware event like /cast does then it should be possible to limit it to only one and not allow going beyond the restrictions while still allowing for not having to do custom keybinds and instead let me simple move around macros on my action bar.