Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#10910 closed task (blessed) (fixed)

Standardize comment form

Reported by: otto42's profile Otto42 Owned by: beaulebens's profile beaulebens
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

Every theme includes a substantially similar piece of code: the comments form. Largely, this form goes unchanged, except in extremely minor ways, from theme to theme. It's ripe for being pulled out of the theme and standardized in the core.

Points of note:

  • The form contains all the same elements (name, email, url, comment text), which must have all the same names in order for the comment form to actually work.
  • Virtually every theme uses styling to adjust the form, not changes to the HTML.
  • Any plugin which enhances comments has to have code manually inserted into the theme, in most all cases. A more standardized form could have useful action hooks around the elements in the form, letting plugins work on the form in a simpler way (adding fields to sane locations, etc).

I suggest that this proceed in phases:

  1. Copy the comments form code from default and classic themes into the core, and replace it with a function call. comment_form(), for example.
  1. Add action hooks everywhere in it. Maybe some filters on the minor amounts of text in it, if it makes sense to do so. Add to these as needed, as the theme authors complain about not being able to easily adjust the HTML anymore. Should be a minority, almost all themes leave this verbatim from whatever theme they started out with.
  1. Encourage themes to adjust. Maybe start suggesting themes in the Expand section to use it, like threaded comments did.

Attachments (14)

10910.diff (5.2 KB) - added by beaulebens 14 years ago.
First pass at adding a function for a standardized comment form. Totally open to actions/filters that should be in here
test-on-default-theme.diff (4.4 KB) - added by beaulebens 14 years ago.
Implements the other attached patch to demonstrate some flexibility with the new comments form function on the default theme.
10910.002.diff (5.1 KB) - added by beaulebens 14 years ago.
test-on-default-theme.002.diff (4.9 KB) - added by beaulebens 14 years ago.
10910.003.diff (5.1 KB) - added by beaulebens 14 years ago.
10910.comment_form.diff (625 bytes) - added by greenshady 14 years ago.
Use comment_form hook.
10910.004.diff (6.7 KB) - added by beaulebens 14 years ago.
Changes all markup to match the new default theme (2010) and adds the aria-required attributes back in where relevant. Also introduces comment_notes_before and _after actions.
comment-form-different.diff (8.6 KB) - added by greenshady 14 years ago.
Some ideas for changing the comment form.
example.diff (6.1 KB) - added by Otto42 14 years ago.
Example of the right way to do semantic markup
10910.005.diff (8.3 KB) - added by beaulebens 14 years ago.
The attached patch takes a swing at fixing the semanticness of the comment_form output, based largely on Otto42's patch. Also includes all the CSS modifications required for 2010 to look the same as it does currently with the new defaults form output.
10910.default-theme.001.diff (4.7 KB) - added by beaulebens 14 years ago.
This patch modifies the previous default/Kubrick to use the new comment_form() function, set up to output the same HTML as it did previously, with CSS to make it look the same where necessary. It's not *quite* perfect, but it's very, very close to the same HTML.
10910-phpDoc-typos-rename-suggestion.patch (1.5 KB) - added by demetris 14 years ago.
Two typos in the comment_form docs, and a rename suggestion
10910-cancel_comment_reply_link.patch (3.4 KB) - added by zeo 14 years ago.
In HTML5, small element represent different meaning from HTML4. Besides, small element probably the most hated element in the history of kubrick theme. Switching to span. Adds new parameter $before, $after to cancel_comment_reply_link(). Also checks when needed.
t10910-allowed-tags-default-simplified.diff (1.5 KB) - added by demetris 14 years ago.
Simplified and more appropriate semantically markup for Allowed HTML part

Download all attachments as: .zip

Change History (62)

#1 @ryan
14 years ago

+1

I'd love to be able to easily insert a login form or login prompt in the comment form area on wordpress.com. If we changed all themes to use a well-hooked comment_form(), this would be simple.

#2 follow-ups: @alexdunae
14 years ago

  • Cc alexdunae added

I agree with exposing the comment form from the default theme as a function in core, just for simplicity's sake, but I strongly disagree with only letting theme developers customize the form via hooks.

In the past I've added multiple fields to comment forms (marketing opt-in, hometown, etc...), removed the URL field and reduced the comment textarea to input[type=text]. I also frequently re-write the HTML to put the labels above the fields. To accomplish all that, I'd have to 'complain' to get a lot of hooks added.

So while it would be nice to be able to pull up a standard form when needed, it needs to be tempered with flexibility for theme creators.

#3 @scribu
14 years ago

It seems like a good ideea, as long as we aren't forced to use comment_form().

#4 in reply to: ↑ 2 @Otto42
14 years ago

Replying to alexdunae:

In the past I've added multiple fields to comment forms (marketing opt-in, hometown, etc...), removed the URL field, and reduced the comment textarea to input[type=text]. I also frequently re-write the HTML to put the labels above the fields. To accomplish all that, I'd have to 'complain' to get a lot of hooks added.

Nope. One filter hook can let you add labels, fields, whatever you like. Or remove existing ones, if done properly.

Consider every "field" to be in an array that looks like this:

array(
'name'=>'<label for="author">Name</label><input type="text"..blah blah..',
'email'=>'<label for="email">Email address</label><input type="text"..blah blah..',
.. blah blah...
)

Now, you run a filter on that array. Adding a new field is as simple as inserting a new element into the array. Delete an element from it and it disappears from the form. Want to redefine one? Just change the HTML in that one. Heck, you can even reorder the array if you like. One filter hook.

After the filter runs, a simple loop outputs the HTML code.

#5 in reply to: ↑ 2 @Otto42
14 years ago

Replying to alexdunae:

I also frequently re-write the HTML to put the labels above the fields.

You should do this with styling, not with changing the HTML. If your labels are in label tags and the inputs are, well, inputs, then some simple CSS will let you control their placement. No HTML editing required.

#6 @ryan
14 years ago

  • Milestone changed from Future Release to 3.0
  • Type changed from enhancement to task (blessed)

#7 @westi
14 years ago

  • Cc westi added

#8 @beaulebens
14 years ago

  • Cc beau@… added
  • Owner set to beaulebens
  • Status changed from new to assigned

#9 @caesarsgrunt
14 years ago

  • Cc caesar@… added

@beaulebens
14 years ago

First pass at adding a function for a standardized comment form. Totally open to actions/filters that should be in here

@beaulebens
14 years ago

Implements the other attached patch to demonstrate some flexibility with the new comments form function on the default theme.

#10 @beaulebens
14 years ago

Forgot to mention that the "test-on-default-theme.diff" patch also requires the patch on #11172 to provide an inline login form.

#11 @beaulebens
14 years ago

  • Keywords has-patch added

#12 @sirzooro
14 years ago

  • Cc sirzooro added

#13 follow-up: @ryan
14 years ago

I'm not really fond of having more globals. Can comment_form() call wp_get_current_commenter() and get_option('require_name_email') itself?

#14 in reply to: ↑ 13 @beaulebens
14 years ago

Replying to ryan:

I'm not really fond of having more globals. Can comment_form() call wp_get_current_commenter() and get_option('require_name_email') itself?

Good call ryan. The updated patch (10910.002.diff) does the following:

  • Uses wp_get_current_commenter() and get_option('require_name_email') instead of globals
  • Moves to a much more "arg-centric" approach, rather than individual filters on everything
  • Makes more of the form controllable via args (HTML ids, labels etc)
  • Uses site_url() for the form action
  • Adds a do_action('comment_form_top') right after the <form> tag
  • Moves the comment_form_before/after actions to only happen when comments are open, and adds a comment_form_comments_closed action which is triggered if comments are closed

Also adding the test-on-default-theme.002.diff file to work with this version, and patch 004 over on #11172, still showing some simple use of the new hooks, including the action for when comments are closed (close comments on a post to try it out).

#15 @Denis-de-Bernardy
14 years ago

  • Cc Denis-de-Bernardy added

#16 @Denis-de-Bernardy
14 years ago

two notes on the top of my head, on 10910.002.diff:

  • could we add apply_filters(the_permalink, get_permalink()) where applicable?
  • could we pass login/logout urls (complete with redirect parameters) to the login/logout captions? (Also, I think I only saw login captions; logout would be useful too.)

#17 @beaulebens
14 years ago

Attaching a revised patch with the apply_filters call on the permalink.

If you want to adjust login/logout urls you can just filter the string must_log_in and logged_in_as and replace the whole thing pretty easily. I guess we could add another filter there, but seems excessive.

#19 @demetris
14 years ago

  • Cc dkikizas@… added

#20 follow-up: @greenshady
14 years ago

Shouldn't this line:

<?php do_action( 'comments_form', $post_id ); ?>

Be:

<?php do_action( 'comment_form', $post_id ); ?>

#21 in reply to: ↑ 20 ; follow-up: @beaulebens
14 years ago

Replying to greenshady:

Shouldn't this line:

That was carried over from the code in the previous wp-content/themes/default/comments.php (and also in Sandbox) in an attempt to maintain compatibility with any themes or plugins already relying on that action.

#22 in reply to: ↑ 21 ; follow-up: @greenshady
14 years ago

Replying to greenshady:

Shouldn't this line:

That was carried over from the code in the previous wp-content/themes/default/comments.php (and also in Sandbox) in an attempt to maintain compatibility with any themes or plugins already relying on that action.

That's my point exactly. From the current default, classic, and pretty much all other theme comments.php files:

<?php do_action('comment_form', $post->ID); ?>

#23 in reply to: ↑ 22 @beaulebens
14 years ago

Replying to greenshady:

That's my point exactly. From the current default, classic, and pretty much all other theme comments.php files:

Bah! Good point, my bad. I'll see if I can get that fixed and/or add a patch to here if required. Thanks for catching it.

@greenshady
14 years ago

Use comment_form hook.

#24 follow-up: @Otto42
14 years ago

Can we get the default theme modified to use this, to show people how it should work?

#25 @ryan
14 years ago

(In [13028]) Use comment_form instead of comments_form as the name of the hook. Props greenshady. see #10910

#26 in reply to: ↑ 24 ; follow-up: @markmcwilliams
14 years ago

Replying to Otto42:

Can we get the default theme modified to use this, to show people how it should work?

Yeah, I like the sound of what's happening! ;) So are we still going to need to call comments_template('',true); from single/index/page.php (whatever) or if we switch that for comment_form(); will it bring in the standard comment form that's been talked about, which I think might just be the case? - But just so I'm clear! :)

@beaulebens
14 years ago

Changes all markup to match the new default theme (2010) and adds the aria-required attributes back in where relevant. Also introduces comment_notes_before and _after actions.

#27 @automattor
14 years ago

(In [13030]) Change comment_form() markup to match twentyten. Props beaulebens. see #10910

#28 in reply to: ↑ 26 @markmcwilliams
14 years ago

Replying to markmcwilliams:

Yeah, I like the sound of what's happening! ;) So are we still going to need to call comments_template('',true); from single/index/page.php (whatever) or if we switch that for comment_form(); will it bring in the standard comment form that's been talked about, which I think might just be the case? - But just so I'm clear! :)

Please do just ignore me, I got that 'ahhhh' moment, and while it wasn't quite what I expected in that respect, I do like having the standard area for people to write their comment and post it! :) I'm glad Otto created the ticket, so, thanks!

#29 @Otto42
14 years ago

Ugh! Why did it get changed to use all these non-semantic DIVs? The P's were correct markup, these DIVs are meaningless garbage. (The classes and arias are irrelevant and can go either way.)

If the 2010 theme actually looks like this, then the theme is fundamentally broken and needs to be fixed. We don't need to mess up the core code in this manner. Core should produce meaningful markup, not DIV-studded gibberish.

#30 @greenshady
14 years ago

As a theme author, the last thing I want to do is tell 1,000s of users, "Hey, look at this cool comment form function WP introduced. Yep, that's why your comments section is broken after upgrading."

If we're going to introduce markup, theme authors need to be able to override it. I'm specifically talking about the comment form title and the cancel comment reply link. Personally, I'd rather just have a callback function for creating my own comment form since I'm already filtering everything anyway.

And, what's with all the divs? Do we really need a <div> wrapping every single element of the form? I also agree with Otto about using <p> instead.

Patch coming that's a bit leaner. At the very least, take some ideas from it and make sure that we're not forcing theme authors into using specific markup.

@greenshady
14 years ago

Some ideas for changing the comment form.

#31 @Otto42
14 years ago

Also, all that "twentyten" stuff shouldn't be in there at all.

I think some of the basic ideas behind this have been lost. The goal here is to make a generic comment form that can be rather easily plugged into any and every theme.

-1 to the callback idea though. Callbacks just make things harder to standardize. Look at how callbacks have completely ruined wp_list_comments. If theme authors don't want to use the function, then they don't have to.

But, in order to make theme authors WANT to use the function, then it needs to fill a need and make life easier. This is overly complicated, IMO.

@Otto42
14 years ago

Example of the right way to do semantic markup

#32 follow-up: @Otto42
14 years ago

Ugh, even worse, the classes and id's are all wrong. This is non-semantic in every possible way. "form-section"? "form-label"? Are you serious? Just awful. Worst markup ever.

Here's a patch that's a bit closer to how it should be done. Not suggesting this as final, just throwing it out there as an example.

#33 in reply to: ↑ 32 ; follow-up: @greenshady
14 years ago

Replying to Otto42:

Ugh, even worse, the classes and id's are all wrong. This is non-semantic in every possible way. "form-section"? "form-label"? Are you serious? Just awful. Worst markup ever.

I didn't introduce those classes and IDs. I just used the ones already committed.

#34 in reply to: ↑ 33 @Otto42
14 years ago

Replying to greenshady:

I didn't introduce those classes and IDs. I just used the ones already committed.

I wasn't blaming anybody in particular. Just saying how wrong they are. :)

The ID's are wholly unnecessary. Classes are enough. You'd only need IDs if you're going to have more than one comment form on the page.

"form-section" and "form-label"? Come on. How generic can you get? The names of these things are supposed to be descriptive. If I wanted to address all the labels in forms using CSS, then there's a perfectly good way to do that: form label { whatever:xx; } Merely reproducing HTML in class-names is bloody stupid.

You really do need a wrapper around the fields. What if I want to hide the fields after an alternative login mechanism (Facebook, for example)? comment_form_before_fields and comment_form_after_fields are find for this, all you need is a div wrapper.

The 2010 theme as a whole has these same basic problems in several places, but those are better expressed in other tickets.

@beaulebens
14 years ago

The attached patch takes a swing at fixing the semanticness of the comment_form output, based largely on Otto42's patch. Also includes all the CSS modifications required for 2010 to look the same as it does currently with the new defaults form output.

@beaulebens
14 years ago

This patch modifies the previous default/Kubrick to use the new comment_form() function, set up to output the same HTML as it did previously, with CSS to make it look the same where necessary. It's not *quite* perfect, but it's very, very close to the same HTML.

#35 @ryan
14 years ago

(In [13180]) Comment form output tweaks. Props beaulebens. see #10910

#36 @holizz
14 years ago

  • Cc tom@… added

#37 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added

#38 @demetris
14 years ago

I attach a patch that fixes two potentially confusing typos in the phpDoc block, and also suggests renaming two action hooks:

comment_form_after_fields
comment_form_before_fields

to:

comment_form_fields_after
comment_form_fields_before

I think the suggested order is more logical.

Another thing I don’t like in the comment form is the markup for the allowed HTML elements: I think the Definition List is not the best choice here semantically, and it will also force people to use extra CSS to make that block look like one single paragraph, as it is most common in current usage. (Twenty Ten does exactly this.)

Why not go with the simplest option for the default? (Which people can then change easily if they so wish.) Something like:

'<p class="form-allowed-tags">
.   __( 'You may use... blah blah...:' )
.   '<code>' . allowed_tags() . '</code>'
.   </p>

Other than these minor niggles, I really enjoyed playing with comment_form. Thanks and congratulations to all who contributed!

@demetris
14 years ago

Two typos in the comment_form docs, and a rename suggestion

@zeo
14 years ago

In HTML5, small element represent different meaning from HTML4. Besides, small element probably the most hated element in the history of kubrick theme. Switching to span. Adds new parameter $before, $after to cancel_comment_reply_link(). Also checks when needed.

#39 follow-up: @nacin
14 years ago

demetris, I would upload a diff of the allowed tags issue if you feel strongly about it.

#40 @nacin
14 years ago

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

(In [14538]) Fix typos in phpdoc for comment_form(). props demetris, fixes #10910.

@demetris
14 years ago

Simplified and more appropriate semantically markup for Allowed HTML part

#41 in reply to: ↑ 39 @demetris
14 years ago

Replying to nacin:

demetris, I would upload a diff of the allowed tags issue if you feel strongly about it.

Done! :-)

#42 @nacin
14 years ago

Ok. It looks like beaulebens added the definition list markup when going over Otto42's suggestion of more semantic markup. I'd rather hear his reasoning before jumping to change it.

#43 @beaulebens
14 years ago

I switched to that just because it seemed to make sense semantically as far as this being a matching label/definition pair. I don't mind either way, so if people are bothered by it, then we can change it just as easily.

#44 @Otto42
14 years ago

Not sure I see the point of using a definition list here, as it's not really a list nor providing definitions. So I don't see the semantic sense in it.

+1 for switching to a simpler paragraph method.

Additional: Could we wrap the top fields in a fieldset? For separation purposes, it'd be easier to style a fieldset than trying to use those actions to add one, especially for multiple plugins to access it.

#45 @zeo
14 years ago

Any dev feedback for http://core.trac.wordpress.org/attachment/ticket/10910/10910-cancel_comment_reply_link.patch?

Just a thought, wouldn't it be nice to move the cancel reply out of the heading. Probably placing it after the comment form heading (before comment-notes) or next to Submit Comment.

#46 @zeo
14 years ago

  • Cc zeo@… added

#47 @nacin
14 years ago

(In [14689]) Use p instead of definition list for allowed tags in comment_form(). props demetris. Style the new markup in Twenty Ten. see #10910.

#48 @nacin
14 years ago

I responded to cancel_comment_reply link in #13198.

I think this one is finally fixed. Yay.

Note: See TracTickets for help on using tickets.