Improve a11y of amp-social-share #16737

eduardogoncalves · 2020-08-05T19:54:33Z

Fix at Jetpack side: ampproject/amphtml#29563 and Automattic/newspack-theme#1014

When a button doesn't have an accessible name, screen readers announce it as "button", making it unusable for users who rely on screen readers.

I think this should be fixed when writing amp-social-share tag on HTML template.

Changes proposed in this Pull Request:

Add aria-label and title using a translated string to $attrs on get_amp_display method

Does this pull request change what data or activity we track or use?

No

Testing instructions:

When you hover over a share button, it will display a tooltip

Proposed changelog entry for your changes:

Improve accessibility (a11y) of amp-social-share buttons

… readers

modules/sharedaddy/sharing-sources.php

Co-authored-by: Weston Ruter <[email protected]>

jetpackbot · 2020-08-05T20:18:32Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16737

Scheduled Jetpack release: September 1, 2020.
Scheduled code freeze: August 25, 2020

Generated by 🚫 dangerJS against 8cd7633

Instead of using *shortname* and *sharing_label* , I changed `get_amp_display` method adding to it's *attrs* `aria-label` and `title` as translated string.

eduardogoncalves · 2020-08-05T21:19:47Z

Hi @westonruter, I've fixed it as you suggested. Can you review it again?

fix tests with the expected arial-label and title

add missing Pocket aria-label and title

modules/sharedaddy/sharing-sources.php

Co-authored-by: Weston Ruter <[email protected]>

eduardogoncalves · 2020-08-06T00:54:52Z

What do I have to do to fix codeclimate issues?

jeherve

What do I have to do to fix codeclimate issues?

Those point towards repetitions in the code. I have not tested this on a real site, but what would you think about doing something like this? It would simplify things a bit:

diff --git a/modules/sharedaddy/sharing-sources.php b/modules/sharedaddy/sharing-sources.php
index 712c82bd9..2b58c5e6f 100644
--- a/modules/sharedaddy/sharing-sources.php
+++ b/modules/sharedaddy/sharing-sources.php
@@ -266,11 +266,19 @@ abstract class Sharing_Source {
 	 * @param array $attrs Custom attributes for rendering the social icon.
 	 */
 	protected function build_amp_markup( $attrs = array() ) {
+		$title = sprintf(
+			/* translators: placeholder is a service name, such as "Twitter" or "Facebook". */
+			__( 'Click to share on %s', 'jetpack' ),
+			$this->get_name()
+		);
+
 		$attrs        = array_merge(
 			array(
-				'type'   => $this->get_id(),
-				'height' => '32px',
-				'width'  => '32px',
+				'type'       => $this->get_id(),
+				'height'     => '32px',
+				'width'      => '32px',
+				'aria-label' => $title,
+				'title'      => $title,
 			),
 			$attrs
 		);
@@ -835,20 +843,6 @@ class Share_Twitter extends Sharing_Source {
 		}
 	}
 
-	/**
-	 * AMP display for Twitter.
-	 *
-	 * @param \WP_Post $post The current post being viewed.
-	 */
-	public function get_amp_display( $post ) {
-		$attrs = array(
-			'aria-label' => __( 'Click to share on Twitter', 'jetpack' ),
-			'title' => __( 'Click to share on Twitter', 'jetpack' ),
-		);
-
-		return $this->build_amp_markup( $attrs );
-	}
-
 	public function process_request( $post, array $post_data ) {
 		$post_title = $this->get_share_title( $post->ID );
 		$post_link = $this->get_share_url( $post->ID );
@@ -946,8 +940,6 @@ class Share_Reddit extends Sharing_Source {
 	public function get_amp_display( $post ) {
 		$attrs = array(
 			'data-share-endpoint' => esc_url_raw( 'https://reddit.com/submit?url=' . rawurlencode( $this->get_share_url( $post->ID ) ) . '&title=' . rawurlencode( $this->get_share_title( $post->ID ) ) ),
-			'aria-label' => __( 'Click to share on Reddit', 'jetpack' ),
-			'title' => __( 'Click to share on Reddit', 'jetpack' ),
 		);
 
 		return $this->build_amp_markup( $attrs );
@@ -1140,8 +1132,6 @@ class Share_Facebook extends Sharing_Source {
 		$attrs = array(
 			/** This filter is documented in modules/sharedaddy/sharing-sources.php */
 			'data-param-app_id' => apply_filters( 'jetpack_sharing_facebook_app_id', '249643311490' ),
-			'aria-label' => __( 'Click to share on Facebook', 'jetpack' ),
-			'title' => __( 'Click to share on Facebook', 'jetpack' ),
 		);
 
 		return $this->build_amp_markup( $attrs );
@@ -1780,8 +1770,6 @@ class Share_Pocket extends Sharing_Source {
 	public function get_amp_display( $post ) {
 		$attrs = array(
 			'data-share-endpoint' => esc_url_raw( 'https://getpocket.com/save/?url=' . rawurlencode( $this->get_share_url( $post->ID ) ) . '&title=' . rawurlencode( $this->get_share_title( $post->ID ) ) ),
-			'aria-label' => __( 'Click to share on Pocket', 'jetpack' ),
-			'title' => __( 'Click to share on Pocket', 'jetpack' ),
 		);
 
 		return $this->build_amp_markup( $attrs );
@@ -1837,8 +1825,6 @@ class Share_Telegram extends Sharing_Source {
 	public function get_amp_display( $post ) {
 		$attrs = array(
 			'data-share-endpoint' => esc_url_raw( 'https://telegram.me/share/url?url=' . rawurlencode( $this->get_share_url( $post->ID ) ) . '&text=' . rawurlencode( $this->get_share_title( $post->ID ) ) ),
-			'aria-label' => __( 'Click to share on Telegram', 'jetpack' ),
-			'title' => __( 'Click to share on Telegram', 'jetpack' ),
 		);
 
 		return $this->build_amp_markup( $attrs );
@@ -1872,8 +1858,6 @@ class Jetpack_Share_WhatsApp extends Sharing_Source {
 	public function get_amp_display( $post ) {
 		$attrs = array(
 			'type' => 'whatsapp',
-			'aria-label' => __( 'Click to share on WhatsApp', 'jetpack' ),
-			'title' => __( 'Click to share on WhatsApp', 'jetpack' ),
 		);
 
 		return $this->build_amp_markup( $attrs );
@@ -1950,8 +1934,6 @@ class Share_Skype extends Sharing_Source {
 				rawurlencode( $this->get_share_url( $post->ID ) ),
 				'en-US'
 			),
-			'aria-label' => __( 'Click to share on Skype', 'jetpack' ),
-			'title' => __( 'Click to share on Skype', 'jetpack' ),
 		);
 
 		return $this->build_amp_markup( $attrs );
diff --git a/tests/php/3rd-party/test_class.jetpack-amp-support.php b/tests/php/3rd-party/test_class.jetpack-amp-support.php
index 73723af7c..ce4b5b2f0 100644
--- a/tests/php/3rd-party/test_class.jetpack-amp-support.php
+++ b/tests/php/3rd-party/test_class.jetpack-amp-support.php
@@ -38,7 +38,7 @@ class WP_Test_Jetpack_AMP_Support extends WP_UnitTestCase {
 
 		$social_icons = Jetpack_AMP_Support::render_sharing_html( '<div class="sd-content"><ul><li>Facebook</li></ul></div>', $services );
 
-		$this->assertEquals( '<div class="sd-content"><amp-social-share type="facebook" height="32px" width="32px" data-param-app_id="249643311490" aria-label="Click to share on Facebook" title="Click to share on Facebook"></amp-social-share></div>', $social_icons );
+		$this->assertEquals( '<div class="sd-content"><amp-social-share type="facebook" height="32px" width="32px" aria-label="Click to share on Facebook" title="Click to share on Facebook" data-param-app_id="249643311490"></amp-social-share></div>', $social_icons );
 
 		// Print.
 		$services = array(
@@ -71,7 +71,7 @@ class WP_Test_Jetpack_AMP_Support extends WP_UnitTestCase {
 
 		$social_icons = Jetpack_AMP_Support::render_sharing_html( '<div class="sd-content"><ul><li>Pocket</li></ul></div>', $services );
 
-		$this->assertEquals( '<div class="sd-content"><amp-social-share type="pocket" height="32px" width="32px" data-share-endpoint="https://getpocket.com/save/?url=http%3A%2F%2Fexample.org%2F%3Fp%3D' . $post->ID . '&amp;title=Test%20post" aria-label="Click to share on Pocket" title="Click to share on Pocket"></amp-social-share></div>', $social_icons );
+		$this->assertEquals( '<div class="sd-content"><amp-social-share type="pocket" height="32px" width="32px" aria-label="Click to share on Pocket" title="Click to share on Pocket" data-share-endpoint="https://getpocket.com/save/?url=http%3A%2F%2Fexample.org%2F%3Fp%3D' . $post->ID . '&amp;title=Test%20post"></amp-social-share></div>', $social_icons );
 
 		// Reset global post.
 		$post = null;

eduardogoncalves · 2020-08-10T14:06:32Z

@jeherve, thank you, I was missing the placeholder, 'Click to share on %s', it's much better.

Fix to use a placeholder 'Click to share on %s'

jeherve · 2020-08-10T14:14:35Z

You'll find that you'll need to update the unit tests as well, as per my diff, as the order of the attributes is now different.

Updated the unit tests as the order of the attributes is now different.

eduardogoncalves · 2020-08-10T21:49:31Z

Is it 'Ok' now?

modules/sharedaddy/sharing-sources.php

use spaces inside inline structures Co-authored-by: Jeremy Herve <[email protected]>

jeherve · 2020-08-12T07:40:09Z

Internal reference: D48000-code

jeherve · 2020-08-12T07:48:33Z

r212034-wpcom

* master: (41 commits) use blog token to make the request (#16635) External Media: Add account disconnect button (#16759) CI: Try collect js coverage (#16786) Sync: Fix nonce action string in theme edit sync (#16702) Connect-in-place: hide new heading during connection process (#16703) Update dependency eslint-plugin-jsdoc to v30.2.1 (#16765) Theme Tools: Resolve PHP 7.4 array offset notice. (#16795) New shell command for easier access to the database. (#16761) My Plan: Add Offer Reset project new plans (Jetpack Security, Jetpack Complete) (#16739) Increase the `editor.MediaUpload` hook priority (#16669) External Media: Remove `speak` announcement when inserting media. Extensions: make `render_callback` optional when checking block registration against plan (#16746) Conditional check for wrapper before giving focus to new page (#16817) Docker: Add package testing shortcut (#16810) Settings: Recognize valid Akismet keys from wp-config and restrict input (#16542) Social Previews: Add Modal (#16704) Update dependency preact to v10.4.7 (#16768) Improve a11y of amp-social-share (#16737) Instant Search: Tweak expanded result path styling (#16762) Docker: Add phpmyadmin to the docker-composer.yml (#16806) ...

Co-authored-by: Weston Ruter <[email protected]> Co-authored-by: Jeremy Herve <[email protected]>

Improve a11y of amp-social-share. Add aria-label and title for screen…

74dd3d7

… readers

matticbot added the Touches WP.com Files label Aug 5, 2020

westonruter reviewed Aug 5, 2020

View reviewed changes

modules/sharedaddy/sharing-sources.php Outdated Show resolved Hide resolved

Update modules/sharedaddy/sharing-sources.php

0fc3004

Co-authored-by: Weston Ruter <[email protected]>

eduardogoncalves marked this pull request as draft August 5, 2020 20:49

Update sharing-sources.php

9a87255

Instead of using *shortname* and *sharing_label* , I changed `get_amp_display` method adding to it's *attrs* `aria-label` and `title` as translated string.

eduardogoncalves marked this pull request as ready for review August 5, 2020 21:07

eduardogoncalves changed the title ~~Improve a11y of amp-social-share. Add aria-label and title for screen…~~ [Status] Needs Review | Improve a11y of amp-social-share Aug 5, 2020

eduardogoncalves changed the title ~~[Status] Needs Review | Improve a11y of amp-social-share~~ Improve a11y of amp-social-share Aug 5, 2020

eduardogoncalves added 2 commits August 5, 2020 18:44

Update test_class.jetpack-amp-support.php

fcbc82e

fix tests with the expected arial-label and title

Update sharing-sources.php

3ba099b

add missing Pocket aria-label and title

westonruter reviewed Aug 5, 2020

View reviewed changes

modules/sharedaddy/sharing-sources.php Outdated Show resolved Hide resolved

Update modules/sharedaddy/sharing-sources.php

70d1c32

Co-authored-by: Weston Ruter <[email protected]>

jeherve added [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Focus] Accessibility Improving usability for all users (a11y) [Feature] Sharing Post sharing, sharing buttons labels Aug 10, 2020

jeherve added this to the 8.9 milestone Aug 10, 2020

jeherve requested changes Aug 10, 2020

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Aug 10, 2020

Update sharing-sources.php

a79d3f5

Fix to use a placeholder 'Click to share on %s'

Update test_class.jetpack-amp-support.php

9ffab51

Updated the unit tests as the order of the attributes is now different.

jeherve requested changes Aug 11, 2020

View reviewed changes

modules/sharedaddy/sharing-sources.php Outdated Show resolved Hide resolved

Update modules/sharedaddy/sharing-sources.php

8cd7633

use spaces inside inline structures Co-authored-by: Jeremy Herve <[email protected]>

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Aug 12, 2020

jeherve approved these changes Aug 12, 2020

View reviewed changes

jeherve merged commit bf95992 into Automattic:master Aug 12, 2020

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 12, 2020

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Aug 25, 2020

jeherve added a commit that referenced this pull request Aug 25, 2020

Changelog: add #16737

417a8e6

pereirinha pushed a commit that referenced this pull request Sep 10, 2020

[not verified] Improve a11y of amp-social-share (#16737)

f1da07e

Co-authored-by: Weston Ruter <[email protected]> Co-authored-by: Jeremy Herve <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve a11y of amp-social-share #16737

Improve a11y of amp-social-share #16737

Uh oh!

eduardogoncalves commented Aug 5, 2020 •

edited

Loading

Uh oh!

Uh oh!

jetpackbot commented Aug 5, 2020 •

edited

Loading

Uh oh!

eduardogoncalves commented Aug 5, 2020

Uh oh!

Uh oh!

eduardogoncalves commented Aug 6, 2020

Uh oh!

jeherve left a comment •

edited

Loading

Uh oh!

eduardogoncalves commented Aug 10, 2020

Uh oh!

jeherve commented Aug 10, 2020

Uh oh!

eduardogoncalves commented Aug 10, 2020

Uh oh!

Uh oh!

jeherve commented Aug 12, 2020

Uh oh!

jeherve commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Improve a11y of amp-social-share #16737

Improve a11y of amp-social-share #16737

Uh oh!

Conversation

eduardogoncalves commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Does this pull request change what data or activity we track or use?

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

Uh oh!

jetpackbot commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduardogoncalves commented Aug 5, 2020

Uh oh!

Uh oh!

eduardogoncalves commented Aug 6, 2020

Uh oh!

jeherve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardogoncalves commented Aug 10, 2020

Uh oh!

jeherve commented Aug 10, 2020

Uh oh!

eduardogoncalves commented Aug 10, 2020

Uh oh!

Uh oh!

jeherve commented Aug 12, 2020

Uh oh!

jeherve commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eduardogoncalves commented Aug 5, 2020 •

edited

Loading

jetpackbot commented Aug 5, 2020 •

edited

Loading

jeherve left a comment •

edited

Loading