Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Dec 27, 2019
Merged

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

merged 11 commits into from
Dec 27, 2019

Conversation

0aveRyan
Copy link
Contributor

@0aveRyan 0aveRyan commented Nov 21, 2019

Description

Fixes #18620.

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

Screenshot of editor displaying new TextControl component for setting aria-label in core/social-block

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. .

@0aveRyan 0aveRyan requested a review from mkaz as a code owner November 21, 2019 05:08
@0aveRyan 0aveRyan added [a11y] Labelling [Block] Social Affects the Social Block - used to display Social Media accounts [Package] Block library /packages/block-library [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility labels Nov 21, 2019
Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
@mkaz
Copy link
Member

mkaz commented Nov 23, 2019

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
Copy link
Contributor Author

@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
Copy link
Member

mkaz commented Nov 25, 2019

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
Copy link
Contributor Author

0aveRyan commented Dec 3, 2019

@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?

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates looks good, thanks! 👍

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

@mkaz mkaz mentioned this pull request Dec 9, 2019
6 tasks
@0aveRyan 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
@0aveRyan
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-12-26 at 10 03 00 AM

@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.

Copy link
Contributor Author

@0aveRyan 0aveRyan Dec 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@youknowriad youknowriad merged commit 2cdeb19 into WordPress:master Dec 27, 2019
@0aveRyan 0aveRyan deleted the a11y/social-link-anchor-labels branch December 31, 2019 16:04
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
$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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue for tracking purposes: #20152

@afercia
Copy link
Contributor

afercia commented Feb 26, 2020

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
Copy link
Member

aduth commented Feb 26, 2020

@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
Copy link
Member

aduth commented Feb 26, 2020

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.

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

cc @mcsf

@aduth
Copy link
Member

aduth commented Feb 26, 2020

Fix at #20468

@afercia
Copy link
Contributor

afercia commented Feb 26, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core/social-link component doesn't render accessible markup
6 participants