Make WordPress Core

Opened 8 years ago

Last modified 3 years ago

#34130 new defect (bug)

Thickbox previous arrow

Reported by: eric3d's profile Eric3D Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 4.3.1
Component: External Libraries Keywords: needs-testing close
Focuses: javascript Cc:

Description

The previous arrow in Thickbox does not work reliably. It sometimes displays 2 images, sometimes displays just the caption.

Since that problem does not happen with the buttons, I suggest using the "trigger" function instead. The following code seems to work more reliably (staring at line 137):

function goPrev(){
	if(jQuery(document).unbind("click",goPrev)){jQuery(document).unbind("click",goPrev);}
	jQuery("#TB_window").remove();
	jQuery("body").append("<div id='TB_window'></div>");
	tb_show(TB_PrevCaption, TB_PrevURL, imageGroup);
	return false;
}
jQuery("#TB_prev").click(goPrev);

function goNext(){
	jQuery("#TB_window").remove();
	jQuery("body").append("<div id='TB_window'></div>");
	tb_show(TB_NextCaption, TB_NextURL, imageGroup);
	return false;
}
jQuery("#TB_next").click(goNext);

jQuery(document).bind('keydown.thickbox', function(e){
	if ( e.which == 27 ){ // close
		tb_remove();
	} else if ( (e.which == 190) || (e.which == 39) ){ // display next image
		jQuery("#TB_next").trigger( "click" );
	} else if ( (e.which == 188) || (e.which == 37) ){ // display previous image
		jQuery("#TB_prev").trigger( "click" );
	}
	return false;
});

Attachments (1)

34130.diff (2.2 KB) - added by antpb 8 years ago.

Download all attachments as: .zip

Change History (12)

@antpb
8 years ago

#1 @antpb
8 years ago

  • Keywords has-patch dev-feedback needs-testing added

This looks right but I am not as js savvy as I would like to be so I think we'll need dev feedback. Patch attached.

#2 @antpb
8 years ago

  • Severity changed from normal to minor

#3 @swissspidy
8 years ago

  • Component changed from Gallery to External Libraries

Hey there

Thanks for your report. Thickbox is an external JS library bundled with WordPress. So usually you should report bugs there. It hasn't been updated since 2007 though, see #31726.

Not sure if it really makes sense to fix this here.

#4 @helen
8 years ago

  • Keywords reporter-feedback added; dev-feedback removed

Do you mind describing how you're getting to this Thickbox instance? Core doesn't use it for media functions anymore, and I don't recall any usage of previous/next functionality in the past, but certainly could be missing something. Screenshots may also be helpful here.

#5 follow-up: @Eric3D
8 years ago

@helen, I just enqueue the script in functions.php

function include_thickbox_scripts()
{
    wp_enqueue_script('thickbox', null, array('jquery'));
}
add_action('wp_enqueue_scripts', 'include_thickbox_scripts');

Then I use jQuery to add class and rel value to the WP gallery items

$(".gallery-icon a").addClass("thickbox").attr("rel","gallery");

I understand it's no longer used in core, but some plugins still use it.

@swissspidy, I understand Thickbox is old and not updated. I did not find an official Github (or other) depository for it. Since the thread you refer accepted the change, the file bundled in core is NO LONGER the same as the original. Would it make sense to create a separate branch as "WordPress Thickbox"?

#6 @Eric3D
8 years ago

Update:
In another plugin, I simply used <?php add_thickbox(); ?> as described in the codex
https://codex.wordpress.org/Javascript_Reference/ThickBox

Based on the description on the codex page, the bundled library is modified from Cody Lindley's release. So it makes sense to continue modifications here instead of going back to the source.

#7 in reply to: ↑ 5 @afercia
8 years ago

Replying to Eric3D:

I understand it's no longer used in core, but some plugins still use it.

To be honest, it is still used in core. For example, the plugin details modal uses it. Also, on multisite, the "View version X details" displayed in the Themes list table when there are available updates, opens a Thickbox modal.
I'm not aware of any core usage of the previous/next functionality though.

Thickbox has been updated several times in the past, also very recently. It is now, I'd say, a "custom WordPress" thickbox implementation. By the way, I'd rater prefer to replace it with some more modern tool instead of trying to still support its original functionalities. Any thoughts welcome.

#8 @swissspidy
7 years ago

  • Keywords close added

#9 @hellofromTonya
3 years ago

  • Keywords reporter-feedback removed

Removes reporter-feedback as feedback was given.

This ticket is marked for close. What do we think about the next step for this ticket?

#10 @sabernhardt
3 years ago

  • Focuses javascript added
  • Keywords has-patch removed

The WordPress (5.7) version of Thickbox already has .trigger('click') as of [50367], so the next step would be testing to confirm whether that is sufficient.

current Thickbox, starting at line 146

#11 @sabernhardt
3 years ago

Correction: it was switched from .trigger('click') to .on('click') in [50410]

Note: See TracTickets for help on using tickets.