search

WordPress / twentytwentyfive

131 stars 95 forks source link

Complete the accessibility-ready review #408

Open carolinan opened 3 weeks ago

carolinan commented 3 weeks ago

Description

Default/bundled themes must pass the requirement for the accessibility-ready tag, which is a tag that is added to style.css and helps identify themes that have been tested for common accessibility problems. More information and list of requirements.

troychaplin commented 2 weeks ago

There are 3 contrast errors in the Coming Soon pattern, they all related to the cover block not having a background image and being reliant on the right opacity to be set. This impacts the Coming Soon pattern as it uses yellow text by default. Using a dark image makes no difference as the contrast ratio is checked between the text and the background colour, the image has zero impact on this.

I have created a ticket for this issue in the Gutenberg repo -- https://github.com/WordPress/gutenberg/issues/65868

Pattern test with HTML_CodeSniffer

Image

Pattern test with Wave Extension

Image

Test after adding inline background colour style

Image

carolinan commented 2 weeks ago

The automated tools can't check the contrast when there is a background image used, and a human can't really check it either because there are many different colors in the photo.

troychaplin commented 2 weeks ago

The automated tools can't check the contrast when there is a background image used, and a human can't really check it either because there are many different colors in the photo.

IMO an example like this has two-parts too it. Scanners will ignore the image and compare against the next available background colour, easily remedied in code with a bg colour on cover. The other goes beyond code and puts the responsibility in the hands of the content creator to consider the visuals impacts to some folks.

carolinan commented 1 week ago

Content links: Links in the post, page and comment content are underlined.

I have a reservation about links in close proximity to the content, such as dates, author names, and categories not being underlined. Without an underline, they can only be discovered by focusing on or hovering over the item.

carolinan commented 1 week ago

ARIA landmark roles: In "News blog with featured posts grid", there is a pattern between the <main> and the <footer> that is outside a landmark (I thought I fixed this once).

In "Right-aligned single post" the comments area between the <main> and the <footer> is outside a landmark. Lets use the asidefor this template, to not make any changes to the existing design.

troychaplin commented 1 week ago

Heading error:

Fixed in PR - https://github.com/WordPress/twentytwentyfive/pull/510

troychaplin commented 1 week ago

I reviewed all patterns by inserted each of them one and a time and noted the following. @carolinan I am happy to fix anything here in a PR, just wanted to confirm that everything I've noted is a change that is needed.

Should the following patterns have a heading?

Items that (maybe) should not display in patterns:

carolinan commented 1 week ago

The headers with the site title should not use H1. The H1 should be the page title, describing the page content.

I think hiding the query loop patterns, the list of posts and the photo blog posts, are handled in https://github.com/WordPress/twentytwentyfive/issues/411 But it has gone quiet.

carolinan commented 1 week ago

@beafialho @juanfra in the list in the comment above, there are some questions about some patterns that visually look like they should use headings, but don't use headings. Should they be updated?

For example, the "Hey," in the CV pattern is very large, but is a paragraph.

beafialho commented 1 week ago

@beafialho @juanfra in the list in the comment above, there are some questions about some patterns that visually look like they should use headings, but don't use headings. Should they be updated?

Sure, as long as the sizes look the same as they do now.

juanfra commented 1 week ago

I second Bea. If they should have headings let's make them use headings as long as the sizes look the same as they do now.

troychaplin commented 1 week ago

Thanks @beafialho and @juanfra! I will create a PR for these later today @carolinan, skipping over the notes I made about site titles and level 0. Also noticed @juanfra PR'd in some changes relating to patterns

juanfra commented 1 week ago

Thank you, @troychaplin - Please keep us updated if you're blocked or need anything else. We're on our way to beta 3 coming Monday 🚀

troychaplin commented 1 week ago

Thank you, @troychaplin - Please keep us updated if you're blocked or need anything else. We're on our way to beta 3 coming Monday 🚀

Thanks, I'll be on this tonight, I cleared my evening to work on this. Shouldn't take long.

troychaplin commented 1 week ago

@juanfra @carolinan I have a question, I just noticed as I was updating the heading the Cover with big heading pattern that the content in the file is not wrapped in esc_html_x like we have in the CV/bio. Which is the preferred method, I will review them all again and make them consistent.

Assuming with translation, but wanna make sure before I get started

carolinan commented 1 week ago

The text strings should all be escaped and translatable, but _x is only needed if additional context needs to be provided. If it is not clear that Stories is intended to be an example brand name, yes, please add the translators context.

Ideally, translators would use the same translation for this word, so maybe they need extra help. In Swedish for example there are several synonyms.

troychaplin commented 1 week ago

@carolinan After a better review there was only 3 patterns to update and only one was missing translation so I used _x and PR'd the changes. I can change that for _e if you'd like. I reviewed again and it should be _e based on other patterns, changes have been pushed.

Some other notes I made for clarification and I can make another PR if necessary: