Shared block form: manage focus to avoid focus losses. #6207
Conversation
afercia
added
the
[Focus] Accessibility (a11y)
| this.titleRef.select(); | ||
| } | ||
| // Move focus back to the Edit button after pressing Save or Cancel. | ||
| if ( ! this.props.isEditing && ! this.props.isSaving ) { |
There was a problem hiding this comment.
We're not checking for a change in state, so there's the potential that this condition is satisfied when not intended on future renders. Should we have prevProps in here somewhere to make sure something has changed?
Is it...
if ( ! this.props.isEditing && prevProps.isSaving && ! this.props.isSaving ) {?
There was a problem hiding this comment.
Yeah but it wouldn't work when pressing "Cancel" 😞
There was a problem hiding this comment.
I understand the potential issue, however is there really a case when is not Editing and not Saving other than the initial view (form closed)?
There was a problem hiding this comment.
other than the initial view (form closed)?
Correct, but we could anticipate a component update to occur and trigger focus to the edit button unexpectedly here. Maybe a plugin modifies the block title externally, triggering an update and forcing the focus? Not the best example 😄 But in general, we'd always want to make sure in this lifecycle method that something has changed between prevProps and this.props.
I think the correct version would be:
const { isEditing, isSaving } = this.props;
if ( ( prevProps.isEditing && ! isEditing ) || ( prevProps.isSaving && ! isSaving ) {
this.editButton.current.focus();
}Alternatively, I wonder if we should just extract out the form into a separate component (local to the same directory, e.g. block-panel/form.js), and apply withFocusReturn to it, so that when it unmounts, we automatically shift focus back to where it was when it first mounted.
| } | ||
|
|
||
| bindTitleRef( ref ) { | ||
| this.titleRef = ref; | ||
| } | ||
|
|
||
| bindEditButtonRef( ref ) { |
There was a problem hiding this comment.
Aside: React 16.3.0 introduced a new createRef function, which is arguably easier to use because it doesn't require this callback function, only to assign the ref in constructor and use it in render:
https://reactjs.org/docs/refs-and-the-dom.html#creating-refs
Main difference is that it has to be accessed by the .current property.
Would introduce a bit of inconsistency in this component, but I'd argue it's an incremental step in the right direction.
There was a problem hiding this comment.
Would introduce a bit of inconsistency
I can change also the other one, though unrelated.
|
@aduth I can't think of another way to better check for |
|
Maybe a bit more elegant way would be adding a new state property in the parent component, for example |
| @@ -20,20 +20,22 @@ class SharedBlockEditPanel extends Component { | |||
| constructor() { | |||
| super( ...arguments ); | |||
|
|
|||
| this.bindTitleRef = this.bindTitleRef.bind( this ); | |||
| this.sharedBlockNameField = createRef(); | |||
There was a problem hiding this comment.
I think titleField would be a more consistent name for this variable. Note that we call it title and not name elsewhere in the code.
There was a problem hiding this comment.
OK agree consistency is key, though that's not exactly the shared block "title" but it's how users will name the shared block ;)
* Shared block form: manage focus to avoid focus losses. * Use createRef(). * Better check prevProps. * Better naming.
This PR tries to avoid focus losses on the Shared block form when the component gets fully re-rendered. For details, please see the related issue #6204.
Previously, the component attempted to
select()the input field contents oncomponentDidMount()however, at that point the component is already mounted so the selection (and focus moved to the input field) just didn't happen.Using
componentDidUpdate()seems appropriate, and allows to check when the form gets open so we can select the text just once. Also allows to move focus back to the Edit button when the component is not being edited or saved (i.e. when the form closes).To test:
Fixes #6204