#13779 closed bug (fixed)

Opened April 16, 2013 01:54AM UTC

Closed April 16, 2013 09:11PM UTC

Last modified April 17, 2013 02:18AM UTC

.remove() semantics have changed in beta3 - and now remove nodes in reverse doc order

Reported by:	BorisMoore	Owned by:	rwaldron
Priority:	undecided	Milestone:	None
Component:	unfiled	Version:	2.0.0-beta3
Keywords:		Cc:
Blocked by:		Blocking:

Description

The code was like this in b2:

remove: function( selector, keepData ) {
	var elem,
		i = 0,
		l = this.length;

	for ( ; i < l; i++ ) {
		elem = this[ i ];

and like this in beta3:

remove: function( selector, keepData ) {
	var elem,
		elems = selector ? jQuery.filter( selector, this ) : this,
		i = elems.length;

	while ( i-- ) {
		elem = elems[ i ];

Note the i--.

This can break client code quite easily. Also it is difficult to write code that will work both with previous versions and new versions, if the client needs to remove in a specific order...

Attachments (0)

Change History (10)

Changed April 16, 2013 03:13AM UTC by gibson042 comment:1

owner:	→ BorisMoore
status:	new → pending

Could you please demonstrate some breaking code on jsFiddle or a similar playground?

Changed April 16, 2013 04:07PM UTC by rwaldron comment:2

owner:	BorisMoore → rwaldron
status:	pending → assigned

Changed April 16, 2013 04:15PM UTC by Rick Waldron comment:3

resolution:	→ fixed
status:	assigned → closed

Fixes #13779. Remove nodes in document order (uses for loop matching empty()).

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>

Changeset: 332ebeb3105a4cab0091dc6f3f6ba8c17b9265bb

Changed April 16, 2013 04:30PM UTC by Rick Waldron comment:4

Fixes #13779. Remove nodes in document order (test)

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>

Changeset: 7a66283d7d35b2024caf85cb588a0972b900cbcd

Changed April 16, 2013 05:41PM UTC by BorisMoore comment:5

_comment0:	Any app that does its own disposal when elements are removed will need to override cleanData, and may need to remove elements in doc order. An example is https://github.com/BorisMoore/jsviews/blob/master/jquery.views.js. \ \ Here is a jsfiddle using 1.9.1: http://jsfiddle.net/BorisMoore/xzGrj/ \ Click on Meet Jo Black and then on Eyes Wide Shut. The master detail selection works correctly. \ \ Here is the same fiddle but using 2.x (edge) (Same would apply to beta3, which is not correctly provided in jsfiddle): http://jsfiddle.net/BorisMoore/xzGrj/ \ Click on Meet Jo Black - it works. \ Click on Eyes Wide Shut. The deleted nodes on the detail are deleted in reverse order, so disposal fails, and there is a script error. → 1366134530588171

_comment0:

Any app that does its own disposal when elements are removed will need to override cleanData, and may need to remove elements in doc order. An example is https://github.com/BorisMoore/jsviews/blob/master/jquery.views.js. \ \ Here is a jsfiddle using 1.9.1: http://jsfiddle.net/BorisMoore/xzGrj/ \ Click on Meet Jo Black and then on Eyes Wide Shut. The master detail selection works correctly. \ \ Here is the same fiddle but using 2.x (edge) (Same would apply to beta3, which is not correctly provided in jsfiddle): http://jsfiddle.net/BorisMoore/xzGrj/ \ Click on Meet Jo Black - it works. \ Click on Eyes Wide Shut. The deleted nodes on the detail are deleted in reverse order, so disposal fails, and there is a script error. → 1366134530588171

Here is a jsfiddle using 1.9.1: http://jsfiddle.net/BorisMoore/xzGrj/

Click on Meet Jo Black and then on Eyes Wide Shut. The master detail selection works correctly.

Here is the same fiddle but using 2.x (edge) (Same would apply to beta3, which is not correctly provided in jsfiddle): http://jsfiddle.net/BorisMoore/WG2av/

Click on Meet Jo Black - it works.

Click on Eyes Wide Shut. The deleted nodes on the detail are deleted in reverse order, so disposal fails, and there is a script error.

Changed April 16, 2013 05:44PM UTC by BorisMoore comment:6

The above comment was from me, BorisMoore. Didn't realize I was no longer logged in...

Changed April 16, 2013 07:42PM UTC by BorisMoore comment:7

Here is a really simple test case: http://jsfiddle.net/BorisMoore/YFNbC/

In the case of JsViews, data-binding is achieved in some scenarios by insertion of script nodes as marker, (with type !== "javascript" of course), before and after the data-bound HTML content. Some frameworks insert comment nodes, though browsers sometimes move those nodes.

Anyway, the framework may need to handle removal of the preceding marker node differently from the following marker node, as part of the process of disposing the binding, and may need to handle the preceding node before the follow one, know which is which, etc...

Up to now jQuery has always used document-order semantics, so code could easily have relied on that. Different users of the framework could be using any number of different version of jQuery. So changing the ordering semantics can create problems...

Changed April 16, 2013 08:33PM UTC by markelog comment:8

resolution:	fixed
status:	closed → reopened

Commits were reverted, reopening for futher discussion.

I'd say, even if removal order is not documented, some plugins might depend on it (see interesting discussion in #12213 ticket).

Changed April 16, 2013 09:11PM UTC by markelog comment:9

resolution:	→ wontfix
status:	reopened → closed

https://github.com/jquery/jquery/commit/332ebeb3105a4cab0091dc6f3f6ba8c17b9265bb#commitcomment-3025058

Changed April 17, 2013 02:18AM UTC by Rick Waldron comment:10

resolution:	wontfix → fixed