#4087 - insertAfter, insertBefore, etc do not work when destination is original element #1047

Closed
wants to merge 20 commits into
from

Projects

None yet

8 participants

Contributor

No description provided.

Owner

Too bad the element needs to be carried around so much. I'm also worried about the jQuery.inArray() to compare to the element inside .clean(). Would it make sense to flag each element somehow, perhaps with a property, to let us know that this element wasn't detached and shouldn't be appended? Then we'd just need to check the flag in .clean().

Owner

Pulling in @gibson042 since he's been involved in this too.

Contributor

I considered that, but in order to flag the element of interest we'd have to iterate through each of the selected elements and compare against each of the elements to be inserted into that fragment - which I believe has equal complexity to this solution... unless you have a better way in mind?

Owner

Nothing in particular ... 😿 Let me think a bit about it.

@gibson042 gibson042 and 1 other commented on an outdated diff Nov 29, 2012
src/manipulation.js
@@ -255,7 +255,7 @@ jQuery.fn.extend({
// Make sure that the elements are removed from the DOM before they are inserted
// this can help fix replacing a parent with child elements
- if ( !isFunc && typeof value !== "string" ) {
+ if ( !isFunc && typeof value !== "string" && jQuery( value ).index( this ) === -1 ) {
value = jQuery( value ).detach();
gibson042
gibson042 Nov 29, 2012 Member

I'd like this better if were handled in the body instead of the condition: value = jQuery( value ).not( this ).detach()

That way we still detach elements in value that aren't in this, and avoid calling jQuery( value ) twice.

PaulBRamos
PaulBRamos Nov 30, 2012 Contributor

👍

@gibson042 gibson042 commented on the diff Nov 29, 2012
src/manipulation.js
@@ -778,7 +778,11 @@ jQuery.extend({
safe = jQuery.contains( elem.ownerDocument, elem );
// Append to fragment
- fragment.appendChild( elem );
+ // #4087 - If origin and destination elements are the same, and this is
+ // that element, do not append to fragment
+ if ( !( selection && jQuery.inArray( elem, selection ) !== -1 ) ) {
gibson042
gibson042 Nov 29, 2012 Member

The ticket was always going to incur a performance hit, but I really like the elegance of this solution. Note that if ( !selection || jQuery.inArray( elem, selection ) < 0 ) will probably compress a little better, but what's a De Morgan transformation or two between friends?

ALSO: This changes the return value of buildFragment, and https://github.com/PaulBRamos/jquery/blob/e67daf2018c4d7661d4c45e3ba5ad38db9a36bf9/src/manipulation.js#L317-L336 will need some attention as a result. @dmethvin, we've been thinking about dropping the fragment cache anyway... it would probably simplify things here. I'm inclined to place the burden of proof on someone who wants to keep a ~100 byte eight-clause if, but I know there's a lot of history behind that preceding me.

PaulBRamos
PaulBRamos Nov 30, 2012 Contributor

I'll take a look at it.

gibson042
gibson042 Nov 30, 2012 Member

The hard numbers are masked by rounding, but http://jsperf.com/jquery-fragcache shows a ~7% speed improvement from the cache on IE6-7, no significant difference on IE8-9 and Chromium 20, and ~13% on Firefox 17 (not that we really care on newer browsers).

There is a benefit, but it's so modest that I'm still in favor of dropping the cache.

dmethvin
dmethvin Nov 30, 2012 Owner

Wow, thanks for that bench @gibson042! IE6 is just pitiful, but we knew that. Okay, I think we should go ahead and do this thing. It will be a good file size savings and also in-memory usage will be lower since we won't be creating frag cache entries. There's already a ticket for it at http://bugs.jquery.com/ticket/11989 and we can just pull it forward into 1.9.

gibson042
gibson042 Nov 30, 2012 Member

@PaulBRamos, do you want to take that too? Removing the fragment cache will make your job here much easier. You'll probably want to submit a separate pull request for http://bugs.jquery.com/ticket/11989 and then rebase this one after it lands.

I'm happy to do it if you prefer, but rebasing seems so much more pleasant when it's on top of your own changes.

jaubourg
jaubourg Nov 30, 2012 Member

/kill fragcache \o/

PaulBRamos
PaulBRamos Nov 30, 2012 Contributor

I'm on it!

mikesherov
mikesherov Dec 1, 2012 Member

Woah. Fragment cache gone just like that. Amazing.

Member

@jquerybot retest

With this change, requests will never be aborted unless one of them immediately responds. If they don't immediately respond, by the time this code is hit, all requests have been "thened" and have no abort method. So if something goes wrong in an ajax test, we'll have dangling ajax requests potentially issueing asserts in the following tests (which is what the initial code was solving by separating the ajax request from the "thened" promise).

Unless I'm missing something, I don't really see the improvement here.

Member

Shoot, you're right. That was supposed to be .done + .fail.

Is this worth the complexity of exposing a global abort method? What's the use-case?

Member

I've been seeing HEAD request tests timeout in BrowserStack, and wanted to guarantee that they wrapped up cleanly.

Member

Global QUnit timeout, got you.

OMG it's spreading! Oo ;)

My only concern is that testSwarm will pass its time global-evaling variable deletes, hence why there was a no-op imp. Couldn't the no-op just have set the variable to undefined when registering and be done with it or am I over-estimating the strain this would take on old IEs. I just want to limit testSwarm timeouts as much as possible but I have no perf data or anything, mind you ;)

Member

Actually, I'm thinking we need a set to undefined AND a delete. It doesn't take that much time (and this coming from someone very concerned about how long our tests take), but even if it did, the ajax suite in particular is so sensitive to globals management that I consider it necessary.

EDIT: But if that proves untenable, set to undefined would definitely be the fallback.

I see this passes in testswarm but i have a feeling it's just by luck (and I have a feeling that's what makes test fail in IE10)... Now we feed $.when requests that will fail and will immediately end the join. Maybe using a counter (a modification of complete could handle it and be fed to the done and fail calls) would help achieve what you're looking for (rather than $.when)... it would also fix our concerns regarding speed by limiting the overhead of using ajaxTest.

Owner

The premature capitulation would apply to options.teardown below as well, right?

Member

Yep, but it's been fixed already ;)

gibson042 and others added some commits Dec 3, 2012
@gibson042 gibson042 More improvements per @jaubourg 4ada325
@gibson042 gibson042 Fix #12856: keep PSEUDO regex non-greedy 3ab2634
@jaubourg jaubourg Organizes the php scripts used for testing better, so that the whole …
…logic of a unit, server-side and client-side, is contained within the unit itself. Nearly all ajax unit tests take advantage of the new 'framework'. Lots of files got deleted because they became redundant or weren't used anymore.
228ab3d
@jaubourg jaubourg Fixes spacing 2a419a7
Member

@Krinkle seems like we have 12 failures on all browsers in ajax units... thing is I ran those test locally against all browsers so there must be some config discrepancy. Gotta get some sleep right now, but if you have manage to guess what's going on, I'd be happy to adapt the tests to testswarm's configuration if needs be later today ;)

Member

Please some consistency. echo, echo/, echo/index.php. What's it gonna be?

I'd recommend either being completely explicit everywhere (echo/index.php) or abstracting it (calling service() only with a symbolic name and mapping those to a file by appending .php). For example, rename echo/index.php to echo.php. And call service("echo"), which then appends .php.

Depending on which web server (Apache, Nginx, Nodejs, ..) and configuration therefor, various of these may fail (12 fail on jQuery servers).

There various random factors being relied on:

  • accepting directory lookups without trailing slash
  • assuming that even if that is supported, that it does not use a redirect (in which case the AJAX request would get a 301 response)
  • using index.php as the (or one of) default directory index files
    etc.

In case of TestSwarm servers, it does support directory lookups without trailing slash but 301 redirects, causing some of the requests to fail when they get 301 instead of whatever response is expected.

Member

There is various other issues with the PHP code that are imho unacceptable (we do execute this, server side, on production servers!). Please do this from a PR next time instead of pushing straight into master.

Member

228ab3d (by @gibson042) fixes a few of the path errors but from running it locally and by actually looking at the errors on TestSwarm I see that that was only a small part of the problem. I can reproduce the majority of the issues locally regardless of any path format.

There seem to be some invalid XML responses and stuff. Not something I can imagine would make any difference on whatever web server or browser.

And there are the style/security issues with PHP code mentioned earlier (though those don't affect the tests). I don't have time for this right now, as in, we can't have this in master taking over other people's priority all of a sudden.

I recommend reverting these 2 commits out of master ASAP. Then re-submit it as a pull request to go through further review and iteration until it passes the tests.

Member

@Krinkle, there are only 12 tests not passing. I'd prefer we fix the issue on master and be done with it rather than revert/PR/commit... We can fix all the issues here with a simple commit.

If you prefer it consistent, then we can make sure the paths are always correctly formatted. I just assumed that 301 were followed and that data just weren't rePOSTed again after the redirection (hence the differences between echo, echo/, ...). I can as easily switch to a pure file.php approach, that like 2 minutes of work.

I see the request array switching as a security risk, is there any other security issue I didn't see? Also, where is the style guide for the php code? Is there any? I tried to stay consistent with the Core style guide as I didn't know of any specific to PHP.

Owner

Also, where is the style guide for the php code? Is there any? I tried to stay consistent with the Core style guide as I didn't know of any specific to PHP.

We should write one (or at least write the differences between PHP and JS). Basically, just follow the core style guide but use single quotes instead of double quotes unless you need string interpolation.

Member

#1058 as per discussion on IRC.

Contributor

I rebased my changes for #4087 here with master.

Member

This needs a trailing slash if you want to be explicit about the directory (as opposed to a file). More on this in the comment section.

Why error @ error suppression?

Member

That was reminiscent of the old code.

Member

Mixed tabs and spaces..

This is a horrible way to simply switch between GET and POST variable retrieval.

Member

OK, then I'll use an array, I see the security concern, I was deep into the tests trying to find a way to switch.

Why stripslashes()?

Html attribute values in raw html should be html escaped.

Member

+1, thanks for catching this

PHP Notice:  Use of undefined constant status 
PHP Notice:  Use of undefined constant statusText 
PHP Notice:  Use of undefined constant contentType
PHP Notice:  Use of undefined constant content

Keys have to be quoted, even in PHP. Please enable error_reporting during development.

Member

Got you. No idea why my installation doesn't show errors though. It should.

PHP Notice:  Use of undefined constant response 

Why is $response['content'] embedded in a string?

Member

Because I thought I didn't need the quotes, which I no know I do.

Member

Why was it changed into an ok()?

Member

While working on an improved version of this commit I figured out why this wasn't working (PHP filters out certain headers in some scenarios, this triggered one of those cases). I'm working on another way to assert that the header was send/received, but I'm still not sure why you changed it to ok(). It is currently masking the fact that the test is broken..

Member

That's a gignatic typo. I meant to change the value of the contentType options, not change strictEqual to "ok" :/

Member

Of these 3 request() cases, this third one (REST-like) doesn't work.

The resulting URL is:

http://krinkle.dev/jquery/test/data/ajax/echo/index.php/jQuery190042229254357516766_1354711880486&135471188059455430?content= 041275&_=1354711880487

When it should be:

http://krinkle.dev/jquery/test/data/ajax/echo/index.php/jQuery190042229254357516766_1354711880486?135471188059455430&content= 041275&_=1354711880487

The difference being that there is an & before the first ?.

This is caused by the fact that service() (which request() uses) calls url() (from testinit.js) which adds &[timestamp]. So it goes like this:

request:
  "/index.php/??"

service:
  "echo/index.php/??"

url:
  "echo/index.php/??&135471246257997808"

src/ajax#data:
  "echo/index.php/??&135471246257997808&content= 041275"

src/ajax#cache ajax_nonce:
  "echo/index.php/??&135471188059455430&content= 041275&_=1354711880487"

src/ajax/jsonp#callback:
  "echo/index.php/jQuery190042229254357516766_1354711880486&135471188059455430&content= 041275&_=1354711880487"

You masked this failure by extracting the callback name in PHP with a regex that specifically looking for the & sign in the path info argument to index.php/ (instead of using $_SERVER['PATH_INFO']).

Member

This wasn't introduced in this commit, but I'm changing the condition to data === "FAIL" && isOpera so that if it doesn't work outside Opera, the error is "Expected: "notmodified", Result: "success" instead of "Opera is incapable of doing ...". Which isn't very useful outside opera.

Also changing ok( data == null, .. ) to equal( data, null, .. ); so that the error is "Expected: null, Result: "FAIL" instead of no visible error at all.

Member

Agreed on both. There are still lots of stuff like that which need fixin' but I mainly worked on the client/server part of the equation in this commit (even thought I tried to clean things up along the way). Suffice to say this whole js file had turned into quite the mess :/

Owner

@PaulBRamos can you put together a new pull request now that we've made a mess of this one and whacked the frag cache in the process? It will be easier to see what is left. Sorry!

@dmethvin dmethvin closed this Dec 10, 2012
Contributor

Sure thing, no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment