Adding accessible markup to sale price.

[BFTrick]

Changes proposed in this Pull Request:

This PR adds accessible markup to the sale price.

A user who only uses screen reader software can now hear and understand the original price as well as the current (sale) price.

This is important for accessibility.

Here's a demo (🔉 on):

woocommerce-proposed-sale-price-accessibility-480.mov

Full issue: #31099

How to test the changes in this Pull Request:

1. Create a simple product in WC > Products with a sale price set.
2. Click its permalink on the admin or search for the product on the frontend and go to the product's page.
3. Using the devtools inspector choose the area around the product price (or look for the <p class="price"> tag).
4. Confirm that both the <del> and <ins> tags are present and have aria-hidden set. These tags should include the original and sale price, respectively (see screenshot below).
5. Confirm that there are a couple of <span> tags with class screen-reader-text which clarify with text what the original and current prices are (see screenshot below).

[github-actions]

Hi ,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[jorgeatorres]

Hi @BFTrick!

Thanks for your contribution. This is a much appreciated change :100.
Thinking of RTL languages, I believe ideally the translatable strings inside screen-reader-text should contain a placeholder for the actual value. For example:

if ( is_numeric( $regular_price ) ) {
	$price .= sprintf( esc_html__( 'Original price was: %s.', 'woocommerce' ), wc_price( $regular_price ) );
}

Otherwise, the text (once translated) along with the price might not make the most sense for certain languages.

Also wanted to note that the new approach isn't equivalent to the current one. When either $regular_price or $sale_price are not numeric we currently don't output anything between the <del> or <ins> tags, but with this new approach we will output, for example, "Current price is:" when that variable is empty.
Ideally we wouldn't add unnecessary output, so we might need to adjust the checks here.

Would it make sense to leave the <del> tag alone with aria-hidden=true and add an <span class="screen-reader-text"> with "Original price was: X" immediately after it and before the sale price? With that in mind, we might not need to clarify what the "current" price is for screen readers.

Please let me know what you think and, once again, thanks for your contribution!

[BFTrick]

Thanks for the thoughtful feedback @jorgeatorres 🙏

Thinking of RTL languages, I believe ideally the translatable strings inside screen-reader-text should contain a placeholder for the actual value

Agreed. Much better solution!

✅ Updated code with this suggestion

When either $regular_price or $sale_price are not numeric we currently don't output anything between the <del> or <ins> tags

Derp! Definitely should avoid printing empty tags. 👍

✅ I updated my code so I do my checks and format the price at the top of the function then use that value lower down.

Would it make sense to leave the <del> tag alone with aria-hidden=true and add an <span class="screen-reader-text"> with "Original price was: X" immediately after it and before the sale price?

From an accessibility point of view either will work. As long as the information is conveyed it's accessible. 👍

With localization in mind: $price .= sprintf( esc_html__( 'Original price was: %s.', 'woocommerce' ), $formatted_regular_price ); it's easier & more straightforward to format with a <del> tag that's hidden and a separate <span class="screen-reader-text"> immediately after.

✅ I updated my code with this.

With that in mind, we might not need to clarify what the "current" price is for screen readers.

I'll disagree here. We should be explicit with the current price. Otherwise it sounds like:

Original price was $XX. $XX.

There's no context for the second number.

Here's without clarification (🔉 on):

without-original-price.mov

Here's with clarification (🔉 on):

with-original-price.mov

[jorgeatorres]

Hi @BFTrick!

Thanks for making those adjustments. I agree with your decisions here. The code looks good to me, but there are just a few minor things to address.

• There are a few coding standards violations in the code (basically spacing related), which you can see here: https://github.com/woocommerce/woocommerce/actions/runs/7944951176/job/21691017251.
• You have to add a changelog file to the PR. You can do that by running command pnpm --filter=@woocommerce/plugin-woocommerce changelog add and responding to the questions in the wizard. Let me know if you're having trouble with that.
• The changes in the HTML broke some of the E2E tests. If you're not sure how to address those, I'll take a look myself.
• A few suggestions (which I've left as comments in the code review).

Once again, thanks for working on this and for your patience during review. Much appreciated.

[BFTrick]

@jorgeatorres

Appreciate your feedback! I've been trying to work on this for 2 weeks now but just haven't had the time. We're launching a big sale in just a couple days so that's taken priority.

I merged in your feedback. I did my best to fix the whitespace issues from Lint but I'm not sure where all of them are coming from. The line numbers seem off. 🤔

I generally cleaned up the whitespace though. 🧹

I also don't have my repo setup the right way to build the changelog 🙈

Is it possible for you to carry this across the finish line? 🤞

[jorgeatorres]

LGTM. Thank you for your work on this @BFTrick!

I've added the changelog file and addressed a few issues with the unit and E2E tests due to the change in markup. We should be good now.