Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34033 closed defect (bug) (fixed)

Insufficient check for existence of DOM elements in jQuery object

Reported by: tywayne's profile tywayne Owned by: karmatosed's profile karmatosed
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: javascript Cc:

Description

This was found in development of Twenty Sixteen (https://github.com/WordPress/twentysixteen/issues/294),
and also exists in Twenty Fifteen, Twenty Fourteen, and Twenty Thirteen.

It presents itself in slightly different ways in each theme, but the essence of the bug is this:

A variable is set to a jQuery object with something like this ( example from twentyfifteen )

secondary = $( '#secondary' );
button = $( '.site-branding' ).find( '.secondary-toggle' );

And then those variables are used in a conditional statement later like this:

if ( ! secondary || ! button ) {
    return;
}

The conditional check is attempting to return early if the DOM elements don't exist, but since the variables contain a jQuery object, they are truthy regardless of whether it found any DOM elements.

A more appropriate check would be to use .length like so:

if ( ! secondary.length || ! button.length ) {
	return;
}

Attachments (4)

34033-1.diff (869 bytes) - added by tywayne 9 years ago.
Patch for Twenty Fifteen
34033-2.diff (633 bytes) - added by tywayne 9 years ago.
Patch for Twenty Fourteen
34033-3.diff (638 bytes) - added by tywayne 9 years ago.
Patch for Twenty Thirteen
34033-4.diff (2.1 KB) - added by tywayne 9 years ago.
Combined patch for all three themes

Download all attachments as: .zip

Change History (12)

#1 @tywayne
9 years ago

Will gladly work up a patch - would it be best as a single combined patch or one for each bundled theme?

@tywayne
9 years ago

Patch for Twenty Fifteen

@tywayne
9 years ago

Patch for Twenty Fourteen

@tywayne
9 years ago

Patch for Twenty Thirteen

@tywayne
9 years ago

Combined patch for all three themes

#2 @tywayne
9 years ago

  • Keywords has-patch added

#3 @karmatosed
9 years ago

Thanks for this @tywayne, it's great to see something from Twenty Sixteen being brought into the other default themes.

This ticket was mentioned in Slack in #core-themes by karmatosed. View the logs.


9 years ago

#5 @tywayne
9 years ago

Anyone have any thoughts or feedback on this? Anything else I can do or provide to help this ticket?

#6 @obenland
9 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Version trunk deleted

#7 @karmatosed
9 years ago

  • Owner set to karmatosed
  • Resolution set to fixed
  • Status changed from new to closed

In 36999:

Twenty Thirteen, Twenty Fourteen and Twenty Fifteen: Fixes insufficient check for existence of DOM elements in jQuery object
Fixes #34033
Props: tywayne

#8 @ocean90
9 years ago

  • Milestone changed from Future Release to 4.5
Note: See TracTickets for help on using tickets.