Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 10 months ago

#55172 closed task (blessed) (fixed)

Twenty Twenty-Two: Add "Accessibility Ready" tag to theme in repository

Reported by: bph's profile bph Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch add-to-field-guide has-testing-info
Focuses: accessibility Cc:

Description (last modified by sabernhardt)

Context:

On this make blog post, @piyopiyofox posted the following question:

"It’s been flagged that none of the FSE themes in the theme directory are Accessibility Ready. Will there be any improvements to TT2 to make it Accessibility Ready?"

@Kjellr answered:

I’m pretty sure Twenty Twenty-Two is just missing the tag actually. The theme was reviewed throughout its development, and found to pass WCAG AA.

The theme review guidelines go beyond WCAG in some areas, and there was one conflict regarding the use of multiple H1s. But there was consensus that the rule would be updated in the guidelines side

Change History (84)

#1 @sabernhardt
3 years ago

  • Component changed from Themes to Bundled Theme
  • Description modified (diff)
  • Focuses accessibility added
  • Summary changed from Add "Accessibility Ready" tag to Twenty-Twenty-Two theme in repository to Twenty Twenty-Two: Add "Accessibility Ready" tag to theme in repository

This ticket was mentioned in PR #2324 on WordPress/wordpress-develop by kjellr.


3 years ago
#2

  • Keywords has-patch added

Adds the accessibility-ready tag to Twenty Twenty-Two.

Trac ticket: https://core.trac.wordpress.org/ticket/55172

#3 @kjellr
3 years ago

I've opened a PR for this, but it would be good to get confirmation from someone on the Theme Review before merge.

cc @pattonwebz or @utz119 for a second look, since you were both involved in the original thread. Thank you!

Version 0, edited 3 years ago by kjellr (next)

#4 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9.1

#5 @utz119
3 years ago

I’m waiting for a confirmation from the accessibility team if we can remove the multiple H1s requirement from the WordPress accessibility guidelines. I’m cc @joedolson for the clarification. I think that’s the major hurdle for creating an accessibility-ready block theme currently.

#6 @hellofromTonya
3 years ago

  • Milestone changed from 5.9.1 to 5.9.2

5.9.1 is happening about 30 minutes. As this ticket is still pending review, moving to 5.9.2.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#8 @ryokuhi
3 years ago

We reviewed this ticket today during the accessibility team's bug-scrub.
While we know that accessibility has been taken into account during the theme design and development, we think it would be better to have an extra review to be sure that the theme complies with the requirements for accessibility-ready themes.
As long as the theme doesn't show any issue, the accessibility-ready tag can then be added.

#9 @utz119
3 years ago

I reviewed the Twenty Twenty-Two and found several accessibility issues.

  • #55198 - Header skips levels.
  • #55197 - “No Button” search option.
  • #55196 - Search SVG icon is missing machine-readable text.

For the multiple H1, I think we need to decide whether to revise or keep the requirement.

Last edited 3 years ago by utz119 (previous) (diff)

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#11 @joedolson
3 years ago

We discussed this ticket in the accessibility bug scrub, and we're agreed that the nature of full site editing and the way it depends on healthy block output means that FSE themes shouldn't be able to use the accessibility-ready tag until the required block markup also meets those guidelines.

If there are fundamental accessibility issues in commonly required blocks such as search, menus, archive lists, etc., then those accessibility issues are nearly if not actually guaranteed to appear in the resulting sites. It would be irresponsible of us to give the impression that an FSE site could meet any accessibility requirements if those core blocks won't.

#12 @kjellr
3 years ago

Thanks for the review @utz119, and for the followup @joedolson.

If there are shared issues beyond the Search issues noted above, would it be possible for the a11y team to help catalog them so they can be prioritized by the Editor development folks? It would be helpful to build up a complete list so we know when this can be closed out. What would be the best way to make that happen?

#13 @joedolson
3 years ago

Thanks, @kjellr - I'll go straight to what would be the ideal from my perspective. One of the problems is just being able to consistently catalog all of the core blocks. The most useful thing for me, and I think for the accessibility team as a whole, would be a publicly accessible index page containing a single instance of all core blocks in their default state. For testing purposes, they should have a basic level of content (e.g., images should have alt text included, galleries should have captions, menus should have items in multiple levels, etc.)

Something like gutenberg.run, but focused on the front-end rather than the back-end, so that we can more easily test the output of blocks. Right now, the labor involved in setting up a site, building Gutenberg, finding the right block, etc., is a major pain point when all you really want to do is verify that the output of a block is OK.

If this was also tied to e2e testing, that would be valuable; even though there are a lot of accessibility issues that can't be tested with automation, being able to quickly catch the issues that are testable would be extremely valuable. Something like an SVG in a button with no accessible name should never make it past automated tests.

If there's a way to easily do that already, please let us know.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

This ticket was mentioned in Slack in #themereview by utz119. View the logs.


3 years ago

#16 @audrasjb
3 years ago

  • Milestone changed from 5.9.2 to 5.9.3

Moving to milestone 5.9.3 since we're about to release 5.9.2.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#18 @audrasjb
3 years ago

  • Type changed from enhancement to task (blessed)

#19 @audrasjb
3 years ago

I converted this to a task, since the Gutenberg team need to collaborate with the Theme and Accessibility teams to help moving this forward. Hopefully it can move toward a resolution in the next weeks :)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#21 @audrasjb
3 years ago

  • Milestone changed from 5.9.3 to 5.9.4

Moving to the next point release.

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

#23 @kirasong
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from 5.9.4 to 6.1

We chatted about this in a bug scrub for 6.0 today.

It's not clear to me what the priority level is for the teams working on this, but it seems like resolution depends on at least one ticket that wouldn't usually land during RC.

Because RC is in a few days, I'm going to move this to 6.1.
As this is a task, if the priority is high / work is planned to continue during RC, please feel free to move it back.

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

#25 @kirasong
3 years ago

Related: https://github.com/WordPress/gutenberg/issues/38924 (this was created as an upstream issue for #55197, above).

@mamaduka and I chatted about this a bit (here), since it was in the 5.9.4 milestone, rather than 6.0 when it was moved out, and I wanted to be sure this was an appropriate shift.

He mentioned to leave it at 6.1 for now due to it being unsure as to whether the enhancement in Gutenberg required would land during a minor release or not, and that he'd take a look at the issue above.

So, I'm going to leave the milestone as is for now, but please feel free to move it back in if it's decided differently!

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#27 @joedolson
2 years ago

Would like to get input from @poena on her opinions concerning the viability of accessibility-readiness for block-based themes and what that really means.

#28 @poena
2 years ago

I have tested the theme against these requirements:
https://make.wordpress.org/themes/handbook/review/accessibility/required/

Theme version: 1.2
WordPress version: 6.0.1
macOs
Chrome
No plugins
Theme test data

Summary

Passes all tests except a decorative image that is missing aria-hidden.
This issue will be solved in the query pagination block in WordPress 6.1.

Recommended:
Headings on single posts and pages could be improved.
The search page is missing a visible label for the search form, so this could be improved.

Recommended actions:
Test the theme again in 6.1 beta after the query pagination block has been updated, then decide.


Keyboard Navigation

Theme authors must provide visual keyboard focus highlighting
in navigation menus and for form fields submit buttons and text links.
-Passed

All controls and links must be reachable using the keyboard.
-Passed

Controls

All theme features that behave as buttons or links must use <button>, <input>, or <a> elements, to ensure native keyboard accessibility and interaction with screen reader accessibility APIs.
-Passed

All controls must also have machine-readable text to indicate the nature of the control.
-Passed

Note: The icon only buttons for the navigation block,
and icon only links for social icons have accessible names.

-Passed

Note: The theme has one template that does not include the <main> element,
and because of that it does not have a skip link by default.
But this is the "blank" template that only shows the post content block.
Which means that by default, it does not have anything to skip past before the content is reached. Advanced Users can add the main landmark by changing the HTML tag of blocks in the block settings sidebar.

Forms

Forms must have appropriate field labels.
All content within form tags need to be explicitly associated to a form control.
-Passed

Note: The default search template uses a search block with the form label hidden.
Displaying the label would be an improvement, but it passes the requirements.

Headings

Required: Passed
Recommended: Did not pass

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

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.

ARIA Landmark Roles

Assign landmark roles to the main content areas of your site.
All content on your site must be wrapped in at least one landmark role.
-Passed

Header, main, footer are used appropriately.
The exception is the optional blank template.
By default, the theme only has one nav element on each page (the main navigation block in the header).
Notably, on the homepage the theme has a header element inside another header element.

Links within large sections of text (e.g., post content or comments) must be underlined.
-Passed

Repetitive Link Text

-Not used, passed

Contrasts

-Passed

Images

-Did not pass, but will pass in WordPress 6.1
Query navigation uses arrows that supplement text, but do not currently
use aria-hidden. Patches for this have been submitted for the block itself.

Media

-Not found, passed

Screen Reader Text

-Passed
Note: Provided by WordPress core, not the theme.

Not Allowed

-Passed
No links in the theme are set to open in new windows by default.

This ticket was mentioned in Slack in #accessibility by poena. View the logs.


2 years ago

#30 @annezazu
2 years ago

For the note on headings above, I've opened this issue in the Gutenberg repo to continue the conversation: https://github.com/WordPress/gutenberg/issues/43203

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

This ticket was mentioned in PR #3136 on WordPress/wordpress-develop by ockham.


2 years ago
#32

Work in progress**

Based on @carolinan's suggestion here, this PR attempts to provide a "Comments" block pattern for use with TT2 that dynamically renders the _Comments_ block for WP >= 6.1, and falls back to the legacy _Post Comments_ block for WP < 6.1.

The motivation for this is a11y, as discussed in 43203 (and previously https://core.trac.wordpress.org/ticket/55172).

Trac ticket: https://core.trac.wordpress.org/ticket/55172

carolinan commented on PR #3136:


2 years ago
#33

Next, the pattern need to replace the block in the HTML templates.

ockham commented on PR #3136:


2 years ago
#34

Next, the pattern need to replace the block in the HTML templates.

I've been trying inserting the pattern manually, and I'm getting the "Post Comments" block (rather than "Comments"), even with GB enabled. I've also tried changing the criterion to look for core/comments-query-loop -- which is present in WP trunk -- with the same result.

So it seems like our check isn't returning the desired (and required result). I'll poke a bit more to find out what's up.

ockham commented on PR #3136:


2 years ago
#35

I looked a bit more into it; I've now also tried if ( function_exists( 'render_block_core_comments' ) ) { -- which also doesn't work (as it evaluates to false). (function_exists( 'render_block_core_comments' ) OTOH evaluates to true, as the Post Comments block is already in Core.)

I think the reason is that the updated Comments block (and thus, render_block_core_comments) is only present in Gutenberg (but hasn't been backported to Core yet). The TT2 theme is likely being loaded before plugins (including GB).

I'll continue to look into this tomorrow. A few possible solutions off the top of my head:

  1. Backport the Comments block into Core. We need to do this anyway, and it should solve the problem -- with a caveat: If the theme is used in an older version of WordPress (with no Comments block) that has the GB plugin installed (with the Comments block), it will not work (and continue to use the Post Comments block).
  2. Check for function_exists( 'render_block_core_comments' ), and render the Post Comments block in that case; otherwise, render the Comments block. I think this should work; the Post Comments block has been around longer than TT2, so any WP install without that block should be more recent and thus have the Comments block. (Same drawback as number 1. with regard to old WPs with GB plugin, though.)
  3. Check for the WordPress version (rather than presence of a function). (Same drawback as the other ones.)

This ticket was mentioned in Slack in #accessibility by chaion07. View the logs.


2 years ago

ockham commented on PR #3136:


2 years ago
#37

Just for completeness, even once the issue with check for the existence of the Comments block is fixed, we need to do a bit more work:

The Comments block has H2 for the "One reply to..." heading (via the Comments Title block -- which can actually be customized to different header sizes, but defaults to H2).

https://i0.wp.com/user-images.githubusercontent.com/96308/187201061-11f25fce-950b-4375-8c39-5a90aa62c6d2.png

However, it uses the Post Comments Form block for rendering the comments form, which is a thin wrapper around `comment_form()` (docs). The latter defaults to H3 for the "Leave a reply" heading.

While that _can_ be customized...

{{{php
comment_form(

array(

'title_reply_before' => '<h2 id="reply-title" class="comment-reply-title">',
'title_reply_after' => '</h2>'

)

);
}}}

...I'm not sure how to best integrate this fairly fine-grained level customization into the _Comments_ -> _Post Comments Form_ block hierarchy:

Are we okay with just hard-wiring it as H2 (even though it used to be H3)? (This could affect existing themes.) Do we need to expose it in the UI? The latter might be the best option, but given that we're still using the "monolithic" Post Comments Form block -- which currently doesn't really have any customization options -- it might be a bit weird to expose only this one customization option. It might also look counter-intuitive to have a heading customization button in the block toolbar of a block that contains more than just a heading.

https://i0.wp.com/user-images.githubusercontent.com/96308/187207311-42ae9797-1034-4d0a-b00c-2b42106e2077.png

These changes would need to be done in Gutenberg. I'll add a TODO item to the PR description, and I guess I'll file a separate issue in the GB repo to solicit design feedback.

ockham commented on PR #3136:


2 years ago
#38

And finally, for the case where we the Comments block is unavailable and we have to use the Post Comments block, we need to make sure that _that_ block renders an H2 for both "One reply to..." and "Leave a reply". This basically means applying this patch after all -- I think that's pretty much unavoidable; the block pattern technique that we're applying in _this_ PR only helps if the Comments block _is_ available. (Unless I'm missing something -- cc/ @carolinan)

I'll add that also to the PR desc PR and will file a separate PR for it, as it can arguably be decoupled from what we're doing here.

This ticket was mentioned in PR #3150 on WordPress/wordpress-develop by ockham.


2 years ago
#39

Add a src/wp-content/themes/twentytwentytwo/comments.php file that will be picked up by the Post Comments block to render, well, a post's comments. The file is copied from Core's `theme-compat/comments.php`, with some modifications to change the H3 headings ("One reply to..." and "Leave a reply") to H2, in order to address this a11y issue.

Note that this issue has been discussed in both https://github.com/WordPress/gutenberg/issues/43203 and https://github.com/WordPress/wordpress-develop/pull/3136. I currently believe that this PR is both necessary and sufficient, so it could be deemed a complete solution. (As @carolina pointed out, it's unfortunate that we have to add a PHP file to customize a block theme, but I currently don't see any alternative.)

Trac ticket: https://core.trac.wordpress.org/ticket/55172

### Testing instructions

Using the TT2 theme, view a single post with comments on the frontend (e.g. the "Hello World" post that comes with a new WP install. Verify that both the "One reply to..." and "Leave a reply" headings are H2.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

This ticket was mentioned in Slack in #accessibility by costdev. View the logs.


2 years ago

ockham commented on PR #3136:


2 years ago
#42

BTW unrelated to all the other prerequisites, there _might_ be a slightly different way to implement the "block fallback":

Instead of having a block _pattern_ that renders either Comments or Post Comments, we could try something like including a fake tt2/comments _block_ with the theme (which does basically the same, i.e. render either Comments or Post Comments, depending on availability). (The difference to using a block pattern might be insignificant -- but maybe it's a bit more intuitive to use an intermediate block rather than a pattern here 🤔 )

carolinan commented on PR #3136:


2 years ago
#43

That would break the users website when they switch themes, since that block would no longer be available.

carolinan commented on PR #3136:


2 years ago
#44

I think it needs to check for the WordPress version, not (only) if the block exists.
If a site use WP 5.9 and has Gutenberg active, the template uses the new block. If the site owner then disables Gutenberg, the new block is no longer available.
The block would be missing on the front. There would be a message in the editor that the block is missing.

#45 @annezazu
2 years ago

After chatting with various folks and confirming there are no additional blockers to adding this label, I wanted to check in on this issue and whether the label is going to be added soon, especially since the block theme and accessibility convo happened on Sept 2nd with folks showing support https://wordpress.slack.com/archives/C02RP4X03/p1662137459194589.

What are the next steps here? Are any resources needed to get the label added?

#46 @poena
2 years ago

Still undetermined. @alexstine recommended having a testing process for the front first.

Link to conversation on WordPress Slack, account needed

(I think it is most important that there are no known issues except the heading order, than adding the tag itself.)

#47 @joedolson
2 years ago

The vote was five in favor of allowing accessibility-ready on block-based themes and one against; while I greatly respect Alex's opinion, I don't think his concerns about the back-end should block this.

We should keep in mind that a breakage to the accessibility of WordPress block output will impact all block themes, so those failures are quite serious, but as it stands now the theme meets all expected criteria.

#48 @audrasjb
2 years ago

  • Keywords commit added; dev-feedback removed
  • Owner set to audrasjb
  • Status changed from new to accepted

Alright! Then we can go with PR 2324. Thanks everyone!

We should keep in mind that a breakage to the accessibility of WordPress block output will impact all block themes, so those failures are quite serious, but as it stands now the theme meets all expected criteria.

This is indeed the main flaw here.

#49 @audrasjb
2 years ago

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

In 54168:

Twenty Twenty-Two: Add "Accessibility Ready" tag to theme in repository

A cross-team consensus has been reached to allow Twenty Twenty-Two to be tagged as "Accessibility Ready" on the WordPress Themes repository.

Props bph, kjellr, utz119, ryokuhi, joedolson, audrasjb, mikeschroder, poena, annezazu.
Fixes #55172.

MaggieCabrera commented on PR #3136:


2 years ago
#50

I think it needs to check for the WordPress version, not (only) if the block exists. If a site use WP 5.9 and has Gutenberg active, the template uses the new block. If the site owner then disables Gutenberg, the new block is no longer available. The block would be missing on the front. There would be a message in the editor that the block is missing.

I tried to reproduce this with 5.9 and installing/uninstalling the plugin and I don't see what you describe. With or without the plugin the comments show correctly with either version of the block both in the frontend and the editor.

MaggieCabrera commented on PR #3136:


2 years ago
#51

@carolinan what do you think this PR needs doing at the moment? I've tested on 6.0 and 5.9 with and without the plugin and the comments show as expected using the correct block for the case.

ockham commented on PR #3136:


2 years ago
#52

@carolinan what do you think this PR needs doing at the moment?

I might be able to answer part of that; the original motivation for this PR was accessibility (see https://github.com/WordPress/gutenberg/issues/43203). To fully address that, we might need some additional changes (e.g. https://github.com/WordPress/wordpress-develop/pull/3136#discussion_r963257776).

However, in order to improve UX (especially to avoid the -- arguably confusing -- "Please update your block to its editable mode" message), we might consider landing this PR as-is, and address a11y separately.

I've tested on 6.0 and 5.9 with and without the plugin and the comments show as expected using the correct block for the case.

Thank you for testing @MaggieCabrera! This is the part I never seemed to have gotten to work properly. I'll retest myself now.

ockham commented on PR #3136:


2 years ago
#55

I think it needs to check for the WordPress version, not (only) if the block exists. If a site use WP 5.9 and has Gutenberg active, the template uses the new block. If the site owner then disables Gutenberg, the new block is no longer available. The block would be missing on the front. There would be a message in the editor that the block is missing.

Yeah, that makes sense 🤔 😕 This means we'll be limiting use of the Comments block in TT2 a bit more (i.e. even when it's present because GB is installed), but it's probably worth avoiding the kind of situation that you described...

I tried to reproduce this with 5.9 and installing/uninstalling the plugin and I don't see what you describe. With or without the plugin the comments show correctly with either version of the block both in the frontend and the editor.

That's curious 🤔 What I could imagine is that if the template is unmodified, it will retain the pattern and thus the check. If it evaluates it on the frontend, it should render the correct block. However, the moment we modify the template that includes the comments pattern, I'd expect the situation @carolinan was describing if we then disable the Gutenberg plugin on an older WP install...

MaggieCabrera commented on PR #3136:


2 years ago
#56

I tried to reproduce this with 5.9 and installing/uninstalling the plugin and I don't see what you describe. With or without the plugin the comments show correctly with either version of the block both in the frontend and the editor.

That's curious 🤔 What I could imagine is that if the template is unmodified, it will retain the pattern and thus the check. If it evaluates it on the frontend, it should render the correct block. However, the moment we modify the template that includes the comments pattern, I'd expect the situation @carolinan was describing if we then disable the Gutenberg plugin on an older WP install...

Yeah, in the same way that adding any kind of other block and then uninstalling the plugin that adds that kind of block would render it invalid. Should we account for that kind of usage? Resetting the template would bring the old block back again in such a case.

ockham commented on PR #3136:


2 years ago
#57

Yeah, in the same way that adding any kind of other block and then uninstalling the plugin that adds that kind of block would render it invalid. Should we account for that kind of usage? Resetting the template would bring the old block back again in such a case.

Good point. I'm not sure TBH -- I can't seem to think of any established precedents here.

We could consider starting with the WP version check since it's "tighter" (and avoids that situation altogether, while still providing better UX for anyone on WP >= 6.1), and loosening it later.

@MaggieCabrera Would you have time to take it from here? I'm still kinda busy with backports and such 😅

MaggieCabrera commented on PR #3136:


2 years ago
#58

Furthermore, we might want to add styling for core/comments, like we have for core/post-comments:

https://github.com/WordPress/wordpress-develop/blob/60e13f468c7eea919715e9e8d101200c177e01ed/src/wp-content/themes/twentytwentytwo/theme.json#L246-L252

Mmmh, I don't think we need to change anything. TT2 launched with the old block and no CSS related to it. When implementing the new block we are trying to imitate the style of the old block instead, so they should look as close as possible (I'm saying this without checking this change specifically).

MaggieCabrera commented on PR #3136:


2 years ago
#59

@MaggieCabrera Would you have time to take it from here? I'm still kinda busy with backports and such 😅

Yeah, I can take it from here. Also, @carolinan might be able to confirm but I don't think theme changes need to be restricted to the freeze date the same way Gutenberg features need to be, being themes and all, not core changes.

MaggieCabrera commented on PR #3136:


2 years ago
#60

We have a few more occurrences of the Post Comments block that we'll need to replace in TT2:

Note that two of those have block attributes. Not sure if that will work with our block-pattern approach 😕

I pushed a change to those templates, adding the attributes to the group block wrapping them.

#61 @milana_cap
2 years ago

  • Keywords add-to-field-guide added

carolinan commented on PR #3136:


2 years ago
#62

I have not been working so I have not been able to stay up to date with the changes.

Requested change:
When the spacing is moved from the comments block to the wrapping group block, the spacing also need to be inline on the group, in a style attribute.

ockham commented on PR #3136:


2 years ago
#63

Thanks a lot for handling this, @MaggieCabrera and @carolinan!

I'm not sure what the outcome was with regard to whether this change can go in after Core Feature Freeze:

Also, @carolinan might be able to confirm but I don't think theme changes need to be restricted to the freeze date the same way Gutenberg features need to be, being themes and all, not core changes.

FWIW, if you can give it approval, we can try to get a Core committer to merge it for us prior to Feature Freeze.

Since the accessibility team decided that this single issue was not a blocker for adding the accessbility-ready tag,

We could remove the heading and only change the comments block with the patterns.

My 2 ct: I think that's preferable, since the heading workaround did seem to have a few unwanted side-effects that we hadn't fully figured out yet. (I'll yield to y'all's decision however since I'm no longer actively working on this PR.)

MaggieCabrera commented on PR #3136:


2 years ago
#64

Since the accessibility team decided that this single issue was not a blocker for adding the accessbility-ready tag,
We could remove the heading and only change the comments block with the patterns.

My 2 ct: I think that's preferable, since the heading workaround did seem to have a few unwanted side-effects that we hadn't fully figured out yet. (I'll yield to y'all's decision however since I'm no longer actively working on this PR.)

I'd prefer to remove the heading and leave the rest of this PR as it is, if possible.

I think I covered the rest of our concerns too, so pending that decision, the rest of this PR should be ready for review.

MaggieCabrera commented on PR #3136:


2 years ago
#65

I did the following:

  • Hid the pattern from the inserter in the same way we do for other utility patterns like the 404
  • Renamed it to hidden-comments.php to maintain that same convention
  • Filtered the post-comments block and changed the h3 to h2 on render block

Does that work? Or is there a better way to achieve this?

MaggieCabrera commented on PR #3136:


2 years ago
#66

A recap of what this PR ended up doing:

The Post Comments block uses an h3 heading on it, this causes a11y issues on the templates where it's inserted, since there are no existing h2 in them. The new Comments block doesn't have the same problem because it allows you to change the heading as you need, but it's only available with Gutenberg or WP >= 6.1. To solve this issue, we are doing the following:

  • We are creating a hidden block pattern that inserts either of the blocks depending on if the new block is registered and the WP version is >= 6.1
  • For users that will get the deprecated version of the block, we are using the render_block hook to swap the h3 with an h2, and solve the a11y for them
  • Users that will get the newer version of the block, will have the h2 instead.

To test this:

  • Run this branch, check a page with comments on it, you should be seeing the headings using h2. On the editor, you should be seeing the new Comments block (there is no deprecation message present). You may need to tweak this line in hidden-comments.php, since this branch is still not a 6.1 version of WP:

if ( WP_Block_Type_Registry::get_instance()->is_registered( 'core/comments' ) && version_compare( $GLOBALS['wp_version'], '6.1', '>=' ) ) {

  • Run the code from this branch on a 6.0 environment, you should be seeing the old block, with the deprecation notice in the editor. In the frontend, you should still see the headings on the comments block using h2, instead of h3.

This ticket was mentioned in Slack in #core by joedolson. View the logs.


2 years ago

#68 @annezazu
2 years ago

Recap of conversation in Core Dev Meeting on Sept 21st 2022

Ahead of 6.1, there was a discussion around whether a version of Twenty Twenty-Two could be released in order for the accessibility label to be added ahead of WordPress 6.1's release. The reason for this conversation is because this would be the first block theme with the accessibility label added, opening up more folks to the world of block themes and broader full site editing features. During this discussion, it was shared that one of the accessibility bugs necessary for the theme to be accessibility ready will be fixed in 6.1 and releasing early would cause that fix to not be available, making the theme not truly accessibility ready: https://core.trac.wordpress.org/ticket/56067 As a result, Twenty Twenty-Two will update alongside 6.1.

Link to conversation: https://wordpress.slack.com/archives/C02RQBWTW/p1663791336069329 - you need a Making WordPress Slack account to view.

Last edited 2 years ago by annezazu (previous) (diff)

ockham commented on PR #3136:


2 years ago
#69

I did the following:

  • Hid the pattern from the inserter in the same way we do for other utility patterns like the 404
  • Renamed it to hidden-comments.php to maintain that same convention
  • Removed the h2 from the pattern
  • Filtered the post-comments block and changed the h3 to h2 on render block

Does that work? Or is there a better way to achieve this?

Sounds pretty good to me!

ockham commented on PR #3136:


2 years ago
#70

A recap of what this PR ended up doing:

The Post Comments block uses an h3 heading on it, this causes a11y issues on the templates where it's inserted, since there are no existing h2 in them. The new Comments block doesn't have the same problem because it allows you to change the heading as you need, but it's only available with Gutenberg or WP >= 6.1. To solve this issue, we are doing the following:

  • We are creating a hidden block pattern that inserts either of the blocks depending on if the new block is registered and the WP version is >= 6.1
  • For users that will get the deprecated version of the block, we are using the render_block hook to swap the h3 with an h2, and solve the a11y for them
  • Users that will get the newer version of the block, will have the h2 instead.

To test this:

  • Run this branch, check a page with comments on it, you should be seeing the headings using h2. On the editor, you should be seeing the new Comments block (there is no deprecation message present). You may need to tweak this line in hidden-comments.php, since this branch is still not a 6.1 version of WP:

if ( WP_Block_Type_Registry::get_instance()->is_registered( 'core/comments' ) && version_compare( $GLOBALS['wp_version'], '6.1', '>=' ) ) {

  • Run the code from this branch on a 6.0 environment, you should be seeing the old block, with the deprecation notice in the editor. In the frontend, you should still see the headings on the comments block using h2, instead of h3.

Thank you! I'll use this to update the PR description.

carolinan commented on PR #3136:


2 years ago
#71

Thank you @MaggieCabrera !

ockham commented on PR #3136:


2 years ago
#72

@carolinan If you get a chance, would you mind giving this a review? I talked to a Core committer and they seemed to agree that this might qualify as an a11y bugfix, so we might still be able to get this into WP 6.1 😊

#73 @poena
2 years ago

I have tested the PR with the theme update in WordPress 5.9.4, 6.0.2, and 6.1-beta1-54304.
(Gutenberg was not active on the WordPress installations).
The heading order is correct for all scenarios.

#74 @SergeyBiryukov
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening, as PR 3136 was brought up for review and needs to be addressed.

Or perhaps a new ticket should be created for that?

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


2 years ago

#76 @sabernhardt
2 years ago

  • Keywords commit removed

#77 @costdev
2 years ago

  • Keywords has-testing-info needs-testing added

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce or Test

  1. Navigate to Appearance > Themes.
  2. Activate Twenty Twenty-Two.
  3. View a single post with comments on the frontend (e.g. the "Hello World" post that comes with a new WP install)
  4. 🐞 Inspect the "One reply to..." and "Leave a reply" heading

Expected Results

When reproducing a bug:

  • ❌ The headings should be h3.

When testing a patch to validate it works as expected:

  • ✅ The headings should be h2.

#78 @joedolson
2 years ago

@SergeyBiryukov I think that PR 3136 should be a separate ticket; while it's an accessibility improvement, it's not a blocker to having the accessibility-ready tag.

#79 @desrosj
2 years ago

  • Keywords needs-testing removed
  • Resolution set to fixed
  • Status changed from reopened to closed

I've opened #56798 to address PR 3136.

@Bernhard Reiter commented on PR #3136:


22 months ago
#80

Pinging WP 6.2 Editor Tech co-leads @mamaduka and @ntsekouras. This is a PR that I hoped to get into WP 6.1 but that didn't make the cut at the time. Flagging for y'all in case you would like to get it into WP 6.2 😊

@Mamaduka commented on PR #3136:


22 months ago
#81

Thank you, @audrasjb! That would be great!

@poena commented on PR #3136:


20 months ago
#82

Hi, are we considering this for 6.2.1 or 6.3?

@Bernhard Reiter commented on PR #3136:


20 months ago
#83

Thank you @audrasjb!

I can land this within the next couple of days so it'll go into 6.3. I don't feel strongly about whether or not it should go into 6.2.1 -- curious if folks things it qualifies as important enough for that 😊

annezazu commented on PR #3136:


20 months ago
#84

I imagine since this doesn't relate to 6.2, we can't include it in 6.2.1. Let's aim for 6.3.

Note: See TracTickets for help on using tickets.