Insert link accessibility #1575

mehigh · 2017-06-29T05:06:20Z

Description

Fixes #694 by improving the screen reader accessibility of the element.
My updates apply to the add link and edit link pop-out elements, handling the following aspects:

The buttons (Apply, Edit, Remove) now have the aria-label attribute added in for accessibility purposes. There is no visual change for them.
The URL input now has an ID that makes use of the withInstanceId feature from components
The URL input now has a screen-reader-text label which uses the ID on the for attribute to improve the screen reader accessibility of the element. This label is not visible.

How Has This Been Tested?

I used the browser's inspector (screenshots reflect that) to confirm the presence of the a11y attributes and screen-reader-text elements on both the add link and edit link forms.

The updates done in blocks/editable/format-toolbar.js were visible in the add link / edit existing link workflow in the text block, just like the screenshots show.

I wasn't able to figure out where is the "blocks/library/button/index.js" actually used. It has in the edit function a form with a class of editable-format-toolbar__link-modal, just like the one generated with the insert/edit URL.

Locally only I tried adding an extra class on the form.editable-format-toolbar__link-modal and trying to hunt it in the browser, but I wasn't able to stumble upon it myself. That's why the changes done on that file are minimal (added the button aria-label and added a and added an ID to the URL input element itself, without making use of withInstanceId - because there is no export default I could add the withInstanceId call around).

Screenshots:

Add link form:

Edit link form:

Types of changes

New feature (non-breaking change which adds functionality).

Checklist:

My code is tested.
My code follows the WordPress code style.

…nlink)

swissspidy · 2017-06-29T07:49:11Z

blocks/editable/format-toolbar.js

@@ -151,17 +151,19 @@ class FormatToolbar extends Component {
 						className="editable-format-toolbar__link-modal"
 						style={ linkStyle }
 						onSubmit={ this.submitLink }>
+						<label className="screen-reader-text" htmlFor="editable-format-toolbar__link-input">URL</label>


This should be { __( 'URL' ) } so it's translatable

swissspidy · 2017-06-29T07:49:51Z

blocks/library/button/index.js

@@ -60,15 +60,17 @@ registerBlockType( 'core/button', {
 					<form
 						className="editable-format-toolbar__link-modal"
 						onSubmit={ ( event ) => event.preventDefault() }>
+						<label className="screen-reader-text" htmlFor="editable-format-toolbar__link-input">URL</label>


This should be { __( 'URL' ) } so it's translatable.

…abel.

mehigh · 2017-06-29T08:10:42Z

Thank you @swissspidy.
I've handled your feedback.

youknowriad · 2017-06-29T14:02:36Z

blocks/editable/format-toolbar.js

 						<input
 							autoFocus
 							className="editable-format-toolbar__link-input"
+							id="editable-format-toolbar__link-input"


Ids should contain a unique uuid. We should consider using the withInstanceId HoC

Thank you for the linked reference. I'll update the PR to contain it.

@youknowriad
I attempted to make use of withinstanceId, but this fails to work.

I imported the component, and loaded it in the render function as a parameter, but I wasn't able to. I'm not sure whether this ID is needed to be passed into the constructor, or somewhere else, but I'm not able to figure out how to do that.

This is where I attempted to do this:
xwp@7aef002

The error I get is:
format-toolbar.js:121 Uncaught TypeError: Cannot read property 'instanceId' of undefined

I also double-checked and the "editable-format-toolbar__link-modal" only shows up once at all time in the DOM. When trying to add a new link the previous link modal is removed from the DOM before adding the new modal, therefore an ID isn't required in this particular case.

I haven't committed the uuid in this PR, so the feature is still fully functional.

@mehigh Instead of export default FormatToolbar; you'd need to use export default withInstanceId( FormatToolbar ); in order to make it work.

Thank you. I'll give that a try.

Cannot read property 'instanceId' of undefined

Feature/694 instanceId

mehigh · 2017-07-01T20:04:24Z

@swissspidy
I was able to get instanceId working.
I had to look at a few other JS files where this was used to figure out that I need to spread that property into here:
const { formats, focusPosition, enabledControls = DEFAULT_CONTROLS**, instanceId** } = this.props;

Even after adding the export default withInstanceId I couldn't pass instanceId as a parameter to the render() function, where I needed to make use of it.

Instance ID appearing (it was 10 in that example):

Thank you for your reviews.

afercia · 2017-07-03T13:54:45Z

@mehigh thanks! Looks good to me, I think the only thing left is that also the input field for the button block needs a unique ID.

mehigh · 2017-07-03T20:35:40Z

@afercia
In believe you read the updated PR description, right? :)

"I wasn't able to figure out where is the "blocks/library/button/index.js" actually used. It has in the edit function a form with a class of editable-format-toolbar__link-modal, just like the one generated with the insert/edit URL."
Thanks for pointing out on where was that URL editing pop-out actually used.

The button component doesn't have an export statement, so I'm curious on whether the withInstanceId works within it or not. I'll have to test it and see.

afercia · 2017-07-04T07:11:46Z

@mehigh updating the description doesn't trigger a notification ;)

And yes, because of the way the button block uses the link inline toolbar, I'm not sure there's a way to use withInstanceId at the moment. On the other hand, the link inline toolbar should probably be a reusable component. Right now, there are two link toolbar that just "look" the same, but they're two different implementations. This is a bit out of the scope of this issue and should probably be addressed in a separate issue. I'd also like to ping @aduth @youknowriad to ask if there are plans to rework the link inline toolbar as a component.

mehigh · 2017-07-04T08:18:30Z

I was curios that your comment was spot on to the addition I made to the description. Wanted to check whether that triggered your response :).

I'll await for input from aduth / youknowriad and see whether this can be brought home and merged in.

mehigh · 2017-07-12T18:20:11Z

@aduth / @youknowriad
Since the 2nd usage of the URL entry doesn't support withInstanceId, I suggest we leave the current implementation as it's accessible and maybe revise in a different ticket if it's decided to merge the two together.
Hope to move this forward.

aduth · 2017-07-13T19:45:30Z

A block's edit property is a component, meaning that it can be wrapped with withInstanceId to inject an instanceId prop.

diff --git a/blocks/library/button/index.js b/blocks/library/button/index.js
index d49c0344..0d4f9b0e 100644
--- a/blocks/library/button/index.js
+++ b/blocks/library/button/index.js
@@ -2,7 +2,7 @@
  * WordPress dependencies
  */
 import { __ } from 'i18n';
-import { IconButton } from 'components';
+import { IconButton, withInstanceId } from 'components';
 
 /**
  * Internal dependencies
@@ -36,9 +36,10 @@ registerBlockType( 'core/button', {
 		}
 	},
 
-	edit( { attributes, setAttributes, focus, setFocus, className } ) {
+	edit: withInstanceId( ( { attributes, setAttributes, focus, setFocus, className, instanceId } ) => {
 		const { text, url, title, align } = attributes;
 		const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );
+		const inputId = `editable-format-toolbar__link-input-${ instanceId }`;
 
 		return [
 			focus && (
@@ -61,10 +62,15 @@ registerBlockType( 'core/button', {
 					<form
 						className="editable-format-toolbar__link-modal"
 						onSubmit={ ( event ) => event.preventDefault() }>
-						<label className="screen-reader-text" htmlFor="editable-format-toolbar__link-input">{ __( 'URL' ) }</label>
+						<label
+							className="screen-reader-text"
+							htmlFor={ inputId }
+						>
+							{ __( 'URL' ) }
+						</label>
 						<input
 							className="editable-format-toolbar__link-input"
-							id="editable-format-toolbar__link-input"
+							id={ inputId }
 							type="url"
 							required
 							value={ url }
@@ -76,7 +82,7 @@ registerBlockType( 'core/button', {
 				}
 			</span>,
 		];
-	},
+	} ),
 
 	save( { attributes } ) {
 		const { url, text, title, align = 'none' } = attributes;

Alternatively, we can split this out into a separate component. Like @afercia mentions, we should probably create a common component for applying links to avoid managing separate implementations between the formatting toolbar and button link.

I think the patch above is a reasonable interim solution for now.

aduth

#1575 (comment)

mehigh · 2017-07-13T20:49:23Z

Thank you, @aduth for the review and for pointing it out how the ID property can be passed through the edit component. I'll amend the PR tomorrow or on Monday.

mehigh · 2017-07-17T06:48:53Z

@aduth changes have been applied, while adjusting the ID to "editable-format-toolbar__button-link-input-XX" from "editable-format-toolbar__link-input-" in order to avoid a potential clash with the toolbar link form input, as they share different set of IDs.
Thank you for your thorough explanation.

mehigh · 2017-07-18T08:41:49Z

I tried to rebase this to squash all of the merge master commits, but it yielded too many conflicts.
@aduth everything is available here:
#1933

mehigh added 2 commits June 29, 2017 07:36

694 - Add visually hidden screen reader labels to the link form input

cc23407

694 - Added aria-label to all link-related buttons (submit / edit / u…

3768ec4

…nlink)

mehigh changed the title ~~Insert link accessibility #694~~ Insert link accessibility Jun 29, 2017

694 - Applied suggestions for aria-label wordings.

2a995c0

swissspidy reviewed Jun 29, 2017

View reviewed changes

mehigh added 2 commits June 29, 2017 11:09

694 - Added translatable strings for the "URL" in the screen reader l…

f969bc7

…abel.

Merge remote-tracking branch 'WordPress/master' into feature/694

5ec1744

ellatrix added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 29, 2017

youknowriad reviewed Jun 29, 2017

View reviewed changes

mehigh and others added 4 commits July 1, 2017 15:23

Merge remote-tracking branch 'WordPress/master' into feature/694

e4ada8e

Adding withInstanceId - Fails to work

7aef002

Cannot read property 'instanceId' of undefined

Updated the usage of InstanceId according to suggestions

e793377

Merge pull request #1 from xwp/feature/694-instanceId

a7322b1

Feature/694 instanceId

aduth requested changes Jul 13, 2017

View reviewed changes

mehigh added 3 commits July 17, 2017 09:31

Merge remote-tracking branch 'WordPress/master' into feature/694

a44fc71

694 - Improve formatting of label element

1439741

694 - Added withInstanceId to the button link form

0b2df35

Formatting update

70c53dd

mehigh closed this Jul 18, 2017

mehigh deleted the feature/694 branch July 18, 2017 08:38

mehigh mentioned this pull request Jul 18, 2017

Insert link accessibility #1933

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert link accessibility #1575

Insert link accessibility #1575

mehigh commented Jun 29, 2017 •

edited

Loading

swissspidy Jun 29, 2017

swissspidy Jun 29, 2017

mehigh commented Jun 29, 2017

youknowriad Jun 29, 2017 •

edited

Loading

mehigh Jun 29, 2017

mehigh Jul 1, 2017

swissspidy Jul 1, 2017

mehigh Jul 1, 2017

mehigh commented Jul 1, 2017

afercia commented Jul 3, 2017

mehigh commented Jul 3, 2017

afercia commented Jul 4, 2017

mehigh commented Jul 4, 2017

mehigh commented Jul 12, 2017

aduth commented Jul 13, 2017

aduth left a comment

mehigh commented Jul 13, 2017

mehigh commented Jul 17, 2017

mehigh commented Jul 18, 2017

Insert link accessibility #1575

Insert link accessibility #1575

Conversation

mehigh commented Jun 29, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots:

Types of changes

Checklist:

swissspidy Jun 29, 2017

Choose a reason for hiding this comment

swissspidy Jun 29, 2017

Choose a reason for hiding this comment

mehigh commented Jun 29, 2017

youknowriad Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

mehigh Jun 29, 2017

Choose a reason for hiding this comment

mehigh Jul 1, 2017

Choose a reason for hiding this comment

swissspidy Jul 1, 2017

Choose a reason for hiding this comment

mehigh Jul 1, 2017

Choose a reason for hiding this comment

mehigh commented Jul 1, 2017

afercia commented Jul 3, 2017

mehigh commented Jul 3, 2017

afercia commented Jul 4, 2017

mehigh commented Jul 4, 2017

mehigh commented Jul 12, 2017

aduth commented Jul 13, 2017

aduth left a comment

Choose a reason for hiding this comment

mehigh commented Jul 13, 2017

mehigh commented Jul 17, 2017

mehigh commented Jul 18, 2017

mehigh commented Jun 29, 2017 •

edited

Loading

youknowriad Jun 29, 2017 •

edited

Loading