Closed bob2517 closed 3 years ago
The option "html-entity-decode" has now been added to the set-attribute and set-property commands to add the ability to handle already html escaped data from the server or wherever. A separate issue was opened for that, and it's now on the branch. I may add in more things like this later if it comes up. Just the main issue to solve now, which I'm going to do next.
Have a list of allowed functions rather than a list of disallowed functions. By the nature of CSS, all CSS run on the target selector, and CSS commands shouldn't be allowed to work on target selectors that are not there, so only allow those actions commands to continue running after element removal that do not directly influence the target selector. Have the list declared in core-start and maintain it from there.
For the record, this is the initial compiled list:
NOTARGETFUNCS = [
'AddHash',
'Ajax',
'AjaxFormPreview',
'AjaxFormSubmit',
'AjaxPreGet',
'Alert',
'CancelTimer',
'CancelTimerAll',
'ClickOnFirst',
'ClickOnLast',
'ClickOnNext',
'ClickOnNextCycle',
'ClickOnPrevious',
'ClickOnPreviousCycle',
'Clone',
'ConsoleLog',
'CopyToClipboard',
'CreateCommand',
'CreateConditional',
'CreateElement',
'DocumentTitle',
'Eval',
'FocusOn',
'FocusOnFirst',
'FocusOnLast',
'FocusOnNext',
'FocusOnNextCycle',
'FocusOnPrevious',
'FocusOnPreviousCycle',
'FormReset',
'FullscreenExit',
'FullscreenOn',
'Func',
'IframeReload',
'LoadAsAjax',
'LoadConfig',
'LoadImages',
'LoadScript',
'LoadStyle',
'Location',
'Remove',
'RemoveClone',
'RemoveCookie',
'RemoveHash',
'Run',
'SelectAll',
'SelectNone',
'SetCookie',
'StopEventPropagation',
'StopImmediateEventPropagation',
'Trigger',
'UrlChange',
'UrlReplace',
'Var',
'VarDelete'
],
I don't like the idea of lists, as it implies having to maintain them. Maybe doing it in each individual function makes more sense. I can put a check around potential CSS to not run if the target is disconnected. Maybe a short function call to return false at the top of each disallowed action command is the best way. This is a simple solution though.
Meh. I'm going to do the reverse and put a check inside each reliant-on-a-target-selector function and not keep this list. It's just going to nag me otherwise.
Hmmm... But I'd need to remember to put a check in for new commands that are written, and update the docs to specify that custom commands need to have this check in the code for any new custom action commands.
It's a tough call. There's got to be a check there for allowed or disallowed functions somewhere, and custom action commands are going to be affected regardless.
I think putting the check in the function itself is probably the right thing to do, and just update the docs to say that checks for the presence of a target selector should be in place for custom commands where it is possible the target selector will get deleted before all the action commands assigned to it are run.
Just a simple conditional check that returns something if the target selector is not selected, at the top of the function itself, should be enough. The functions shouldn't be returning anything at the moment, so returning an explicit false should be fine.
This is done offline and looks good so far. The list above is no longer in use, and there's a condition in each function that needs a target selector to work. Just needs tests written for it and then a brief update for the docs.
Now on branch with tests passing. WIll port over to work project tomorrow and see if I can remove the workarounds I had in place.
One little caveat to this that needs fixing, so re-opening. Only allow target selector action commands to run if they have already started running. So don't run them if they just aren't on the page to start with. They have to have a valid element there to begin to run the action commands. If during that time the element is deleted, then it is permitted to get to the end of the action command list before exiting.
This is now on the branch, closing.
This has come up on a work project, and has come up before but I've not specifically looked for a solution for it until now.
Currently, action commands that have a delay do not run if the target selector is removed prior to the command being reached. It's intentional behaviour, in order to stop delayed actions happening directly on elements that are no longer on the page. However, this causes a problem if you want to run a function after the target selector has been removed, maybe to update something external - in my case a manual drag and drop restoration.
The existing workaround is to trigger a custom event. But this is a very unintuitive workaround and no one but me would probably even think of that as a solution unless they knew how the core worked or stumbled onto the solution by accident. I could document the workaround but I don't like workarounds.
So remove the inability to run delayed action commands after a target selector has been removed, but only if the action command itself does not rely on the target selector being present. Each command needs looking over and putting into a list if the target selector needs to be there for the command to run.
This should be a quick fix, so do this for 2.5.1, as I need it for work and the workaround is really annoying.
Also while your at it, implement an unescaped option on the set-attribute command, as there's a workaround currently needed for that if ajax data is escaped on the server and needs to get into an attribute - ie. it gets escaped twice. Either that or add the temporary set-attribute-unescaped custom command onto the core.