WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Consider adding H2 Heading on single posts or pages when comments are enabled #43203

Closed annezazu closed 2 years ago

annezazu commented 2 years ago

What problem does this address?

Pulling out of this trac issue on adding an "accessibility ready" tag for the Twenty Twenty-Two Theme. From @carolinan who did the audit:

On single posts or pages, when comments are enabled, the site title is an H1, the post title is an H1, but the next title is the "Leave a Reply" title and it is an H3, so the H2 level is skipped. If there are comments, the "One response to “title” is present, but it is also an H3: H1 Site Title H1 Sample Page H3 One response to “Sample Page” H3 Leave a Reply

What is your proposed solution?

From @carolinan in that ticket:

There are a few different ways to solve this, but I think that the best option, that would work for both older versions of WP and 6.1, is to add a new heading above the comments area.

Further from DM with @joedolson:

Structurally, it would be better to have an additional heading, because of what this structure looks like once there are comments present. With comments, there's an additional heading "One response to "{post title}"", which is also an H3. That heading should be an h2, and should be present whether there are comments or not, so it provides structural context.

cc @SantosGuillamot @ockham do you have thoughts here, considering your work with the Comments block? This isn't a severe, deal breaking issue but jumping from H1 to H3 is generally discouraged.

ockham commented 2 years ago

IIRC this was discussed on another PR where @c4rl0sbr4v0 had a bit more context.

cbravobernal commented 2 years ago

I will take a look at it, as it was mentioned on this PR by @justintadlock .

Also, on that PR, we set H2 as the default one: https://github.com/WordPress/gutenberg/pull/40419/files#diff-f1c094255127dfb8013473531eabaa07f7cd0029bc4bdcb90db3b7745d9c2e25

I also checked Core code, and it puts H2 as a default: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/blocks/comments-title.php

ockham commented 2 years ago

I will take a look at it, as it was mentioned on this PR by @justintadlock .

Ah, that's exactly what I was thinking of (but couldn't remember where to find it).

Also, on that PR, we set H2 as the default one: https://github.com/WordPress/gutenberg/pull/40419/files#diff-f1c094255127dfb8013473531eabaa07f7cd0029bc4bdcb90db3b7745d9c2e25

I also checked Core code, and it puts H2 as a default: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/blocks/comments-title.php

Ah, indeed! Thanks Carlos!

I feel we need to provide a bit more context:

Twenty Twenty-Two uses the Post Comments block to render comments. However, per WP 6.0, we've deprecated that block in favor of the Comments Query Loop block (which we've since renamed to (just) Comments). And in that block, the One response to “Sample Page” header is indeed an H2!

I actually just had an idea or two on how we might resolve the issue; I'll try it out and will post an update in a bit!

ockham commented 2 years ago

Okay, I think there might be a few different solutions. I'll provide a bit more context for starters (as it helps inform those solutions):

The Post Comments block is really just a thin wrapper around comments_template. Since we’re calling it without arguments, it uses Core's theme-compat comments.php template to render comments — which happens to have a H3 hardcoded for the "One response to…" heading. (I'll add that the theme-compat comments.php has a deprecation note which I hadn't previously noticed.)


OTOH, in more recent versions of Gutenberg, we've actually merged the Post Comments block into the Comments block, and added a banner in the editor on top of the legacy Post Comments block that allows users to convert it to Comments with one click:

Bildschirmfoto 2022-08-16 um 17 15 56

(The copy is a bit verbose BTW -- my bad. Maybe @annezazu @carolinan have some suggestions?)

This works well enough when the user loads a template with an existing Post Comments block in the editor -- they'll see the prompt which will allow them to easily convert the block to a Comments block (which comes with the desired H2). But of course, no blocks are converted if the user doesn't edit the containing template in the Site Editor.


So I think the optimal solution would be if TT2 could be updated to use the Comments block (instead of the Post Comments block): This would turn the "One response to…" heading into an H2 and provide better UX for handling comments in the Site Editor. However, I don't think that's possible, since the Comments (Query Loop) block was only part of WP 6.0, whereas TT2 is also available on sites running older versions of WP. And while it's obviously possible to have conditional logic in PHP themes, I don't think we can have a block theme that conditionally uses different blocks depending on their availability in different WP versions (correct me if I'm wrong).

So instead, we can tackle the problem at the (PHP) comments_template() level (as called by the Post Comments block). This would likely mean including a copy of Core's theme-compat comments.php (with the slight modification to use an H2 rather than an H3 for "One response to...") with the TT2 theme. I don't love the idea of including more PHP code with a block template, but it's probably the most effective way of fixing the issue.

(Fortunately, we won't have to do anything like that for newer themes, now that the Comments (Query Loop) block is also available.)

Curious to hear y'all's thoughts on this.

ockham commented 2 years ago

This would likely mean including a copy of Core's theme-compat comments.php (with the slight modification to use an H2 rather than an H3 for "One response to...") with the TT2 theme.

I just verified that this works, i.e. in wordpress-develop, run

cp ./src/wp-includes/theme-compat/comments.php ./src/wp-content/themes/twentytwentytwo/
sed -i '' 's/h3/h2/g' ./src/wp-content/themes/twentytwentytwo/comments.php
ockham commented 2 years ago

Hmm, I just realized that this doesn't really solve the a11y issue when there are no comments though, as there will be only the "Leave a reply" H3 -- right after the post title H1 😕

annezazu commented 2 years ago

(The copy is a bit verbose BTW -- my bad. Maybe @annezazu @carolinan have some suggestions?)

Hah it is! Perhaps this?

Comments block: You’re currently using the legacy version. What you see is a placeholder with the styling based on the current theme. For the latest updates, switch the block to its editable mode.

As for this broader issue, who else can we involve to perhaps move this work forward/consider more ideas? I'll tag in @gziolo for now in case he has any thoughts. While this isn't necessarily a blocker for the theme being accessibility ready, it feels important to get right.

ockham commented 2 years ago

(The copy is a bit verbose BTW -- my bad. Maybe @annezazu @carolinan have some suggestions?)

Hah it is! Perhaps this?

Comments block: You’re currently using the legacy version. What you see is a placeholder with the styling based on the current theme. For the latest updates, switch the block to its editable mode.

Much better 👍 Thanks a lot. I'll file a PR with this change (maybe with a slight tweak to the "latest updates" part, since as a user, I wouldn't find that the most compelling reason to click that button 😄).

As for this broader issue, who else can we involve to perhaps move this work forward/consider more ideas? I'll tag in @gziolo for now in case he has any thoughts. While this isn't necessarily a blocker for the theme being accessibility ready, it feels important to get right.

I was also thinking about this a bit more over the past week. I think I'd been caught up a bit too much with the feature parity question (getting the legacy block to make the "One response to..." heading an H2, as it already is for the editable version). While that's part of the puzzle, it doesn't solve the problem if there are no comments.

To solve this larger problem, maybe it's best to look how other themes have been addressing this? (Specifically Core's Twenties themes -- assuming that at least the more recent ones are a11y compliant.)

TT1 uses H2 for both "1 comment" and "Leave a comment":

image

Same for Twenty Twenty:

image

Looks like an elegant enough solution for me that also works if there are no comments 😄

So how about we follow this pattern for TT2?

annezazu commented 2 years ago

Adjust away! Thanks for being open to feedback on the message either way. I think this makes a ton of sense to follow the same pattern as other themes.

ockham commented 2 years ago

Okay, so it's possible to implement this suggestion by using my earlier solution and tweaking it a bit. The following is a patch against WordPress:

diff --git a/wp-content/themes/twentytwentytwo/comments.php b/wp-content/themes/twentytwentytwo/comments.php
new file mode 100644
index 0000000000..b11dd8b3fe
--- /dev/null
+++ b/wp-content/themes/twentytwentytwo/comments.php
@@ -0,0 +1,66 @@
+<?php
+// Do not delete these lines.
+if ( ! empty( $_SERVER['SCRIPT_FILENAME'] ) && 'comments.php' === basename( $_SERVER['SCRIPT_FILENAME'] ) ) {
+   die( 'Please do not load this page directly. Thanks!' );
+}
+
+if ( post_password_required() ) { ?>
+       <p class="nocomments"><?php _e( 'This post is password protected. Enter the password to view comments.' ); ?></p>
+   <?php
+   return;
+}
+?>
+
+<!-- You can start editing here. -->
+
+<?php if ( have_comments() ) : ?>
+   <h2 id="comments">
+       <?php
+       if ( 1 == get_comments_number() ) {
+           printf(
+               /* translators: %s: Post title. */
+               __( 'One response to %s' ),
+               '&#8220;' . get_the_title() . '&#8221;'
+           );
+       } else {
+           printf(
+               /* translators: 1: Number of comments, 2: Post title. */
+               _n( '%1$s response to %2$s', '%1$s responses to %2$s', get_comments_number() ),
+               number_format_i18n( get_comments_number() ),
+               '&#8220;' . get_the_title() . '&#8221;'
+           );
+       }
+       ?>
+   </h2>
+
+   <div class="navigation">
+       <div class="alignleft"><?php previous_comments_link(); ?></div>
+       <div class="alignright"><?php next_comments_link(); ?></div>
+   </div>
+
+   <ol class="commentlist">
+   <?php wp_list_comments(); ?>
+   </ol>
+
+   <div class="navigation">
+       <div class="alignleft"><?php previous_comments_link(); ?></div>
+       <div class="alignright"><?php next_comments_link(); ?></div>
+   </div>
+<?php else : // This is displayed if there are no comments so far. ?>
+
+   <?php if ( comments_open() ) : ?>
+       <!-- If comments are open, but there are no comments. -->
+
+   <?php else : // Comments are closed. ?>
+       <!-- If comments are closed. -->
+       <p class="nocomments"><?php _e( 'Comments are closed.' ); ?></p>
+
+   <?php endif; ?>
+<?php endif; ?>
+
+<?php comment_form(
+   array(
+       'title_reply_before' => '<h2 id="reply-title" class="comment-reply-title">',
+       'title_reply_after'  => '</h2>'
+   )
+); ?>

I'll file a PR against wordpress-develop.

joedolson commented 2 years ago

Link it against #55172 and I can review & commit it.

carolinan commented 2 years ago

I would like for other options to be explored, only because I am concerned that this will set a precedence for including a .php file which should not be needed in a block theme, in a default theme. The default themes are often used as examples.

carolinan commented 2 years ago

Untested: By moving the comments area to a pattern, the blocks can be included conditionally depending on WordPress version. The caveat is that it would only help installations where the single and page templates have not already been altered (markup saved to the database), and new installations.

ockham commented 2 years ago

Untested: By moving the comments area to a pattern, the blocks can be included conditionally depending on WordPress version.

Ah, I wasn't aware of that technique -- thank you @carolinan!

The caveat is that it would only help installations where the single and page templates have not already been altered (markup saved to the database), and new installations.

Still a net win I guess 😄

annezazu commented 2 years ago

Thanks for your guidance here @carolinan <3

ockham commented 2 years ago

I've given this some more thought (see https://github.com/WordPress/wordpress-develop/pull/3136), but I don't think we can entirely avoid customizing the comments template via PHP. I've thus now filed https://github.com/WordPress/wordpress-develop/pull/3150. Curious to hear y'all's thoughts!

carolinan commented 2 years ago

I'd still prefer a new heading block. The text could just be "Comments". Even if that heading is hidden visually with a screen reader text.

ockham commented 2 years ago

I'd still prefer a new heading block. The text could just be "Comments". Even if that heading is hidden visually with a screen reader text.

Fair enough 😄 But I'm afraid I'll have to put this on the backburner myself, since I need to focus on backporting code into Core, as the Feature Freeze is approaching fast 😅 Overall, all the attempted solutions to this issue had some rather complex implications (owed mostly to the different Comments block variants), so I'm afraid that it'll still take a while to get it right -- even with the extra heading block...

carolinan commented 2 years ago

I think that is the right priority :) I wonder if this issue should be closed and discussions can continue in the trac issue and PR's for Twenty Twenty-Two?

annezazu commented 2 years ago

Great point. I'll close this out. Thank you all for discussing this!