WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 19 months ago

Last modified 4 weeks ago

#33509 closed defect (bug) (fixed)

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

Reported by: nikeo Owned by: westonruter
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.1
Component: Customize Keywords: has-patch commit
Focuses: javascript, administration Cc:

Description

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/

Attachments (1)

33509.diff (625 bytes) - added by nikeo 21 months ago.

Download all attachments as: .zip

Change History (16)

@nikeo
21 months ago

#1 @nikeo
21 months ago

  • Keywords has-patch added

#2 @BinaryKitten
21 months ago

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

#3 @nikeo
21 months ago

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 :)

#4 @BinaryKitten
21 months ago

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.

#5 @nikeo
21 months ago

yep sure, same here. :)

#6 @westonruter
20 months ago

  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to westonruter
  • Status changed from new to accepted
  • Version changed from 4.3 to 4.1

@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?

#7 @westonruter
20 months ago

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

#8 @nikeo
20 months ago

  • Keywords reporter-feedback removed

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

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

This ticket was mentioned in Slack in #core by sergey. View the logs.


19 months ago

#10 @SergeyBiryukov
19 months ago

  • Keywords commit added

#11 follow-up: @westonruter
19 months ago

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.

#12 @westonruter
19 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#13 in reply to: ↑ 11 @nikeo
19 months ago

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 :)

#14 @westonruter
6 weeks ago

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.

#15 @swissspidy
4 weeks ago

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.

Note: See TracTickets for help on using tickets.