fix(a11y): maintain focus when uploading an image

[dylanb]

Description

Changed the image block to maintain focus when uploading an image

How has this been tested?

Tested this in the browser (Chrome) only and also tested with Voiceover on OS X

Types of changes

Introduces the concept of an "autoFocus" state to the Image Block component, Introduces a ref to the same component, focuses the figcaption when the autoFocus state is set

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[dylanb]

I am not sure what sort of automated tests you would expect to see for code like this, so I would appreciate some guidance on that

[aduth]

Rather than setting this in state, could the focusing just occur as part of the logic for onSelectImage ?

The issue with this is that setState will force a render, and is generally discouraged to be incorporated into componentDidUpdate, since the nature of the lifecycle method is such that it means we'll have a minimum of two renders (when ideally it'd just be one max per "change") and leaves open the potential for an infinite loop (since this will re-call componentDidUpdate).

[dylanb]

The problem here is that the DOM elements do not yet exist when onSelectImage gets called. The if statement is intended to stop the infinite loop.

[aduth]

Another option could be to assign this value as an instance property, so that we don't need to call setState here. It's a bit more fragile / non-standard, but specifically avoids the unneeded render.

Effectively, the difference would be:

this.setState( {
	isEditing: false,
} );
this.autoFocusCaption = true;

if ( this.autoFocusCaption && this.myRef.current ) {
	this.autoFocusCaption = false;
	// Or: 
	// delete this.autoFocusCaption;
	const caption = this.myRef.current.querySelector( 'figcaption' );
	// ...

[aduth]

Ideally we'd just assign the intended focus target as the ref. This is blocked by #9740. It seems like it'd be reasonable enough for RichText to use forwardRef to make the underlying element available for reference.

[dylanb]

I agree that having a mechanism for managing focus more globally would be good and could eventually replace this individual fix

[aduth]

I'd encourage using createRef for this instead, largely because it requires less code to implement.

https://reactjs.org/docs/refs-and-the-dom.html

[dylanb]

I can make this change, initially it did not work for me but I realize that was due to the fact that I was trying to use the <Fragment> which does not work at all.

[aduth]

We should never reference any globals, nor use React directly.

The creatRef function is available from the @wordpress/element package.

The line below further down in the file:

import { Component, Fragment } from '@wordpress/element';

...can be changed to:

import { Component, Fragment, createRef } from '@wordpress/element';

[aduth]

Another option could be to assign this value as an instance property, so that we don't need to call setState here. It's a bit more fragile / non-standard, but specifically avoids the unneeded render.

Effectively, the difference would be:

this.setState( {
	isEditing: false,
} );
this.autoFocusCaption = true;

if ( this.autoFocusCaption && this.myRef.current ) {
	this.autoFocusCaption = false;
	// Or: 
	// delete this.autoFocusCaption;
	const caption = this.myRef.current.querySelector( 'figcaption' );
	// ...

[afercia]

I guess this is something that was discussed in person at WCUS? 🙂Wasn't immediately clear what is the goal of this PR and how to test it.

Had a quick look and I'm not sure focus should be moved to the caption. I'd tend to think moving focus shouldn't be used to assume a specific work flow. Entering a caption is only one of the available tasks for users after they've uploaded an image. For example, users might want to:

• align the image
• set it full width
• access other options in the toolbar
• and yes, also insert a caption

Also, focusing the caption would force keyboard users to tab backwards to be able to access any other action other than inserting a caption.

Instead, I'd consider to move focus to the block focusable container: this way all the block UI is available in the natural tab order:

[afercia]

See comment.

[dylanb]

@afercia argument in favor of placing the focus in the caption is that it encourages the creation of the text alternative but if you are insistent on the outer block focus, lmk

[afercia]

Focus should be managed programmatically only to "repair" a focus flow in a way that is as close as possible to the native interaction. Not to assume a specific flow.

A bit surprisingly, but very welcomed, this is also what the React accessibility guidelines suggest as a general recommendation:

Use it to repair the keyboard focus flow when it is disturbed, not to try and anticipate how users want to use applications.

[aduth]

What's the current status of this pull request? Are there plans for revisions? Is the direction clear?

The notes of my previous review are still valid.

I guess this is something that was discussed in person at WCUS? 🙂Wasn't immediately clear what is the goal of this PR and how to test it.

There was no prior conversation. My understanding is that it improved a scenario where otherwise a focus loss would occur.

[youknowriad]

Hi There! I've triaging PRs today and it seems this one is stale. I'm going to close now but please consider reopening if you have some time to dedicate to it. Thanks for your efforts.