Wp-calypso: Search Component focus loss (blur) disrupts UX and breaks a11y guidance.

[getdave]

Whilst working on https://github.com/Automattic/wp-calypso/pull/42625, I noticed that the Calypso <Search> component will lose focus when the "reset" ("X") icon is clicked.

This also happens anywhere where the component is used. Read on for more!

---

If we look at the implementation of the "X" icon we can see it was probably originally intended as a "close" button rather than a "reset" button (these are conceptually different).

https://github.com/Automattic/wp-calypso/blob/de317dc6e08ea2cd38e7a7fe1d2c1b2244317785/client/components/search/index.jsx#L390-L408

Note the onClick handler closeSearch. If we look at the implementation of this we will notice that as well as being "reset" (this.searchInput.value = '';) the input is forceable _blurred_ (this.searchInput.blur();).

Specifically, note these lines:

https://github.com/Automattic/wp-calypso/blob/de317dc6e08ea2cd38e7a7fe1d2c1b2244317785/client/components/search/index.jsx#L253-L254

The consequence of this is that everytime you "clear" the search input using what looks like a typical "reset" UI, you actually completely blur the input. This can result in a total loss of focus and often results in the document receiving focus.

This is bad for UX and a11y.

I believe this happens because the search component was designed for a specific use case which probably involve toggling it's expanded/collapsed state. The reset button "collapsed" the search and the "search" magnifying glass icon "expanded" it. Presumably there was originally some kind of active focus manageable in play which would have refocused the toggle control button. However this has not be considered when the component was refactored.

Somehow the UI has evolved to be used in other scenarios where this UI toggle is not required and the collapse button has been repurposed as "reset", unfortunately without handling the focus.

This also happens in the Site search and anywhere else this component is used.

Similar issue noted at: https://github.com/Automattic/wp-calypso/issues/41508.

Steps to reproduce

1. Starting at URL: https://wordpress.com/home
2. Choose any site.
3. Click on "Switch Site" (top left).
4. Focus the search input and perform any search.
5. Click the "X" reset cross.
6. Type on your keyboard to verify that the search input is not longer focused.
7. Use the devtools console to check the actively focused element by typing document.activeElement.

What I expected

The search input to retain focus when it is "reset" using the reset icon.

What happened instead

The focus was lost and went to the document.

Browser / OS version

All.

Screenshot / Video

Context / Source

[getdave]

cc @jeryj you might be interested in this one.

[frontdevde]

As a first step, we should look into all the places this component is used in and identify whether the open/close behaviour is still in use anywhere in the codebase. This will inform our approach to fixing the issue. @hrrsppzgl Could you start using looking into this?

[getdave]

Also if this is refactored please can we have a distinct "onReset" callback prop? Let me know when you'd like testing/review as this impacts on my work and I"m happy to help. Thanks!

[cpapazoglou]

Well, it seems that the X button in the search component is used in many ways:

• Closes the Write popup - I still need to write something, that's why I clicked Write. I would expect just a reset of the search

• It resets and loses focus, when it should just reset (same as site selector)

• It resets, loses focus, and closes, as expected

Generally, I tend to believe that there are two cases:

1) Search is pinned, only magnifying glass is visible

• Clicking magnifying glass should expand the search input and focus on input
• Clicking X should reset the search and collapse the search input

2) Search is not pinned, the whole search input is visible

• Clicking on the search input should focus on itself
• Clicking X should just reset the search

Components affected:

client/blocks/author-selector/switcher-shell.jsx
client/blocks/location-search/index.jsx
client/blocks/term-tree-selector/terms.jsx
client/components/domains/register-domain-step/index.jsx
client/components/language-picker/modal.jsx
client/components/search/docs/example.jsx
client/components/search-card/index.jsx
client/components/section-nav/docs/example.jsx
client/components/site-selector/index.jsx
client/devdocs/design/playground.jsx
client/extensions/zoninator/components/search-autocomplete/index.jsx
client/my-sites/comments/comment-navigation/index.jsx
client/my-sites/media-library/filter-bar.jsx
client/my-sites/pages/main.jsx
client/my-sites/people/people-section-nav/people-search.jsx
client/my-sites/plugins/main.jsx
client/my-sites/plugins/plugins-browser/index.jsx
client/my-sites/post-selector/selector.jsx
client/my-sites/post-type-filter/index.jsx
client/my-sites/site-settings/settings-performance/main.jsx
client/my-sites/themes/themes-magic-search-card/index.jsx

I have inspected ~ 5 of them, please advise whether I should inspect everything

Actions:

1) In case that my assumption that there are only 2 cases that depend from pinned we can nest

this.searchInput.blur(); in

if ( this.props.pinned ) {
  this.openIcon.focus();
}

2) Decide what to do with the Write popup that closes unexpectedly, should it be part of an other issue / PR?

[getdave]

Great research. Thank you.

I generally find it strange that we use the "X" to provide a close action, when it is standard UX practice to have this only _reset_ the search input. The proof for this is that browsers themselves add the "X" and clicking it will reset the input. I don't think Calypso should be going against the grain here.

Personally I'd say there should be a clear distinction in the UI between the actions of:

• Close
• Reset

This should be reflected both in the component UI and also the underlying implementation and API of the component.

This may necessitate revisiting the UI in various areas of Calypso to provide a dedicated "close" button for search where we need to be able to "close" it. If this is not tenable then we need to update the API and documentation of the component to:

• make it VERY clear that the "X" means "close" and _not_ reset.
• update the implementation so that the blur action is "opt in" behaviour as it's too easy to miss and introduce horrible a11y issues. Once opt in then we'd need to update all the locations where we still require this behaviour.

I would be tempted to get Design/UX input on this from @jancavan but I'll leave it up to @frontdevde to advise.

[frontdevde]

@hrrsppzgl Thank you for taking a deeper look into how the search component is being used throughout the codebase.

From your findings we can deduce, that both the permanently open search as well as the pinned search are in use.

Our fix thus needs to ensure that

a) in the pinned state, closing the search moves the focus to the open search icon.
b) in the permanently open state, deleting the search query moves the focus back inside the search

A brief test locally seems to suggest that for b) in addition to removing the blur, we might need an intentional this.searchInput.focus(); to ensure the focus is set as we want it to. For a) it looks like the current state doesn't allow the search to be reopened by hitting Enter after closing it. I assume it should though.

Decide what to do with the Write popup that closes unexpectedly, should it be part of an other issue / PR?

Let's focus on the focus loss in this issue. If we find other issues along the way, it's better to open separate issues for those so they can be triaged separately.

Edit: fyi typed this comment prior to @getdave's comment above being visible.

[frontdevde]

This may necessitate revisiting the UI in various areas of Calypso to provide a dedicated "close" button for search where we need to be able to "close" it.

Conceptually, I wouldn't disagree. Right now the X button behaves differently based on which type of search you're using. This definitely not ideal. That said, I'm not sure folks would see this as high enough priority to warrant a more significant time investment here ( /cc @davemart-in @jancavan). For now in the context of the Dotcom Manage Janitor rotation we'll focus on solving the focus loss issue first.

If this is not tenable then we need to update the API and documentation of the component to:

make it VERY clear that the "X" means "close" and not reset.

There's two different states though, right? The X seems to mean "close" in the pinned stated and "reset" in the permanently open state.

update the implementation so that the blur action is "opt in" behaviour as it's too easy to miss and introduce horrible a11y issues. Once opt in then we'd need to update all the locations where we still require this behaviour.

The blur is required in the pinned state (the hidden input field should retain focus) and only there. We will remove the blur for the permanently open state.

[jancavan]

I generally find it strange that we use the "X" to provide a close action, when it is standard UX practice to have this only reset the search input. The proof for this is that browsers themselves add the "X" and clicking it will reset the input. I don't think Calypso should be going against the grain here.

Agreed. The "x" should only be used to clear the search field, not close it. One quick solution that comes to mind is for the lens icon to function as a toggle to open/close the search field, while we consistently use "x" to clear it. That said, I don't recommend hiding search fields behind icons. It's generally used on mobile, so I don't know why we use such pattern even on desktop where there is plenty of space.

In Posts/Pages, for example, the search field can either:

A) be in a permanently open state
B) the lens icon functions as a toggle to open/close the search field (when its open, it doesn't take over the entire SectionNav, it'll only take up perhaps less than half of it.)

@frontdevde @getdave

[jancavan]

That said, I'm not sure folks would see this as high enough priority to warrant a more significant time investment here ( /cc @davemart-in @jancavan). For now in the context of the Dotcom Manage Janitor rotation we'll focus on solving the focus loss issue first.

This is fine to handle separately, but I'll defer to @davemart-in. I would personally just do away with that SectionNav search functionality and replace it with a standard search that's, of course, styled to fit within such element. @frontdevde @getdave

[davemart-in]

For now in the context of the Dotcom Manage Janitor rotation we'll focus on solving the focus loss issue first.

+1 Thank you for making this distinction.

[davemart-in]

If we can just make sure once you click the X that the focus gets placed back on the search field rather than being lost, I think that is a good first step. I'd prefer not to swap out any components, or do a large refactor here.