a11y: Add aria-label attributes and custom override control to core/social-link block #18651

0aveRyan · 2019-11-21T05:08:26Z

Description

Per #18620, I've modified the core/social-link block to contain an aria-label attribute for the anchor elements wrapping the social icon SVGs.

Deque classifies missing descriptive text for links as a serious user impact and Google Lighthouse accessibility scores are negatively impacted without an aria-label if the anchor doesn't contain text or an image with an alt description.

By default, if the user doesn't customize the label in the provided TextControl in the Inspector, the block will render Link to {BRAND}. However, if the block is used 10 times in a document, hearing the same label 10 times isn't very useful if each social icon links to a different profile/type of page, hence the custom attribute to enable clearer descriptions.

At this point, there is a fair amount of duplication between the PHP and JS file containing data about the brands. These are unlikely to change greatly, but I'm happy to address with a different approach if desired.

How has this been tested?

I've run the lint command, inserted each social icon and validated the display and markup for the Editor and Frontend render.

Types of changes

Introduces label attribute for the core/social-link block (only stores customizations, default is handled in render method)
Introduces no breaking changes and progressively enhances any existing social blocks in-use.

Screenshot

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

mkaz

This is a nice addition, thanks.
I added a couple minor suggestions.

packages/block-library/src/social-link/edit.js

packages/block-library/src/social-link/index.php

mkaz · 2019-11-23T16:23:13Z

Unrelated to your changes, I'm still not thrilled with the implementation. I agree there is too much duplication between PHP and JS. The core issue at hand is wrangling the SVG, I have one unsuccessful attempt here: #18243

Additionally, I think we can simplify the list if we can solve the above, and then implement the ability to extend or modify, related issue #17277

0aveRyan · 2019-11-24T19:21:39Z

@mkaz Thanks for the review! I'll push a commit to address the description & LinkedIn.

I'm also a fan of svgr, but from the chatter on #18243 sounds like that may require some creative solutions for the Gutenberg codebase.

I agree the implementation is fairly easy to understand, but perhaps leaves something to be desired from a developer experience and maintenance standpoint. Unfortunately, most other approaches I can think of would either transfer that burden to visitors or obfuscate to developers what's happening.

On my brain apart from svgr...

Leave implementation as-is, add filter hooks in JS & PHP for extension and enhance with clearer inline documentation that any changes must happen to both objects -- still duplicated but extensible and clearer.
Add a socialData.json file with icons base64 encoded and filter hooks in JS & PHP to allow extension -- adds filesystem read in PHP and base64 strings would be about 1/3 larger.
Use a socialData.json file to inject the objects into JS & PHP during webpack build, base64 decode icons and add filter hooks -- obfuscates what's happening and some complexity, but reduces client burden.
Separate icons into a SVG sprite approach or one-off SVG files, use a socialData.json to point to the proper icon and add filter hooks -- makes a larger payload to transmit to client/increases client requests, but makes using a data file easier.

mkaz · 2019-11-25T02:32:58Z

One more, Github --> GitHub, see #18714

With regards to SVG, thanks for the suggestions, I'll need to noodle on them a bit more, I like how you draw out the pros and cons to the various solutions. We discuss further in a separate PR, I have on my list to move them out of PHP into single SVG files this would improve one aspect, but need to consider how it might change after this PR, it may not make it easier overall

0aveRyan · 2019-12-03T01:43:40Z

@mkaz I've addressed the branding label issues and field helper text. Should we start a new issue to discuss and address the social SVG storage and label data?

mkaz

Updates looks good, thanks! 👍

Yes, I think a new issue to discuss the alternate storage would be best.

packages/block-library/src/social-link/edit.js

packages/block-library/src/social-link/index.php

packages/block-library/src/social-link/social-list.js

0aveRyan · 2019-12-26T02:11:30Z

@youknowriad I've addressed your feedback, this should be good to go.

youknowriad · 2019-12-26T08:30:10Z

packages/block-library/src/social-link/edit.js

@@ -6,18 +6,23 @@ import classNames from 'classnames';
 /**


I see a few strings here where we use Title Case. I think we've been recently moving to always using "Sentence case" in all of our strings. @karmatosed do you confirm?

@youknowriad that doesn't appear to be the standard used for Panel Titles and that doesn't seem to be the standard outlined in the docs. Was there a discussion about that I missed? If that's changed, the docs need to be updated and issues opened to address the many instances of title case throughout the application.

I've since dug and found the PRs and issue discussion around sentence case. A major departure like that should be discussed and memorialized in docs with clearer examples well before it's put into practice, to set clear expectations, avoid confusing situations like this and throwing up hurdles to contribution. Let me know if you'd like these strings updated to sentence case.

I don't know to be honest, I'll let @karmatosed chime in when she comes back. I think we can land this as is for now.

packages/block-library/src/social-link/edit.js

aduth · 2020-02-10T19:40:46Z

packages/block-library/src/social-link/index.php

- $url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false;
+ $site = ( isset( $attributes['site'] ) ) ? $attributes['site'] : 'Icon';
+ $url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false;
+ $label = ( isset( $attributes['label'] ) ) ? $attributes['label'] : __( 'Link to ' ) . core_social_link_get_name( $site );


I believe that this should be using a placeholder:

https://codex.wordpress.org/I18n_for_WordPress_Developers#Placeholders

Otherwise, translators will only be provided the string "Link to " without contextual clues to know that it is being used in a concatenation. Furthermore, some translations may require that the position of the placeholder be changed relative to the rest of the sentence.

Even with a placeholder, this sort of string can still be problematic where the translation may vary depending on e.g. the gender of the noun of the placeholder value. In this case, I don't believe that should be a problem, given that the values to be substituted are proper names of companies / services.

That said, it would be good to:

Confirm whether this is a correct assumption.

Update to use a placeholder.

Consider including a translator comment to explain this.

Issue for tracking purposes: #20152

afercia · 2020-02-26T14:09:22Z

The aria-label doesn't seem to work for me: whatever I enter in the field, the aria-label rendered on the front end has always the default value "Link to {something}".

@youknowriad @aduth do you see any potential regression since this was merged?

aduth · 2020-02-26T16:21:50Z

@afercia I'm not personally familiar enough with these changes to know if it had ever been working as expected, but I can also reproduce the issue you're seeing.

aduth · 2020-02-26T16:34:24Z

On further investigation, it appears to have regressed as a result of #19887, where the label attribute type was incorrectly assigned as 'number', which would invalidate the custom string value. This was correctly assigned as 'string' previously.

gutenberg/packages/block-library/src/social-link/block.json

Lines 12 to 14 in fcbc6ca

                           
                              "label": {
                             
                              "type": "number"
                             
                              }

cc @mcsf

aduth · 2020-02-26T16:37:54Z

Fix at #20468

afercia · 2020-02-26T16:49:21Z

@aduth thanks so much for the quick investigation and fix!

add aria-label to social link block

bb35aaf

0aveRyan requested a review from mkaz as a code owner November 21, 2019 05:08

phpcs codesmells

aadde37

0aveRyan mentioned this pull request Nov 21, 2019

core/social-link component doesn't render accessible markup #18620

Closed

phpcs docblock and one bad array key

fcee78e

mkaz requested changes Nov 23, 2019

View reviewed changes

packages/block-library/src/social-link/edit.js Outdated Show resolved Hide resolved

packages/block-library/src/social-link/index.php Outdated Show resolved Hide resolved

0aveRyan added 2 commits December 2, 2019 18:40

social link brand spellings

c9ffe6b

sidebar helper text

05541ca

mkaz approved these changes Dec 9, 2019

View reviewed changes

mkaz mentioned this pull request Dec 9, 2019

Social Links Block: SoundCloud Typo #18994

Closed

6 tasks

0aveRyan mentioned this pull request Dec 10, 2019

Social Links Block: Icon Storage/Extension Architecture #19041

Open

0aveRyan added [Type] Bug An existing feature does not function as intended and removed Needs Accessibility Feedback Need input from accessibility labels Dec 18, 2019

youknowriad reviewed Dec 24, 2019

View reviewed changes

packages/block-library/src/social-link/edit.js Outdated Show resolved Hide resolved

youknowriad reviewed Dec 24, 2019

View reviewed changes

packages/block-library/src/social-link/edit.js Outdated Show resolved Hide resolved

youknowriad reviewed Dec 24, 2019

View reviewed changes

packages/block-library/src/social-link/index.php Outdated Show resolved Hide resolved

youknowriad reviewed Dec 24, 2019

View reviewed changes

packages/block-library/src/social-link/social-list.js Show resolved Hide resolved

0aveRyan and others added 4 commits December 25, 2019 18:14

addressing feedback

f3bda49

Merge branch 'master' into a11y/social-link-anchor-labels

d10d5ef

linting

982b894

final linting

6d319a2

0aveRyan added 2 commits December 25, 2019 19:12

label text removed in swap

c20f894

doh, final translation string fix

cf861db

youknowriad reviewed Dec 26, 2019

View reviewed changes

youknowriad merged commit 2cdeb19 into WordPress:master Dec 27, 2019

0aveRyan deleted the a11y/social-link-anchor-labels branch December 31, 2019 16:04

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

aduth reviewed Jan 7, 2020

View reviewed changes

packages/block-library/src/social-link/edit.js Show resolved Hide resolved

aduth mentioned this pull request Jan 7, 2020

Block Editor: Remove (more) legacy "editor-" class name compatibility #19489

Merged

aduth reviewed Feb 10, 2020

View reviewed changes

aduth mentioned this pull request Feb 10, 2020

Block Library: Social Links: Translated label string should use placeholder #20152

Closed

aduth mentioned this pull request Feb 26, 2020

Block Library: Social Link: Fix label attribute type as string #20468

Merged

Soean mentioned this pull request Mar 5, 2020

Remove Redundant "Link to" in Social Icons aria-label #20665

Closed

0aveRyan mentioned this pull request Apr 3, 2020

Fix overly verbose aria-label in Social Link block #21369

Merged

6 tasks

priethor removed the [a11y] Labelling label Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a11y: Add aria-label attributes and custom override control to core/social-link block #18651

a11y: Add aria-label attributes and custom override control to core/social-link block #18651

0aveRyan commented Nov 21, 2019 •

edited by gziolo

mkaz left a comment •

edited

mkaz commented Nov 23, 2019 •

edited

0aveRyan commented Nov 24, 2019

mkaz commented Nov 25, 2019

0aveRyan commented Dec 3, 2019 •

edited

mkaz left a comment

0aveRyan commented Dec 26, 2019

youknowriad Dec 26, 2019

0aveRyan Dec 26, 2019

0aveRyan Dec 27, 2019 •

edited

youknowriad Dec 27, 2019

aduth Feb 10, 2020

aduth Feb 10, 2020

afercia commented Feb 26, 2020

aduth commented Feb 26, 2020

aduth commented Feb 26, 2020

aduth commented Feb 26, 2020

afercia commented Feb 26, 2020

a11y: Add aria-label attributes and custom override control to core/social-link block #18651

a11y: Add aria-label attributes and custom override control to core/social-link block #18651

Conversation

0aveRyan commented Nov 21, 2019 • edited by gziolo

Description

How has this been tested?

Types of changes

Screenshot

Checklist:

mkaz left a comment • edited

Choose a reason for hiding this comment

mkaz commented Nov 23, 2019 • edited

0aveRyan commented Nov 24, 2019

mkaz commented Nov 25, 2019

0aveRyan commented Dec 3, 2019 • edited

mkaz left a comment

Choose a reason for hiding this comment

0aveRyan commented Dec 26, 2019

youknowriad Dec 26, 2019

Choose a reason for hiding this comment

0aveRyan Dec 26, 2019

Choose a reason for hiding this comment

0aveRyan Dec 27, 2019 • edited

Choose a reason for hiding this comment

youknowriad Dec 27, 2019

Choose a reason for hiding this comment

aduth Feb 10, 2020

Choose a reason for hiding this comment

aduth Feb 10, 2020

Choose a reason for hiding this comment

afercia commented Feb 26, 2020

aduth commented Feb 26, 2020

aduth commented Feb 26, 2020

aduth commented Feb 26, 2020

afercia commented Feb 26, 2020

0aveRyan commented Nov 21, 2019 •

edited by gziolo

mkaz left a comment •

edited

mkaz commented Nov 23, 2019 •

edited

0aveRyan commented Dec 3, 2019 •

edited

0aveRyan Dec 27, 2019 •

edited