arvgta / ajaxify

Ajaxify - An Ajax Plugin
https://4nf.org/
274 stars 124 forks source link

Inline scripts bugs #97

Closed NZX-DeSiGN closed 8 years ago

NZX-DeSiGN commented 8 years ago

Hello,

I use your awesome framework for a Woocommerce theme and I have problems with inline scripts. First, inline scripts placed in the dynamic content are not loaded. Second, WooCommerce uses scripts inline with CDATA tags in comments to keep an old XML validity. These scripts are not executed to.

Have you solutions ?

Sorry for my English and thanks you !

arvgta commented 8 years ago

Hi,

thanks for raising this issue! Which version are you using? Could you please verify, that the problem also pertains to the latest version? (You can see it running against 4nf.org, where there are also a few error messages)

If all else fails, you could try specifying:

inlineappend: false

Of course, it would be better to solve this issue properly...

Here's the active code in the latest version:

apptext: function (t, type) { 
        var scriptNode = document.createElement('script'), $cd0 = $.cd("g").get(0);
        scriptNode.type = type;
        scriptNode.appendChild(document.createTextNode(t));
        $cd0.appendChild(scriptNode);
    }

(where $.cd("g") fetches the main content div...)

When examing the console against 4nf.org, it shows me, there are errors in the above function. Maybe you can spot the error?

Sorry about the inconvenience!

NZX-DeSiGN commented 8 years ago

Hi,

Yes, I use the lastest version. I think I solved the CDATA problem, check my pull request ! For now I haven't try to solve the inline scripts in dynamic content bug.

I've also succeed to execute inline scripts before file scripts (because Woocommerce use inline scripts for variables), but I think it would be more interesting to execute all scripts (inline and files) in the same order as in the HTML DOM. Do you think it's possible ?

arvgta commented 8 years ago

Hi,

First of all - thanks once more for the pull-request & fix!

I always thought, that Ajaxify preserves the order of inline scripts and does not discriminate between those inside and outside of the content div? All inline scripts should really be attached to the bottom of the main content div, each time around? What order of execution are you experiencing?

Is the CDATA issue solved to your content?

Thanks again!

EDIT: Sorry - you mean a mixture of inline scripts & files with the same order as in the DOM. No that is not being done at the moment indeed. It would require major re-engineering... (At the moment, the external scripts are being loaded beforehand, assuming, that that's what most users want)

EDIT2: However, if you think it would add value to preserve the order of the DOM, I could try to implement it, maybe even setting it to default. I would then like to keep the benefits of delta-loading.

NZX-DeSiGN commented 8 years ago

Hi,

You're welcome ! Yes inline scripts with CDATA works for me now.

That's what I thought, keep the order of the scripts would be complicated given the framework structure. It would be nice to keep the order, to keep the variable declaration order, but I work around my problem by loading inline scripts before. I could put an option to let the user choose, but I don't know how to retrieve options in the GetPage object. Specify me how to access options in this object if you want to add this option and I pull a request with it.

For the inline scripts in the dynamic content, I found the problem. In the GetPage object, the parameter $h of the ld method contain also inlines scripts in the dynamic content and if they have no src attribut, they are deleted (see in old lp method below). I modified this to replace the div tag by script. I think not select them would be better, but I have not found were to filter them. Below is my temporary fix :

New lp method for GetPage object

_ld = function ($t, $h) {
    $h.find(".ajy-script").each(function(){
        if(!($(this).attr("src"))) {
            $(this).replaceWith($(this)[0].outerHTML.replace(/<div class="ajy-[^"]*"/, '<script').replace(/<\/div>$/, '</script>'));
        } else {
            $(this).replaceWith(scri.replace('*', $(this).attr("src")));
        }
    });
    $t.html($h.html());
}

Old lp method for GetPage object that delete inline scripts

_ld = function ($t, $h) {
    $h.find(".ajy-script").each(function(){
        if(!($(this).attr("src"))) $(this).replaceWith('');
        else $(this).replaceWith(scri.replace('*', $(this).attr("src")));
    });
    $t.html($h.html());
}
arvgta commented 8 years ago

Hi,

thanks very much for your sharp thinking and the incredible propositions!

I would like to discuss things a bit, before changing the getPage() or similar logic further...

Please consider the following code, which should run before the code you mentioned:

pO("detScripts", { head: 0, lk: 0, j: 0 }, 0, function ($s) {
    head = pass ? $.getPage("head") : $("head");
    lk = head.find(pass ? ".ajy-link" : "link");
    j = pass ? $.getPage("script") : $("script");
    $s.c = _rel(lk, "stylesheet");
    $s.y = head.find("style");
    $s.can = _rel(lk, "canonical");
    $s.s = j.filter(function() {
         return($(this).attr("src"));
    });
    $s.t = j.filter(function() {return(!($(this).attr("src")));});
    }

...especially these salient bits (please see comments as well):

j = pass ? $.getPage("script") : $("script"); // fetches all scripts, without discriminating - content div or not

and

$s.t = j.filter(function() {return(!($(this).attr("src")));}); // should include all inline scripts of the target, including the content div

So to cut a long story short, the inline scripts in the content div should also be in this group, and thus be attached to the content div with all the others?

Could you please double-check, that they are really not there by inspecting the DOM?

We'll go from there, depending on your findings?

(the following code was intentional and meant to rip-out double inline scripts:

if(!($(this).attr("src"))) $(this).replaceWith('');

)

Thanks very much!

NZX-DeSiGN commented 8 years ago

Hi,

Ok I looked these lines, I begin to understand the framework structure. But the j variable doesn't contain scripts in dynamic content (inline or files) when pass is greater than zero. The ld method replace ajy-script divs in the dynamic content by file script and rip-out inline scripts when $.getPage("script") is executed. So inline script aren't executed at all. So my lp method just correct this mistake (below a cleaner method). Correct my if I'm wrong, but for me j variable just contain script outside of dynamic content when it's equal to $.getPage("script") !

_ld = function($t, $h) {
    $h.find(".ajy-script").each(function() {
        if(($(this).attr("src"))) {
            $(this).replaceWith(scri.replace('*', $(this).attr("src")));
        } else {
            $(this).replaceWith($(this)[0].outerHTML.replace('<div class="ajy-script"', '<script').replace(/<\/div>$/, '</script>'));
        }
    });
    $t.html($h.html());
}
arvgta commented 8 years ago

Hi,

You're absolutely right, though I still don't know why? My compliments, you obviously know my code better than me ;)

Here's what I did - a few tests say a thousand words:

1) Inserted a test inline script:

<script>alert('Success!')</script>

...in the content div of the logging sub-page at 4nf.org with the old code.

Result: no execution at all, as you said, when navigating to this sub-page! Also checked the DOM after load -> the above script is not attached to the bottom of the content div

2) Enabled the code you kindly supplied

Result: Works as it should! The alert is triggered only once...

I still don't really know why it works now, without the scripts being called double, or why it didn't work in the old code. Here's what you pointed out, but I don't get so far:

but for me j variable just contain script outside of dynamic content

...seems to be the case, but why?

Thanks a bunch!

NZX-DeSiGN commented 8 years ago

Hi,

Ok, we move forward ! I've noticed that $.getPage.o.a('-') (I just don't know where this call is made) is called before $.getPage.o.a('script').

So _lSel, then _lDivs and finally _ld are executed before getting scripts in j variable. _ld method replace div tags by script, so they no longer have a ajy-script class to be selected.

Happy to contribute to this amazing framework ! :)

arvgta commented 8 years ago

Hi Nicolas,

you're a genius! Yes, exactly - that is not the order, I was intending.

I reckon, if we can fix the order of execution, we could do without the new fix.

In a nutshell, I would like _ld() to be loaded after $.detScripts($s);

$.getPage("-") is called in the "pronto" sub-plugin, however with "fn" instead of "getPage"

$.rq("can", fn('-', $gthis)); // Update DOM and fetch canonical URL

(I think that's alright)

but the "scripts" bits are called in here and I think that's where the misunderstanding could be:

    lSel: function ($t) { //load page into DOM and handle scripts
        pass++;
        _lDivs($t); //this fires too quicky? -> let's put it at least one line further down?
        $.scripts($.rq("s?")); //will also call "$.detScripts($s);" and initiate delta-loading
        $.scripts("s"); //calls style handling
        $.scripts("a"); //handles all inline scripts
        return $.scripts("c"); //return the canonical URL
    }

If we're lucky, _lDivs($t); just has to be placed further down the ladder?

NZX-DeSiGN commented 8 years ago

Hi,

Ok, thanks for the explanation! I did a quick test, it seems to work ! I don't have so much time until a few days, I let you test a bit more !

arvgta commented 8 years ago

Hi, thanks again and have a great break!

NZX-DeSiGN commented 8 years ago

Hi,

Thanks ! I just have a quick question, regex tagso and tagsc contain 'meta' tag, but you doesn't update meta when you change page, right ? I'm asking because I've got an html template in a script that contain meta tags and ajaxify break that template by replacing meta by div (unclosed if meta hasn't got a closing backslash). Could I remove meta tag from regex ?

arvgta commented 8 years ago

Hi,

Sure, just give it a try, and please tell us, whether Ajaxify still works... Cheers!

NZX-DeSiGN commented 8 years ago

Hi,

It's seems to be ok for me ! Would you like I make a pull request ?

arvgta commented 8 years ago

That'd be great - thanks very much!

mattdown247 commented 6 years ago

Sorry to bring up an old thread but I'd be really interested to hear how you integrated Ajaxify & Woocommerce NZX-DeSiGN.

I'm currently attempting it myself and I'm having a few issues with the Single Product pages if they contain variations. The variation form doesn't seem to attach to the form DOM object properly so I'd be interested to hear what sort of inline scripts / delta loading settings you ended up using.

Thanks