Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#19098 closed defect (bug) (fixed)

QTags.addButton not working according to the docblocks

Reported by: amereservant's profile amereservant Owned by: azaozz's profile azaozz
Milestone: 3.3 Priority: normal
Severity: minor Version: 3.3
Component: General Keywords:
Focuses: Cc:

Description

Hi, I'm using the latest svn trunk for 3.3 and I was trying to modify the quicktags bar according to the recently re-written quicktags.js.

Following the author's instructions in the docblock in quicktags.dev.js, http://core.trac.wordpress.org/browser/trunk/wp-includes/js/quicktags.dev.js#L305 and hooking the admin_print_footer_scripts action hook, it doesn't render the added buttons.
Here's my example code placed in my plugin's function file:

<?php
if( !function_exists('_add_my_quicktags') ):

function _add_my_quicktags()
{ ?>
<script type="text/javascript">
/* Add H1 Tag */
QTags.addButton('h1', 'h1', '<h1>', '</h1>');
</script>
<?php }
add_action('admin_print_footer_scripts', '_add_my_quicktags', 100);

endif; // End of "!function_exists('_add_my_quicktags')"

I can view the source in the browser and it's outputting the code as it should. I've also replaced the contents of quicktags.js with quicktags.dev.js and changed Lines 356-358 to the following to check if my buttons are being added:

                } else {
			edButtons[edButtons.length] = btn;
			if(id == 'h1') console.log(edButtons);
		}

The answer was 'yes', but they still are not rendering.

If I've missed something, please let me know, but I believe this seems to be a bug with the rewritten code.

Thanks!

Change History (10)

#1 @amereservant
12 years ago

  • Severity changed from normal to minor

Ok, I figured it out. If a filter hook is added to quicktags_settings and values are given for the buttons variable, then it will reject any additionally added buttons.

Here's an example:

<?php
if( !function_exists('_default_quicktags') ):

function _default_quicktags( $qtInit, $e_id )
{
    $qtInit['buttons'] = 'em,strong,block';
    return $qtInit;
}
add_filter('quicktags_settings', '_default_quicktags', 10, 2);

endif;

Now if I also use the previously mentioned code to add tags, it will ignore them since it appears if any values have been added to the $qtInitbuttons? variable will reject all other added buttons.
I'm not sure if this is a preferred behaviour or not, but these can conflict in certain circumstances as I discovered it by accident.

Thanks!

#2 @ocean90
12 years ago

  • Owner set to azaozz
  • Status changed from new to reviewing

#3 @azaozz
12 years ago

This behavior is described in the phpdoc in quicktags.js. It may not be the most intuitive (for developers) as we don't have "real" hooks system for JS that mimics the php filters. Suggestions (and patches) for improving it are welcome.

#4 @amereservant
12 years ago

Tell me what you think of this idea...

There's a couple of ways to do this and one idea is using a global such as $wp_quicktag_buttons and then you could create two functions, add_quicktag_button($id, $title, $arg1, $arg2, $access_key, $priority) and remove_quicktag_button($id) that modify the $wp_quicktag_buttons global.

In WP_Editor::editor_settings, add the default buttons (like they have been added in quicktags.js at the bottom) and then add them to the $qtInit variable before calling the 'quicktags_settings' filter, which would give developers a chance to filter out buttons they don't want or add new ones at the last minute, although they should use the two previously mentioned functions and hook to the quicktags_settings filter at the latest.

The value of $wp_quicktag_buttons could be a simple array such as:

$wp_quicktag_buttons = array(
    10 => array(
              'id'         => 'strong',
              'title'      => 'b',
              'arg1'       => '<strong>',
              'arg2'       => '</strong>',
              'accesskey'  => 'b'
    ),
    20 => array(
              'id'         => 'em',
              'title'      => 'i',
              'arg1'       => '<em>',
              'arg2'       => '</em>',
              'accesskey'  => 'i'
    ),
);

Finally, once that is converted into a JSON-Encoded object and added to the qtInit object, the values can easily be iterated over in the quicktags.js and added, similar to how they are added at the bottom now.
This would allow you to do away with the disabled_buttons variable in the $qtInit array and the buttons variable could be changed to the value of $wp_quicktag_buttons, making it an array instead of a comma-separated string.

I'm not sure what else would need to be changed to accommodate these changes since I'm not spending too much time on this idea until I get feedback, but it seems very easy to implement with very little changes to the current code and would allow greater flexibility in the future.

I'd be glad to write the patches for this if you like the idea, but I wanted to get feedback first in case there's something I'm overlooking or if this doesn't sound like a good idea to anyone but me. Thanks!

#5 follow-up: @azaozz
12 years ago

If we make the Quicktags buttons depend on a PHP var/array, we could filter them the "normal" way (similar to how the TinyMCE buttons are handled). Was thinking about that earlier. The downside is that Quicktags stops being a self-contained script, i.e. it will have to always be used together with WP_Editor class.

#6 in reply to: ↑ 5 @amereservant
12 years ago

Replying to azaozz:

If we make the Quicktags buttons depend on a PHP var/array, we could filter them the "normal" way (similar to how the TinyMCE buttons are handled). Was thinking about that earlier. The downside is that Quicktags stops being a self-contained script, i.e. it will have to always be used together with WP_Editor class.

Well it wouldn't be any different from it currently relying on quicktagsL10n, which is a localized variable created by PHP. And like I mentioned above, it could still run independently (not sure why or when it would need to) from WP_Editor by checking if the buttons added by WordPress exist or not before beginning to iterate over them.

Considering the fact quicktags.js is meant to be used with the editor, I don't know why integrating it to work with WordPress' WP_Editor class would be a drawback. If someone wants to use quicktags.js for other uses than how it can be used in WordPress, then they can always download it from Alex's site.
Making it integrate better with WordPress so users can further customize WordPress for their needs would seem to me to be the better choice since WordPress is meant to work together and yet provide a great deal of flexibility to the developer building upon it.

Here's sorta what I was thinking starting at http://core.trac.wordpress.org/browser/trunk/wp-includes/js/quicktags.dev.js#L633, although this is a rough concept to show the idea because I'm not taking the scope of the settings variable into account:

if( typeof settings.buttons == 'undefined' ) return;
for( i=0; i < settings.buttons.length; i++ ) {
    // If the keys are the priority order, this will need to be done differently...
    if( typeof settings.buttons[i] != 'undefined' ) {
        var curBtn = settings.buttons[i];
        edButtons[i] = new qt.TagButton(curBtn.id, curBtn.title, curBtn.arg1, curBtn.arg2, curBtn.accesskey);
    }
}

I dunno, that's just my 2 1/2 cents on it.... with the quicktagsL10n variable being relied upon within the js already, I don't see where it'd make much difference. A simple test for if( qtInit.buttons typeof 'undefined' ) return; before adding the buttons would make it just as flexible as now.

#7 @azaozz
12 years ago

Went ahead and reviewed the whole Quicktags init and addButton process. Don't think the disabled_buttons were useful there, removed that and made the init.buttons control better.

Also adjusted the priority, plugins can override init.buttons:

  • all buttons added by plugins are shown,
  • buttons can be added to specific instance,
  • default buttons can be removed from an instance by not setting their IDs in the init,
  • if the arg is not specified, WP_Editor sets all default buttons.
Last edited 12 years ago by azaozz (previous) (diff)

#8 @azaozz
12 years ago

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

In [19172]:

Improve handling of init and adding buttons to Quicktags, fixes #19098

#9 @azaozz
12 years ago

In [19173]:

Phpdoc fix for quicktags.js, see #19098

#10 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.3
Note: See TracTickets for help on using tickets.