Repeater: script-tags of a fields content output are stripped when loaded via ajax

[owzim]

!Short description of the issue

When the InputfieldRepeater.js loads fields via ajax, the <script>-tags inside each field are not executed, because they are stripped by:

$(data).find('.InputfieldRepeaterItemRequested');

Notes, a.k.a. how I discovered the issue

Since the Repeater Field has been overhauled a great deal, and with the very useful Profield Repeater Matrix, it's usage will become more important in the future, so I started to tackle the issue of Inputfield Ace Extended not being compatible with it. I managed to get it working, when loaded via ajax as a regular field on a page edit, with some major changes in how the Javascript is loaded and executed. I had to inject some inline script in each inputfield output, it works well with ajax load and I thought I was done, thinking this must make this work with repeaters as well. Unfortunately not yet, see below.

Expected behavior

Any inline script of a field output should be executed.

Actual behavior

<script>-tags are stripped, so their content will not be executed.

Links, related to the issue

• jQuery - script tags in the HTML are parsed out by jQuery and not executed
• jQuery .load() / .ajax() not executing javascript in returned HTML after appended

Suggestion for a possible fix

Source where things are happening: InputfieldRepeater.js:332

Option 1:

This one is tested, and works well.

var
  $data = $(data),
  // Explicitly collect scripts only that are marked by the field module dev
  // to be excuted when loaded via ajax
  // otherwise initPageEditForm(); which is part of the ajax response
  // will be executed as well
  $scripts = $data.filter('script[data-ajax-enable]');
  
// ...

var $addItem = $data.find('.InputfieldRepeaterItemRequested');

// ...

initRepeater($addItem);

// execute the contents of all scripts, collected before, after item has been
// added to the DOM
$scripts.each(function() {
  $.globalEval(this.text || this.textContent || this.innerHTML || '');
});
    

Option 2:

Limit the response to only the markup, that is needed, hence the content of .InputfieldRepeaterItemRequested so that the following is not needed:

var $addItem = $data.find('.InputfieldRepeaterItemRequested');

But rather:

var $addItem = $(data);

Steps to reproduce the issue

1. Create an inputfield InputfieldWithInlineScripts.module, install it and add it to a repeater

<?php namespace ProcessWire;

class InputfieldWithInlineScripts extends InputfieldTextarea {
  public function ___render() {
    return implode(array(
      parent::___render(),
      '<script type="text/javascript" charset="utf-8">',
      'console.log("inline js!");',
      '</script>',
    ));
  }
}

2. Add a new repeater item on page edit and the string inline js! will not appear in the console

Setup/Environment

• ProcessWire 3.0.44
• PHP 5.5.12

[Toutouwai]

Just linking this for future reference: #103

Glad you found some solutions @owzim

[owzim]

Glad you found some solutions @owzim

Has been a PITA though ... took my last night's sleep :)

[ryancramerdesign]

The standard we use with Inputfields is a JS "reloaded" event that gets triggered whenever an Inputfield is inserted into the DOM. It's not just a matter of repeaters, but any ajax-loaded Inputfield, which can be from anywhere in PW according to visibility settings. Other modules like Lister/ListerPro make use of the reloaded event as well. Inline scripts generally aren't a great practice, which is why we've pursued an event approach rather than a inline-script approach. It's not a matter of wanting to exclude them, but rather just seeing it as not necessary since the reloaded approach is the standard PW is using.

While I personally prefer to avoid inline scripts, I'm happy to have them work for those that want them. I did try to get them working at one time because I think it's preferable that they do (even if just to save people time trying to determine why they aren't working), but eventually gave up on it since it didn't seem that important at the time. But your option 1 seems like a simple way to make them work – thanks for your work in finding that option. I'll work on adding this. The only thing I'm wary about is that it seems like this would need to be added everywhere that PW is using ajax loaded Inputfields, which goes far beyond Repeater.

While I'm working on adding this, try using the "reloaded" event with your Inputfield, as you may find it preferable. In your InputfieldOwzim.js file, just use this code:

function InputfieldOwzim($inputfield) {
  // initialize given $inputfield(s)
}
$(document).on('reloaded', '.InputfieldOwzim', function() {
  InputfieldOwzim($(this)); 
}).on('ready', function() {
  InputfieldOwzim('.InputfieldOwzim'); 
}); 

PW already takes care of making sure that your InputfieldOwzim.js file is loaded when it needs to be. If you have other files that need to be loaded as well, add them to $config->scripts in your Inputfield::renderReady() method.

[teppokoivula]

I've used inline JavaScript in a few of my own modules, and one of the most common use cases has been JavaScript that gets created run-time by PHP. In most cases it would be possible to avoid this by storing necessary parts in data-attributes and rethinking the implementation a bit.

In one specific case I had to use inline script to track when an inputfield was properly loaded, i.e. if the script was triggered then everything was ready at browser-side. Details are a bit fuzzy by now, so not entirely sure why I did to do that and if it would've been possible to circumvent this somehow.

Just wondering if there are still valid reasons to inline the JavaScript used by an inputfield? :)

[Toutouwai]

It's not just a matter of repeaters, but any ajax-loaded Inputfield, which can be from anywhere in PW according to visibility settings.

Inputfields that are ajax-loaded according to visibility settings are not affected by this issue because the entire ajax response is inserted. The issue only occurs when the ajax response is filtered into a new jQuery object. This happens in ajax-loaded repeater items, and it seems also in Lister Pro (although I've not been able to trace the exact piece of code responsible for the ajax-loading of Lister Pro fields). So one way to avoid the issue is to return only the wanted markup in the ajax response so no filtering is needed. Perhaps this could be done according to some GET variable being present in the ajax request.

@owzim's idea of putting script elements into a variable before the response data is filtered works well. Rather than require a special data attribute I think it would be nicer if any inline script within the inputfield <li> were executed. It isn't possible to get only child scripts of .Inputfield with jQuery but it seems that plain Javascript works fine:

var d = document.createElement('div');
d.innerHTML = data;
var scripts = d.querySelectorAll('.Inputfield script');
$(scripts).each(function() {
    $.globalEval(this.text || this.textContent || this.innerHTML || '');
});

The main effect of this issue (for me at least) is when you are hooking an existing inputfield type and adding extra Javascript functionality. If you're creating a new inputfield type in your module then you're fine because you can use Inputfield::renderReady() to attach a listener for 'reloaded', plus any additional scripts you need. But renderReady() isn't hookable so the only workaround is to hook ProcessPageEdit and load your scripts there on every page edit load. I don't like this because the inputfield you are modifiying may not even be present on the page (depending on the template of the page being edited).

[owzim]

First of all, thank you all for responding!

@Toutouwai

... so the only workaround is to hook ProcessPageEdit and load your scripts there on every page edit load. I don't like this because the inputfield you are modifying may not even be present on the page (depending on the template of the page being edited).

Yes, this is the only other option I could think of as well, but it feels kind of dirty and I am not fond of it. You also would have to do that with custom CSS. Yikes.

@teppokoivula To describe it in my words, why inline Javscript is necessary and how I used it:

Normally you would add scripts like so:

$this->config->scripts->add('path/to/module-script.js');

and you would add config values like so:

$this->js('fieldName', json_encode($options));

.. so that it results in somehting like this:

window.config.ProcessWire.fieldClassName.fieldName = { ... options ... };

Neither the window.config object, nor the scripts will be added when the field is ajax loaded, so what I did was inject a small script for each field (strings in double curly braces are replaced with the actual values by php):

(function($, config) {
    var fieldClassName = '{{fieldClassName}}',
        fieldName = '{{fieldName}}',
        inputfieldName = 'Inputfield_' + fieldName,
        field, module;

    if (!config[fieldClassName]) config[fieldClassName] = { fields: {} };
    module = config[fieldClassName];

    field = module.fields[inputfieldName] = {
        options: {{jsOptions}}
    };

    if (!{{ajax}}) return;

    if (typeof module.initFields === 'function') {

        // module.initFields exists, meaning the module script
        // is already loaded by another field,
        // so call it
        module.initFields();
    } else {
        var $body = $('body');

        // module.initFields does not exist, meaning the module script has not
        // been loaded by the page or another field
        // so load all scripts passed from the php
        $({{scripts}}).each(function(i, url) {
            $body.append($('<script>').attr({
                type : 'text/javascript',
                src  : url
            }));
        });
    }
})(jQuery, window.config.ProcessWire);

The module script itself will attach its initFields method to window.config.ProcessWire.fieldClassName and call it. It will check if a field has been initialized already and skips it if so.

Every time a new field is loaded via ajax, it will, via the inline script call the initFields method.

...

This solution is very solid and way better that having a script sit there on every page load, with the possibility that the actual fields are not even there.

@ryancramerdesign you proposed solution would make "load scrips on every page load" necessary.

If you have other files that need to be loaded as well, add them to $config->scripts in your Inputfield::renderReady() method.

Good to know, I have not taken that in account with my solution.

I'm happy to have them work for those that want them.

It's not a matter of me wanting that, it's a matter of people who use my modules wanting that :)

[LostKobrakai]

you proposed solution would make "load scrips on every page load" necessary.

I'm not sure how relevant that is with caching. As long as a file was loaded once it shouldn't be loaded much more often and for an admin backend I don't feel like initial load times on a first request do matter that much.

[owzim]

@LostKobrakai I might be a bit anal about fronted ethics :)
"Don't load stuff that might not be used."

[owzim]

@ryancramerdesign

Your proposed method with:

$(document).on('reloaded', '.InputfieldAceExtended', function() {
  // ...
}).on('ready', function() {
  // ...
});

in conjunction with adding scripts and config options in Inputfield::renderReady() works well. I will use that in this case.

This will render my inline approach unnecessary and might be the most solid solution yet.

Still:

The main effect of this issue (for me at least) is when you are hooking an existing inputfield type and adding extra Javascript functionality. If you're creating a new inputfield type in your module then you're fine because you can use Inputfield::renderReady() to attach a listener for 'reloaded', plus any additional scripts you need. But renderReady() isn't hookable so the only workaround is to hook ProcessPageEdit and load your scripts there on every page edit load.

I'd like to see a hookable renderReady() if the proposed inline script evaluation is not an option.

[LostKobrakai]

@LostKobrakai I might be a bit anal about fronted ethics :)
"Don't load stuff that might not be used."

That's true but I rather load them once (and have it cached) than with each ajax request because it's a inlined script.

[ryancramerdesign]

Hey guys, I've pushed an update that adds this script parsing to repeaters consistent with the code posted earlier. I've tested it out and it seems to work well. Though I wasn't 100% sure about when it should run, so currently it's running at the end, after the repeater item is fully initialized. Let me know if you find that it should be different.

Also I didn't think I could make Inputfield::renderReady() hookable, just because it's already well established and used on many Inputfields outside of the core, so a method name change isn't as simple here. What I did instead is add another method called renderReadyHook() that appears under the existing renderReady(): https://github.com/processwire/processwire/blob/dev/wire/core/Inputfield.php#L1031

The renderReady() method checks to see if anything is hooking renderReadyHook, and if so, it runs it. If not, it skips it. So basically, if you want to hook renderReady(), then hook renderReadyHook() instead:

$wire->addHookAfter('InputfieldText::renderReadyHook', function($event) {
  // do something
}); 

So now we've got both inline script support and a hookable renderReady. Glad to hear that the updates aren't necessary for inputfields.js since it already executes inline scripts. That makes sense, because ProcessPageEdit is in charge of that one, and since it's a Process module, it controls the output, which lets it send just the HTML to be inserted.

[owzim]

Thanks a lot @ryancramerdesign for the hook solution.

I inserted the code from your commit to ProcessPageLister.js and ProcessPageLister.min.js which does not seem to fix this particular issue. The ProcessPageLister.js is not even loaded, when working with repeaters, it's the InputfieldRepeater.js. Am I not getting something?

Edit: Sorry, I see you updated InputfieldRepeater.js in processwire/processwire@d077dfa which wasn't referenced here.

Works well on my end. Thanks again!

[ryancramerdesign]

Make sure you grab the entire commit, not just a file. That ProcessPageLister.js update was just another one I found that needed it, but it's not relevant to repeaters, just to ListerPro. You'd want the commit from before that for the repeaters update, but I'd suggest just grabbing a fresh copy of the entire dev branch where it's currently at.

…

On Wed, Dec 14, 2016 at 9:32 AM, Christian Raunitschka < ***@***.***> wrote: Thanks a lot @ryancramerdesign <https://github.com/ryancramerdesign> for the hook solution. I inserted the code from your commit to ProcessPageLister.js and ProcessPageLister.min.js which does not seem to fix this particular issue. The ProcessPageLister.js is not even loaded, when working with repeaters, it's the InputfieldRepeater.js. Am I not getting something? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUCUIEbFoQPyLCoiRrTqDMWcspC1qh6ks5rH_3mgaJpZM4LKAPs> .