Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

In Request.HTML all script tags are evaluated despite a filter selector is being used #2427

Closed
wants to merge 1 commit into from

4 participants

@jbgutierrez

Script tags should be also filtered when using a filter criteria IMHO.

@swhiteman

Then how could you ever run a script against a filtered result? That sounds like removing existing functionality and there's already a solution (evalScripts).

@jbgutierrez

Say you have built your app so that every page you serve is made up of little cached fragments.
You may have provided tiny block scripts within each fragment to provide everyone of them with dynamic behaviour, whatever it may be.

Then you might want to refresh a whole container, but not the full page upon a user interaction. Just like pjax does.

In this scenario, you only need to reevaluate the script tags along the content you want to refresh.

Does it make sense?

@swhiteman

I didn't say there wasn't a use case, I said that disabling scripts just because they weren't inside an element that remains after filtering is a BC break. The current behavior isn't a bug, so what you want is a new feature.

@jbgutierrez

Sure is not a bug! I really meant a feature! Thanks.

@erubio

I think is a good idea, and could be very useful in some cases, +1 from my side

@arian
Owner

This wouldn't be an easy fix, because IE (< IE9) can't handle script with innerHTML, so without https://github.com/mootools/mootools-core/blob/d54312c5/Source/Request/Request.HTML.js#L34-36 it wouldn't execute any JS at all. In other browsers I've tested (IE9, Chrome) it doesn't execute the JS either, but at least it creates the script element. According to this it can create the script tag if there are some other DOM nodes before the script tag, but haven't tried that myself.

I see you made a commit but this probably doesn't work cross browser, which is why we can't simply pull that.

Anyway, this would require some cross browser testing.

Besides that, we can't break backward compatibility. So unless we can do this in a very elegant and simple way we could add it as option, but otherwise I don't think it's really worth it.

Correct me if I'm wrong, but it looks like you're sending a while DOM tree, and only pick some specific parts out of it? Can't you do this more efficient, with only requesting the correct parts, preferable as a JSON data structure...

Generally I think loading the JS through XHR is a bit ugly too, I would try to avoid it.

@jbgutierrez

I might be wrong but, I think Request.HTML doesn't rely on tag scripts being inserted through innerHTML. That's why it parses any script tags in the response and uses Browse.exec which ends up calling createElement to inject a single script tag into the DOM. This approach works crossbrowser.

Honestly, I didn't run the revised unit test through IE anyway :-(

You're right, I'm not fully satisfied with this pull request neither. This commit definitely breaks backwards compatibility because it might be evaluating less tag scripts due to the filtering criteria. I guess, I just wanted to make a call for a revision on this confusing method option.

I'm with you, I don't see a good practice in loading JS through XHR, but that's something Request.HTML offers and it cames handy when you want to lower the number of full page reloads in an existing site without too much effort.

@arian arian closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
15 Source/Request/Request.HTML.js
@@ -30,14 +30,21 @@ Request.HTML = new Class({
success: function(text){
var options = this.options, response = this.response;
+
+ response.html = text;
- response.html = text.stripScripts(function(script){
+ var match = response.html.match(/<body[^>]*>([\s\S]*?)<\/body>/i);
+ if (match) response.html = match[1];
+ var temp = new Element('div', {html: response.html});
+
+ if (options.filter)
+ response.html = temp.getElement(options.filter).outerHTML;
+
+ response.html = response.html.stripScripts(function(script){
response.javascript = script;
});
- var match = response.html.match(/<body[^>]*>([\s\S]*?)<\/body>/i);
- if (match) response.html = match[1];
- var temp = new Element('div').set('html', response.html);
+ temp = new Element('div', {html: response.html});
response.tree = temp.childNodes;
response.elements = temp.getElements(options.filter || '*');
View
5 Specs/1.3client/Request/Request.HTML.js
@@ -157,7 +157,7 @@ describe('Request.HTML', function(){
it('should create an ajax request and correctly filter it by the passed selector', function(){
- var response = '<span>text</span><a>aaa</a>';
+ var response = '<span>text</span><script>___SPEC___=1;</script><a>aaa<script>___SPEC___=2;</script></a><script>___SPEC___=3;</script>';
this.spy.identity = 'Request.HTML onComplete filter';
var request = new Request.HTML({
@@ -173,7 +173,8 @@ describe('Request.HTML', function(){
expect(onCompleteArgs[0].length).toEqual(1);
expect(onCompleteArgs[0][0].get('tag')).toEqual('a');
expect(onCompleteArgs[0][0].get('text')).toEqual('aaa');
-
+ expect(onCompleteArgs[3].trim()).toEqual('___SPEC___=2;');
+ expect(___SPEC___).toEqual(2);
});
it('should create an ajax request that filters the response and updates the target', function(){
Something went wrong with that request. Please try again.