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

Inserter: Autofocus the Search input #1187

Merged
merged 7 commits into from
Jun 27, 2017

Conversation

oskosk
Copy link
Contributor

@oskosk oskosk commented Jun 15, 2017

Makes the inserter menu autofocus on the input search. We can autofocus if the search input is on the top as the first focusable item.

Related to #1064

Testing instructions

  1. Run this branch
  2. Click the "Insert" button
  3. Expect the focus to be set on the search input.

@oskosk oskosk added [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. [Feature] Inserter The main way to insert blocks using the + button in the editing interface and removed [Status] In Progress Tracking issues with work in progress labels Jun 15, 2017
@oskosk oskosk requested a review from ellatrix June 15, 2017 14:02
@oskosk
Copy link
Contributor Author

oskosk commented Jun 15, 2017

cc @mtias and @jasmussen for review

@youknowriad
Copy link
Contributor

This works great for me but I wondering if there're accessibility issues with this.

Not totally related: We should select the first block automatically and allow "type type + enter" to add the first selected block.

@nb
Copy link
Member

nb commented Jun 15, 2017

We chatted with @afercia again today and we can autofocus if we move the search input to the top. If you think that will be a better experience, feel free to change to that, otherwise 👍 from me.

@oskosk oskosk force-pushed the add/focus-input-search-on-alpha-keydown branch from 6e331af to eda4e9e Compare June 16, 2017 13:02
@aduth
Copy link
Member

aduth commented Jun 16, 2017

With the search input moved, we may also need to update this logic, which assumed that the search input would appear at the end of the inserter when keyboarding through the set:

https://github.com/WordPress/gutenberg/blob/19ecc2c/editor/inserter/menu.js#L92-L93

@youknowriad
Copy link
Contributor

We chatted with @afercia again today and we can autofocus

Should we change the logic here and just use the autoFocus prop instead of detecting alpha keys?

@oskosk
Copy link
Contributor Author

oskosk commented Jun 16, 2017

@aduth indeed. Will update this. Thanks!

@youknowriad I think you're right. Autofocus was accepted if we moved the search input to the top

@mtias
Copy link
Member

mtias commented Jun 22, 2017

@oskosk can you rebase this so there are no conflicts?

@oskosk oskosk force-pushed the add/focus-input-search-on-alpha-keydown branch from 19ecc2c to f8bfc1e Compare June 22, 2017 13:07
This allows us to be able to focus the search input when the user starts
typing while following accexxibility recommendations.
@oskosk oskosk force-pushed the add/focus-input-search-on-alpha-keydown branch from f8bfc1e to 14c36e5 Compare June 22, 2017 13:52
@oskosk oskosk changed the title Inserter: Focus the Inserter Menu search input when pressing an alpha key Inserter: Autofocus the Search input Jun 22, 2017
@oskosk
Copy link
Contributor Author

oskosk commented Jun 22, 2017

I've updated this PR (code, description and title) to just introduce the autoFocus prop on the search input and move it to the top of the inserter.

Also removed the onKeydown behaviour for focusing the search input when a letter key is pressed. This turned out to be an unexpected behaviour in terms of accessible menu navigation. If pressing letter keys is to be implemented as a control for navigation it's expected that pressing the key, will focus the first item with text starting with that letter as detailed by @afercia's research on menu navigation.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Such a nice improvement. Ship it 👍

@afercia
Copy link
Contributor

afercia commented Jun 27, 2017

@youknowriad not sure if this is still needed onClick={ this.setSearchFocus } ?

@oskosk
Copy link
Contributor Author

oskosk commented Jun 27, 2017

@afercia calling setSearchFocus updates a currentFocus var in the component's state which is used for later sequential navigation among blocks and the search input.

@oskosk
Copy link
Contributor Author

oskosk commented Jun 27, 2017

@afercia thanks to your comment I ended up realizing that this state variable was being initialized as null instead of "search" which probably makes more sense since we're autofocusing it now. I've updated this PR to do this

@afercia
Copy link
Contributor

afercia commented Jun 27, 2017

Thanks @oskosk I see now changeMenuSelection() not only sets the focus but also updates the state.

@oskosk
Copy link
Contributor Author

oskosk commented Jun 27, 2017

@afercia are you OK with merging this and handling the observations in #1378 afterwards ?

@afercia
Copy link
Contributor

afercia commented Jun 27, 2017

@oskosk Sure, I was just asking a question, not arguing anything.

@oskosk
Copy link
Contributor Author

oskosk commented Jun 27, 2017

Oh, didn't parse it as arguing. I just wanted to bring the observations from #1378 into this convo so you knew I'm aware of those :P

@oskosk oskosk merged commit b1146ce into master Jun 27, 2017
@oskosk oskosk deleted the add/focus-input-search-on-alpha-keydown branch June 27, 2017 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants