WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#10910 closed task (blessed) (fixed)

Standardize comment form

Reported by: Otto42 Owned by: 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 5 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 5 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 5 years ago.
test-on-default-theme.002.diff (4.9 KB) - added by beaulebens 5 years ago.
10910.003.diff (5.1 KB) - added by beaulebens 4 years ago.
10910.comment_form.diff (625 bytes) - added by greenshady 4 years ago.
Use comment_form hook.
10910.004.diff (6.7 KB) - added by beaulebens 4 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 4 years ago.
Some ideas for changing the comment form.
example.diff (6.1 KB) - added by Otto42 4 years ago.
Example of the right way to do semantic markup
10910.005.diff (8.3 KB) - added by beaulebens 4 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 4 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 4 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 4 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 4 years ago.
Simplified and more appropriate semantically markup for Allowed HTML part

Download all attachments as: .zip

Change History (62)

comment:1 ryan5 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.

comment:2 follow-ups: alexdunae5 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.

comment:3 scribu5 years ago

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

comment:4 in reply to: ↑ 2 Otto425 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.

comment:5 in reply to: ↑ 2 Otto425 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.

comment:6 ryan5 years ago

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

comment:7 westi5 years ago

  • Cc westi added

comment:8 beaulebens5 years ago

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

comment:9 caesarsgrunt5 years ago

  • Cc caesar@… added

beaulebens5 years ago

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

beaulebens5 years ago

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

comment:10 beaulebens5 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.

comment:11 beaulebens5 years ago

  • Keywords has-patch added

comment:12 sirzooro5 years ago

  • Cc sirzooro added

comment:13 follow-up: ryan5 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?

comment:14 in reply to: ↑ 13 beaulebens5 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).

beaulebens5 years ago

comment:15 Denis-de-Bernardy5 years ago

  • Cc Denis-de-Bernardy added

comment:16 Denis-de-Bernardy5 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.)

comment:17 beaulebens4 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.

beaulebens4 years ago

comment:19 demetris4 years ago

  • Cc dkikizas@… added

comment:20 follow-up: greenshady4 years ago

Shouldn't this line:

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

Be:

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

comment:21 in reply to: ↑ 20 ; follow-up: beaulebens4 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.

comment:22 in reply to: ↑ 21 ; follow-up: greenshady4 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); ?>

comment:23 in reply to: ↑ 22 beaulebens4 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.

greenshady4 years ago

Use comment_form hook.

comment:24 follow-up: Otto424 years ago

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

comment:25 ryan4 years ago

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

comment:26 in reply to: ↑ 24 ; follow-up: markmcwilliams4 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! :)

beaulebens4 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:27 automattor4 years ago

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

comment:28 in reply to: ↑ 26 markmcwilliams4 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!

comment:29 Otto424 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.

comment:30 greenshady4 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.

greenshady4 years ago

Some ideas for changing the comment form.

comment:31 Otto424 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.

Otto424 years ago

Example of the right way to do semantic markup

comment:32 follow-up: Otto424 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.

comment:33 in reply to: ↑ 32 ; follow-up: greenshady4 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.

comment:34 in reply to: ↑ 33 Otto424 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.

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

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

comment:35 ryan4 years ago

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

comment:36 holizz4 years ago

  • Cc tom@… added

comment:37 mikeschinkel4 years ago

  • Cc mikeschinkel@… added

comment:38 demetris4 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!

demetris4 years ago

Two typos in the comment_form docs, and a rename suggestion

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

comment:39 follow-up: nacin4 years ago

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

comment:40 nacin4 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.

demetris4 years ago

Simplified and more appropriate semantically markup for Allowed HTML part

comment:41 in reply to: ↑ 39 demetris4 years ago

Replying to nacin:

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

Done! :-)

comment:42 nacin4 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.

comment:43 beaulebens4 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.

comment:44 Otto424 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.

comment:45 zeo4 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.

comment:46 zeo4 years ago

  • Cc zeo@… added

comment:47 nacin4 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.

comment:48 nacin4 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.