Make WordPress Core

Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#16695 closed enhancement (fixed)

quicktags needs some refactoring

Reported by: garyc40's profile garyc40 Owned by: azaozz's profile azaozz
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.1
Component: Editor Keywords:
Focuses: Cc:

Description

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.

Attachments (6)

16695.diff (5.0 KB) - added by garyc40 13 years ago.
don't use document.write(), and output toolbar when DOM is ready
16695.2.diff (4.0 KB) - added by filosofo 13 years ago.
16695.3.diff (29.4 KB) - added by garyc40 13 years ago.
completely refactor quicktags.dev
16695.4.diff (29.4 KB) - added by garyc40 13 years ago.
remove console.log calls
16695.5.diff (29.1 KB) - added by garyc40 13 years ago.
backward compatibility with edButtons and edButton
16695.closeAllTags-fix.patch (402 bytes) - added by SergeyBiryukov 13 years ago.

Download all attachments as: .zip

Change History (63)

@garyc40
13 years ago

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

#1 @garyc40
13 years ago

  • Keywords has-patch added; needs-patch removed

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.

#2 follow-up: @filosofo
13 years ago

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
13 years ago

#3 follow-up: @filosofo
13 years ago

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;
}

#4 in reply to: ↑ 2 @garyc40
13 years ago

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.

#5 in reply to: ↑ 3 @garyc40
13 years ago

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
13 years ago

completely refactor quicktags.dev

@garyc40
13 years ago

remove console.log calls

#6 @garyc40
13 years ago

  • Keywords dev-feedback 3.2-early added

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

What has been improved:

  • 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.

Last edited 13 years ago by garyc40 (previous) (diff)

#7 @scribu
13 years ago

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

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

#8 @garyc40
13 years ago

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.

#9 @nacin
13 years ago

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

#10 @garyc40
13 years ago

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.

#11 follow-up: @scribu
13 years ago

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.

Last edited 13 years ago by scribu (previous) (diff)

#12 in reply to: ↑ 11 @garyc40
13 years ago

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.

#13 @scribu
13 years ago

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.

#14 @garyc40
13 years ago

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.

#15 follow-up: @scribu
13 years ago

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

@garyc40
13 years ago

backward compatibility with edButtons and edButton

#16 in reply to: ↑ 15 @garyc40
13 years ago

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.

#17 @azaozz
13 years ago

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

Last edited 13 years ago by azaozz (previous) (diff)

#18 follow-up: @scribu
13 years ago

"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.

#19 in reply to: ↑ 18 @azaozz
13 years ago

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.

I'll be adding the functions from our editor.js in a TinyMCE plugin anyways, no need to load a separate file for them.

Last edited 13 years ago by azaozz (previous) (diff)

#20 @scribu
13 years ago

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

#21 @azaozz
13 years ago

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.

Last edited 13 years ago by azaozz (previous) (diff)

#23 @azaozz
13 years ago

  • Milestone changed from Awaiting Review to 3.3

#24 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

#25 @nerrad
13 years ago

  • Cc nerrad added

#26 @azaozz
13 years ago

In [18497]:

Refactor Quicktags, props garyc40, see #16695

#27 @scribu
13 years ago

  • Keywords 3.2-early removed

#28 @scribu
13 years ago

  • Keywords needs-patch added; has-patch removed

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

#29 @scribu
13 years ago

  • Owner changed from scribu to azaozz

#30 @ocean90
13 years ago

  • Cc ocean90 added
  • Keywords dev-feedback removed

#31 @azaozz
13 years ago

In [18499]:

Remove debug remnants, props scribu, see #16695

#32 @Viper007Bond
13 years ago

"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

#33 @azaozz
13 years ago

In [18511]:

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

#34 @SergeyBiryukov
13 years ago

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

#35 follow-up: @SergeyBiryukov
13 years ago

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.

#36 @azaozz
13 years ago

In [18575]:

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

#37 in reply to: ↑ 35 @azaozz
13 years ago

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

#38 @azaozz
13 years ago

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

#39 @azaozz
13 years ago

In [18611]:

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

#40 follow-up: @azaozz
13 years ago

In [18612]:

Properly reset vars, see #16695

#41 @azaozz
13 years ago

In [18619]:

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

#42 in reply to: ↑ 40 ; follow-up: @westi
13 years ago

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?

#43 @azaozz
13 years ago

In [18623]:

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

#44 in reply to: ↑ 42 @azaozz
13 years ago

Replying to westi:

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

#45 @azaozz
13 years ago

In [18628]:

More quicktags back-compat, see #16695

#46 follow-up: @scribu
13 years ago

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

#47 in reply to: ↑ 46 @azaozz
13 years ago

Replying to scribu:

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

#48 @nacin
13 years ago

In [18629]:

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

#49 follow-up: @trepmal
13 years ago

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.

#50 in reply to: ↑ 49 @azaozz
13 years ago

Replying to trepmal:

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

#51 @trepmal
13 years ago

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

#52 @trepmal
13 years ago

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

#53 @azaozz
13 years ago

In [18727]:

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

#54 @jane
12 years ago

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

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.

#55 @jane
12 years ago

  • Milestone 3.3 deleted

#56 @jane
12 years ago

  • Milestone set to 3.3

#57 @jane
12 years ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.