customize-control.js : $.contains( document, this.container ) always returns false

[nikeo]

Problem

In the method control.onChangeActive(), the first conditional statement is always true :

if ( ! $.contains( document, this.container ) ) {

because method $.contains( document, this.container ) is always false :).
The two other statements are never parsed.

onChangeActive: function ( active, args ) {
			if ( ! $.contains( document, this.container ) ) {
				// jQuery.fn.slideUp is not hiding an element if it is not in the DOM
				this.container.toggle( active );
				if ( args.completeCallback ) {
					args.completeCallback();
				}
			} else if ( active ) {
				this.container.slideDown( args.duration, args.completeCallback );
			} else {
				this.container.slideUp( args.duration, args.completeCallback );
			}
		},

Cause

=> the arguments of $.contains() must be a DOM element while this.container is a jQuery Object.
see the jQuery method doc : ​https://api.jquery.com/jQuery.contains/

[BinaryKitten]

Hi @nikeo

First off thank you for the report and patch. I'd like to add this jsFiddle as a proof of the issue: ​http://jsfiddle.net/pwnn7dj0/

for myself coming from a jQuery/JS background I'd prefer that the this.container[0] be changed to this.container.get(0).
The former is an undocumented way to get the domElement (yes it works but not the point) and the .get is the documented way.

again this is just my two cents/pence

[nikeo]

Hi, @BinaryKitten thanks for the appreciation and fiddle :)

About the point you've been raising, I'm not sure about this though. According to the .get() jQuery doc ( ​https://api.jquery.com/get/) the use of .get() or the array dereferencing operator is equivalent and shall be driven by the need of additional capabilities that .get() can provide, such as specifying a negative index, which we don't specifically need in our case.

...Each jQuery object also masquerades as an array, so we can use the array dereferencing operator to get at the list item instead...

Another reason that makes me think the zero-based array "approach" is fine is that it has already been used in this file by the previous developers. For example here https://core.trac.wordpress.org/browser/trunk/src/wp-admin/js/customize-controls.js#L290.

Would be curious to have other's point of view about this!
Thanks :)

[BinaryKitten]

Hi @nikeo,

Just a quick reply. I don't think there is much major difference, but it would be my preference to use .get instead of array access - which is, just my preference. There is nothing more than that to it.

Both are valid, and I'm not speaking from authority just as another user/developer.

[nikeo]

yep sure, same here. :)

[westonruter]

@nikeo Thanks for the report. You're quite right.

The relevant commit here that introduces this code was [30307] in WordPress 4.1, which was to fix #30251.

How did you come across this issue in the code? Did you notice a regression?

[westonruter]

I bet you discovered this when hacking on a patch for #33428.

[nikeo]

@westonruter : that's right, discovered when investigating the #33428 ! :)

=> I've noticed no regressions specifically related to the current issue for the moment

[westonruter]

Here's the practical effect of the fix: ​https://cloudup.com/cLR62NiEG0x

When a control's active state changes, it will now animate in and out of view, whereas previously its display would toggle instantly.

[westonruter]

In 34557:

Customizer: Fix usage of jQuery.contains() allowing active state changes to again animate control visibility.

Aligns usage of jQuery.contains() in a control's onChangeActive method with the existing usage in the corresponding onChangeActive method for panels and sections.

Props nikeo.
Fixes #33509.

[nikeo]

Replying to westonruter:

Here's the practical effect of the fix: ​https://cloudup.com/cLR62NiEG0x

When a control's active state changes, it will now animate in and out of view, whereas previously its display would toggle instantly.

Thanks for this feedback :)

[westonruter]

In 40304:

Customize: Fix failure to collapse expanded sections and panels that become deactivated.

Improve jsdoc for onChangeActive function. Restores fix from [34557] which got dropped in [38648].

Props dlh, westonruter.
See #34391, #33509.
Fixes #39430.

[swissspidy]

In 40375:

Customize: Fix failure to collapse expanded sections and panels that become deactivated.

Improve jsdoc for onChangeActive function. Restores fix from [34557] which got dropped in [38648].

Props dlh, westonruter.
See #34391, #33509.
Fixes #39430.

Merges [40304] to the 4.7 branch.