arvgta / ajaxify

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

Style tag attributes being removed #230

Closed vijayhardaha closed 1 year ago

vijayhardaha commented 1 year ago

Hey,

I am working on a WordPress website and Recently I started using WordPress Blocks. If you know about WordPress blocks then you might be aware they add multiple <style> tags to your page content.

Since I was using style: false in Ajaxify options before so I was not getting styles for those extra tags after a request. In this case, I set style: true and I am getting all those style tags, but my wpadminbar was hidden after the request. I tracked the issue. so I found

<style media="print">#wpadminbar { display:none; }</style>

Above line becomes

<style>#wpadminbar { display:none; }</style>

You can see media attribute is removed, after the request.

So First I am interested to know if I set style: false then why I am not getting those style tags in my body as they should be?

second, can we keep/restore the style attributes as it is when style: true is defined?

Thanks

vijayhardaha commented 1 year ago

Additional Information:

These are the options that I am using with v8.2.5

{
    "elements": "#main-container, #wpadminbar",
    "selector": "#main-container a:not(.no-ajaxy,.ajax_add_to_cart,a[href*=myaccount],a[href*=account])",
    "forms": "false",
    "canonical": false,
    "refresh": false,
    "requestDelay": 100,
    "scrolltop": true,
    "scrollDelay": 0,
    "bodyClasses": true,
    "deltas": true,
    "asyncdef": false,
    "alwayshints": "wp-includes/js/admin-bar",
    "inline": true,
    "inlinehints": "",
    "inlineskip": "adsbygoogle",
    "inlineappend": true,
    "intevents": true,
    "style": true,
    "prefetchoff": true,
    "verbosity": false,
    "memoryoff": false,
    "passCount": false,
    "pluginon": true
}

And to solve this issue temp. I have used this solution for now:

  1. Replace el.textContent with el.outerHTML in line $s.forEach(el => _addstyle(el.textContent))
  2. then Ay.parse('<style>' + t + '</style>') replaced with Ay.parse(t)

So far, no issue with this change.

arvgta commented 1 year ago

Hey Vijay,

Thanks for bringing this up and your sharp thinking. Your fix is very elegant, but it didn't work against https://4nf.org straight away unfortunately - why, I don't know. So I modified the new code to become:

S.forEach(el => {
        let st = Ay.parse('<style>' + el.textContent + '</style>');
        _copyAttributes(st, el);
        qha(st); //append to the head
    })

...which works against https://4nf.org and www.oeko-fakt.de (another test site).

It is currently only enabled in here:

Could you please test, whether it works for you?

(I will leave it like this until there is news from your side)

vijayhardaha commented 1 year ago

It's working fine.

arvgta commented 1 year ago

Thanks for posting back so quickly and that's good news!

vijayhardaha commented 1 year ago

I have also noticed a strange thing. This is not related to the current topic, but maybe you wanna take a look at this one as well.

window.addEventListener( 'pronto.request', function() {
  console.log( 'test' );
} );

When I use this code, I get 2 test in the console log. I injected this code in https://4nf.org/ from console to test if this happens in other sites as well and then I made a request by clicking on a link, again I am getting two test

Is this a normal thing? because in my work when I append a loader HTML div in body on pronto.request event, I get 2 same divs in body.

arvgta commented 1 year ago

Thanks for pointing that out as well and no - that's most definitely not intended that way. I will check the Ajaxify code and try to reproduce the bug on www.oeko-fakt.de

vijayhardaha commented 1 year ago

Thanks, I will try to debug as well and will update you if I find anything useful.

arvgta commented 1 year ago

I just tried out your kindly supplied snippet:

window.addEventListener( 'pronto.request', function() {
  console.log( 'test' );
} );

...against www.oeko-fakt.de and can confirm that I see the logging message twice. It gets worse -> if I specify the pronto.render event, I can even see 4 logging messages...

arvgta commented 1 year ago

(I will examine, if this is a bug in the newer versions or an old bug)

arvgta commented 1 year ago

I'm afraid to say, that the bug is as old as at least version 8.2.1:

(I get the same thing in case of pronto.render -> 4 logging messages)

Most probably, that "only" happens, when the user defines the Pronto event handler in an inline script. But nevertheless, he should be able to without problems. What's going on is obvious - the event handler is being attached many times, in case of definion within an inline script.

Am thinking of a way to avoid that...

arvgta commented 1 year ago

One simple approach would be to add the string pronto. to ìnlineskip like this:

Just tested that and instead of the 4 messages described above, I "only" get 2... I really should examine, why there are still two messages displayed instead of one...


A different and more elegant approach would be to somehow add a sort of:

...directive

...to the Pronto event handlers


And the very best solution would be to contemplate, why the old jQuery on() worked so well and why the plain vanilla

...doesn't


For the moment, I am adding addEventListener to ìnlineskip like this:

and will examine, why there are nevertheless double logging messages...

arvgta commented 1 year ago

Think I found the culprit - here's the current code for triggering the bespoke events:

Ay.trigger = (t, e) => { 
let ev = document.createEvent('HTMLEvents'); 
ev.initEvent("pronto." + t, true, false); ev.data = e ? e : Ay.Rq("e"); 
window.dispatchEvent(ev); document.dispatchEvent(ev); //this is dodgy!
};

...so our custom event is being dispatched twice...

I think it should only be dispatched once at a window level? (After all, we are listening for these Pronto events at a window level)

What is your opinion?

vijayhardaha commented 1 year ago

Yes, Removing one of them doesn't trigger requests twice.

Yes, Keeping thewindow level will be a good choice here.

arvgta commented 1 year ago

Thanks very much!

I have committed the changes to this file:

Does everything work as expected for you now?

I am wondering whether to keep the addition of addEventListener to inlineskip:

?

vijayhardaha commented 1 year ago

Everything looking fine so far with the new committed file.

I don't think, addition of addEventListener to inlineskip will be needed.

arvgta commented 1 year ago

That's good news.

In some cases modifying ìnlineskip doesn't make a difference, but in the case of pronto.render for example, it does!

So, I'll keep it in there for the moment.

Users that don't like it can simply wipe it out...

arvgta commented 1 year ago

I agree with you - adding constraints to ìnlineskip is not a solution because if there are specialised inline scripts on subpages, they would simply be ignored.

At the moment, I'm afraid that it's up to the user to make sure any event handlers are only attached once. He can do so, by placing appropriate hints in inlineskip...

vijayhardaha commented 1 year ago

Yeah, leave it to the user and if possible write a note about it somewhere on documentation, with a use-case/example.

arvgta commented 1 year ago

Thanks very much. I believe we agree on most everything then...

The documentation would belong on the inline script page and maybe on the event page. I agree on that as well...


What I don't quite understand is why everything worked with jQuery on() before and I can't get it to work programmatically now anymore in the plain vanilla environment:

Obviously, jQuery on() performs a lot more than the native addEventListener() (Have a look at the code behind the links) It would be great to get the Pronto event handling in inline scripts to work "out of the box" again. It's worth mentioning, that we got e.g. native event delegation handling working in the plain vanilla versions already. (See function _on() towards the end of the current Ajaxify source) Maybe there is some kind of corresponding extension that can be made, in order to make event registration work more like jQuery's on()?


I'm thinking of providing a public method to the user instead of addEventListener() something like ->

arvgta commented 1 year ago

Hey Vijay,

in case you're still able to read this - I've introduced the following shorthand function for the user:

function _won(a, b, c = false) { if(c === false) c = {once: true}; window.addEventListener(a, b, c); };

...and called it on this site like this in the head:

<script>_won('pronto.render', function() {
  console.log( 'test' );
  });
</script>

Everything seems to work fine and I like the approach but just one thing is irritating:

When I specify the handler for pronto.render like shown above, only on the pass right after the inital load it fires twice?! (On every subsequent load, it only fires once like it should)

I'm wondering why this is the case?

When I specify pronto.request with the same handler, everything is fine...

What's your opinion on this? Any ideas would be greatly appreciated...


That's to say, that on inital load, the message is missing and "deferred" to the first real load, where it is shown twice...

vijayhardaha commented 1 year ago

Hey,

I tested version 8.2.6 and yes, the initial load pronto.render fires twice. I am not sure why such behavior is happening, but it happens only on inline scripts, not in JS files.

However, I tried setTimeout within _won like this setTimeout( () => window.addEventListener( a, b, c ) ); and it stops the twice fires, but I have no idea how & why? 😅

arvgta commented 1 year ago

Hey Vijay,

Thanks for answering - very kind!

Yes, I introduced the _won() function also for usage in inline scripts. As your kindly provided test proves - we have a timing problem here.

Will think about this for a while...

Thanks again!

arvgta commented 1 year ago

Am closing this thread and opening a new one, dedicated to the second part of this one...

Many thanks to Vijay once more!