quicktags needs some refactoring

[garyc40]

Related: #16694

In quicktags.js, document.write() is used, which is obtrusive.

This also causes a bug where dragging a meta box containing quicktags would render a page blank, with only a line of buttons at the top.

[garyc40]

don't use document.write(), and output toolbar when DOM is ready

[garyc40]

Attached a patch​ for this.

Because the_editor() can be used on the front-end, in order for it to be light-weight (having no dependencies), I had to write a DOM ready function for quicktags.js . But it will still utilize jQuery function if it's already there.

[filosofo]

I don't think loading the buttons that fraction of a second earlier warrants the necessary extra code. 16695.2.diff is a simplified version; not as clever but it works in every JS-supporting client I know of.

[filosofo]

Also, if we're going to rewrite quicktags, let's get rid of some of that HTML-building duplication and generalize the edButton object to generate the markup (and attach its click callback) based on the parameters it already has.

I did this a couple years ago like so: (I'd change some things now, such as setting the DOM element properties instead of attributes, and it could just as well be an input element instead of a button.)

// button object
var Button = function(args) {
        args = args || {}; 
        var that = this;
        this.build = function() {
                var attrs = { 
                        accesskey:this.access,
                        'class':'qtags-button',
                        id:this.id,
                        type:'button'
                },  
                i, button = document.createElement('button');

                button.innerHTML = this.display;
                for ( i in attrs )
                        if ( attrs[i] )
                                button.setAttribute(i, attrs[i]);
                button.onclick = function() { that.callback.apply(that, that.callbackArgs) };
                return button;
        };  

        this.opentags = function() {
                return !! ( this.opens > 0 );
        };  

        this.opens = 0; // counts of open versions of this tag 
        this.display = args.display;
        this.tagStart = args.tagStart || '<' + this.display + '>';
        this.tagEnd = args.tagEnd || '</' + this.display + '>';
        this.id = args.id || 'qtags-' + this.display;
        this.callback = args.callback || function() {}; 
        this.callbackArgs = args.callbackArgs || []; 
        this.access = args.access;
        this.open = args.open || 0;
        this.el = this.build();
        return this;
}

[garyc40]

Replying to filosofo:

I don't think loading the buttons that fraction of a second earlier warrants the necessary extra code. 16695.2.diff is a simplified version; not as clever but it works in every JS-supporting client I know of.

The simplified version does work, but it calls the callback too late - when everything on the page has finished loading. That's the downside of the onload event.

In other words, if this function is used on the front end, where there is 500kb of css, scripts, images etc., the difference could be half a minute instead of a fraction of second. Not to mention this depends on the connection speed, not everyone has broadband.

[garyc40]

Replying to filosofo:

Also, if we're going to rewrite quicktags, let's get rid of some of that HTML-building duplication and generalize the edButton object to generate the markup (and attach its click callback) based on the parameters it already has.

Agreed.

There's lots of ugly code duplication in quicktags.dev that I'm addressing in an upcoming patch.

[garyc40]

completely refactor quicktags.dev

[garyc40]

remove console.log calls

[garyc40]

Attached 16695.4.diff​, which is a complete refactoring of quicktags.dev.js.

What has been improved:

• Code passes ​JSLint.

• Code duplication is addressed. Reduce total lines from 579 to 384.

• Unobtrusive Obtrusive JavaScript such as document.write() or onclick="edInsertButton(...)" is not used.

• Everything is properly namespaced, i.e. no more cluttered global scope.

• Quick Links is removed

• An API is provided for plugins and themes to add buttons to quicktags toolbar. Previously they either have to hack quicktags.js directly, or use the global edButtons array. The API is based on JavaScript prototypal inheritance pattern.

To add a button, a developer can extend QuickTags.Button or QuickTags.TagButton, override the callback to provide the desired functionality, then add the button by using:

QuickTags.registerButton(id, btnClass);

To extend the base classes:

// DivButton which inherits from TagButton
var DivButton = function() {
	// initialize properties, like parent::__construct() in PHP
	QuickTags.TagButton.call(this, 'div', 'div', '/div', 'd');
	
	// override the callback when the button is clicked
	this.callback = function(element, canvas, toolbar) {
		// do something
		// ...
		// then call the default TagButton callback, just like parent::callback() in PHP
		QuickTags.TagButton.prototype.callback.call(this, element, canvas, toolbar);
	};
};

// inherit all methods and properties from TagButton
DivButton.prototype = new QuickTags.TagButton();

This of course still needs more testing and feedback from core devs, but I believe it's definitely an improvement compared to the original code.

[scribu]

I agree with everything, except the API change.

The new API is over-engineered and breaks backwards compatibility:

​http://scribu.net/wordpress/right-way-to-add-custom-quicktags.html

[garyc40]

Providing backward compatibility is not hard. I was thinking about including it in my patch, but didn't have time to get around to it. We can always restore the global variables edButtons, as well as the edButton class (which will be a wrapper of the TagButton class).

Also, personally, I didn't consider the current way of adding quick tag button as in the article above an API. It's more like a hack.

I actually don't think this is over-engineering. Big change, yes. A bit harder to inherit and extend, yes. But if we have a chance to refactor it this time, why don't we do it right?

To make it easier for devs to inherit and extend base classes, we can always provide another abstraction layer like prototype does with Class.create() or Object.extend(). But that would make quicktags less lightweight.

[nacin]

I don't see how being harder to extend makes it better. We need to keep back compat here.

[garyc40]

OK, perhaps I should have explained a bit more.

• Providing backward-compatibility is not hard. I already made enough room in this patch to support backward compat. I'll update the patch in a couple of days.

• Creating a new tag button (equivalent to new ed_button() in the old approach) is basically similar function call, just replace edButton() with QuickTags.TagButton(). The argument order is the same.

So in short, when you look at it from outside, the interface is basically the same. Only difference is proper namespaces.

I shouldn't have chosen the word "harder", but instead "more advanced", since this new API allows devs to create new button classes that suit their needs. For example, custom buttons that have a custom callback that displays a lightbox etc.

[scribu]

Would you still be able to manipulate the button list array (remove, re-order)?

Having two methods of doing the exact same thing is not a good idea.

I think a better approach would be to wrap everything in a closure and just expose edButtons and edButton, with an additional callback argument.

[garyc40]

Replying to scribu:

Would you still be able to manipulate the button list array (remove, re-order)?

Yes you would after I provide back compat.

Having two methods of doing the exact same thing is not a good idea.

Which 2 methods, and which same thing?

I think a better approach would be to wrap everything in a closure and just expose edButtons and edButton, with an additional callback argument.

In my patch, all the things that should be private are already in a closure. Only QuickTags class and public classes that belong to that namespace are exposed.

[scribu]

Ok, what I'm trying to say is: Since you can't get rid of edButtons and edButton, there's no point in exposing a new QuickTags class.

In general, it's better to extend the existing API than to deprecate it. And yes, edButtons and edButton are API.

[garyc40]

There IS a point. It's there to support multiple instances of quicktags toolbar (Quick edit comment in Post edit page, for example). Actually, it might be preferable if I restore its old name (QTags). So technically I'm just extending the old class, but with better structure and more possibilities.

[scribu]

Ok, well you din't mention that anywhere. :)

[garyc40]

backward compatibility with edButtons and edButton

[garyc40]

Replying to scribu:

Ok, well you din't mention that anywhere. :)

Sorry for the confusion. Just attached a patch that restores the class name to QTags instead of QuickTags, and add 2 globals variables edButtons as well as edButton that maps nicely to the new API. ​The code sample that you linked to works perfectly fine with the new API now.

Hopefully the outcome of this ticket will allow for a more formal quicktag API than what we have now. I'm willing to further update this ticket to comply with core commiters' direction.

[azaozz]

This is an interesting project and the patches look good. Although I don't think it only needs "some refactoring". IMHO it needs full rewriting.

For example it would be nice to have init options similar to the way TinyMCE works, i.e. an init object that overrides the internal defaults: instances can be added to several textareas by ID (or even CSS class), buttons can be enabled|disabled|rearranged, the toolbar can start hidden, etc. Also a clear documented API for extending the editor (well, adding buttons), maybe build-in resizing of the textareas, some basic CSS overrides from the JS like font and background color, font face, line spacing, etc.

I'm even debating the possibility of just adding this functionality into a TinyMCE plugin for the main WP editor and keeping quicktags.js for places where we need limited HTML code.

Perhaps we can have this new editor in 3.3 :)

[scribu]

"maybe build-in resizing of the textareas" - most modern browsers already do that automatically for all textareas, so I don't think it's worth wasting time on that.

I'm even debating the possibility of just adding this functionality into a TinyMCE plugin for the main WP editor and keeping quicktags.js for places where we need limited HTML code.

I don't really undertand which functionality you are refering to. If it's rearanging buttons, I would consider it plugin territory.

[azaozz]

Replying to scribu:

"maybe build-in resizing of the textareas" - most modern browsers already do that automatically for all textareas, so I don't think it's worth wasting time on that.

True but we want it saved in a cookie so we can size the Visual editor when switching and resize it on page load.

I don't really undertand which functionality you are refering to...

The whole HTML editor functionality. TinyMCE can be hidden on demand exposing the textarea but is still active in the background, so a plugin that adds buttons for easy entering of HTML tags there would work nicely.

[scribu]

What about users that have permanently disabled TinyMCE in their profile settings? You would then be forced to load it all the time.

[azaozz]

No, I meant we can init and call the (new) functions/methods in quicktags.js from a TinyMCE plugin, so there will be no need to separately attach quicktags to the textarea, etc. If TinyMCE is included, we only will need to load quicktags.js and the editor switching, hiding the toolbar, filtering, etc. will be handled from whitin TinyMCE (which would also make it a lot easier for plugins to use both editors in the same way as on the Write screen).

Of course quicktags.js will remain a separate JS script that can be attached to any textarea.

Actually I should open a new ticket for that for 3.3.

[azaozz]

See #17144.

[azaozz]

In [18497]:

Refactor Quicktags, props garyc40, see #16695

[scribu]

There's a console.debug() in edit-comments.dev.js.

[azaozz]

In [18499]:

Remove debug remnants, props scribu, see #16695

[Viper007Bond]

"Insert into post" is now broken for attachments in the thickbox.

Error: quicktags.insertContent is not a function
Source File: http://www.viper007bond.com/wordpress/wp-admin/post.php?post=3210&action=edit&message=10
Line: 1321

[azaozz]

In [18511]:

Fix back-compat for quicktags, introduce QTags.addButton(), see #16695

[SergeyBiryukov]

16695.closeAllTags-fix.patch​ fixes "close tags" button, broken since [18497].

[SergeyBiryukov]

The patch actually fixes qt.CloseButton.prototype.callback(), however qt.prototype.closeAllTags() still doesn't seem to work (or I can't figure out how to call it properly from edit-comments.dev.js). Trying to refresh the patch from #15911.

[azaozz]

In [18575]:

Fix the "close tags" button in quicktags, props SergeyBiryukov, see #16695

[azaozz]

Replying to SergeyBiryukov:
Seems to be working properly in all places.

[azaozz]

In [18576]:

Fix QTags.closeAllTags(), replace 'tb' with 'ed' in quicktags,js to make it clear it is the editor instance not the toolbar, small comments quick edit fixes, see #16695, fixes #15911

[azaozz]

In [18611]:

Improve quicktags back-compat, add method instance.getButtonElement(id), see #16695

[azaozz]

In [18612]:

Properly reset vars, see #16695

[azaozz]

In [18619]:

Fix styling and quicktags error in press this, see #16695

[westi]

Replying to azaozz:

In [18612]:

Properly reset vars, see #16695

var t = this, instance, canvas, name, settings, buttons, theButtons, html, id, i, qb, btn;;

Is the double semi-colon on the end intentional?

[azaozz]

In [18623]:

Fix typo in quicktags.js, props westi, see #16695

[azaozz]

Replying to westi:

It's a "no-error" typo :) even the minifier removes the second semicolon.

[azaozz]

In [18628]:

More quicktags back-compat, see #16695

[scribu]

[18628] introduced a typo: 'hask' :)

[azaozz]

Replying to scribu:

Yes, saw that too late, will fix in the next patch :)

[nacin]

In [18629]:

s/hask/hack/. props scribu, see #16695.

[trepmal]

I'm having trouble getting self-closing tags to work using QTags.addButton but I'm not sure if it's a bug or if I just have bad code.

Here's what I have:

add_action( 'admin_print_footer_scripts', 'kl_footer_scripts', 20 );
function kl_footer_scripts() {
	?>
	<script type='text/javascript'>
		QTags.addButton('peb_0', 'H4', "<h4>", "</h4>");
		QTags.addButton('peb_1', 'NP', "<!--nextpage-->", "", -1);
	</script>
	<?php
}

The H4 tag works fine, but NP won't - the button simply doesn't appear in the toolbar.

If I remove the arg2 check in quicktags.dev.js (line 340), then the NP button works perfectly.

I can upload a patch if needed, I'm just not sure if the problem can be solved by making my own code better.

[azaozz]

Replying to trepmal:

Right, [18725] fixes the 4th arg and removes the '-1' for self closing tags as it's redundant.

[trepmal]

If the pointer is in an empty HTML mode editor, the first 2 clicks of the "b" button* will add opening tags.

So, if you click "b" then type "lorem ipsum" and click "b" again, you get

<strong>lorem ipsum<strong>

Instead of

<strong>lorem ipsum</strong>

*or any of the buttons with opening and closing tags

[trepmal]

the "close tags" button won't close the first open tag either.

[azaozz]

In [18727]:

Quicktags: fix closing of the first tag in an empty textarea, allow closing of all tags without the button, see #16695

[jane]

Feature freeze. Closing this ticket as a lot more has been done beyond initial ticket description. For future quicktags enhancements, please open separate new tickets.