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

Shared block form: manage focus to avoid focus losses. #6207

Merged
merged 4 commits into from Apr 18, 2018

Conversation

Projects
None yet
3 participants
@afercia
Contributor

afercia commented Apr 16, 2018

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 on componentDidMount() 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:

  • using the keyboard, press the Edit button
  • verify focus is moved to the input field and its content gets selected
  • modify the shared block name
  • using the keyboard, tab to focus the Save or Cancel buttons, press them and verify that:
    • focus is moved back to the Edit button
    • anything else works as expected
    • [Edit]: verify the previous 2 points also pressing the Escape key on your keyboard

Fixes #6204

@afercia afercia requested a review from aduth Apr 16, 2018

this.titleRef.select();
}
// Move focus back to the Edit button after pressing Save or Cancel.
if ( ! this.props.isEditing && ! this.props.isSaving ) {

This comment has been minimized.

@aduth

aduth Apr 16, 2018

Member

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 ) {

?

This comment has been minimized.

@afercia

afercia Apr 16, 2018

Contributor

Yeah but it wouldn't work when pressing "Cancel" 😞

This comment has been minimized.

@afercia

afercia Apr 16, 2018

Contributor

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

This comment has been minimized.

@aduth

aduth Apr 17, 2018

Member

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 ) {

This comment has been minimized.

@aduth

aduth Apr 16, 2018

Member

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.

This comment has been minimized.

@afercia

afercia Apr 16, 2018

Contributor

Would introduce a bit of inconsistency
I can change also the other one, though unrelated.

afercia added some commits Apr 16, 2018

@afercia

This comment has been minimized.

Contributor

afercia commented Apr 17, 2018

@aduth I can't think of another way to better check for prevProps. A bit redundant though ;)

@afercia

This comment has been minimized.

Contributor

afercia commented Apr 17, 2018

Maybe a bit more elegant way would be adding a new state property in the parent component, for example isDisplay, update that in the state and pass it as prop to this controlled component? A bit overkill maybe.

@noisysocks

👍 thanks for fixing this.

@@ -20,20 +20,22 @@ class SharedBlockEditPanel extends Component {
constructor() {
super( ...arguments );
this.bindTitleRef = this.bindTitleRef.bind( this );
this.sharedBlockNameField = createRef();

This comment has been minimized.

@noisysocks

noisysocks Apr 18, 2018

Member

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.

This comment has been minimized.

@afercia

afercia Apr 18, 2018

Contributor

OK agree consistency is key, though that's not exactly the shared block "title" but it's how users will name the shared block ;)

@afercia afercia merged commit 0815bc4 into master Apr 18, 2018

2 checks passed

codecov/project 43.84% (-0.11%) compared to 9966a96
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@afercia afercia deleted the update/shared-block-form-managed-focus branch Apr 18, 2018

nuzzio added a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Shared block form: manage focus to avoid focus losses. (WordPress#6207)
* Shared block form: manage focus to avoid focus losses.

* Use createRef().

* Better check prevProps.

* Better naming.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment