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

Components: Prevent <Search> from stealing the focus when typing elsewhere #1450

Merged
merged 1 commit into from
Dec 10, 2015

Conversation

nylen
Copy link
Contributor

@nylen nylen commented Dec 9, 2015

This fixes #1389 where if the location field has user-entered text, it steals the focus back when typing into another text field:

location-steals-focus

I'm not seeing a good reason why we would want the <Search> component to focus itself if it has text: in any such situation, shouldn't it already be focused?

This code or some variation of it has been present since long before OSS release. As far as I can tell, these are the conditions needed for the bug to surface:

  • A <Search> and another text field both present on the same screen
  • A structure that causes componentDidUpdate to be called when the other text field changes (in the editor, this is the case, but I'm not entirely sure why)

@aduth @mtias I saw your names a lot in the history of this component, want to take a look?

@nylen nylen added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Dec 9, 2015
@nylen nylen self-assigned this Dec 9, 2015
@rralian
Copy link
Contributor

rralian commented Dec 10, 2015

I'm not positive, but I think this had to do with search pages that append the search query as a url parameter (e.g., posts: /posts/example.wordpress.com?s=test). What happens on those pages is that we update the url, which triggers the controller, which then passes the parameter back into the search box. I think at the time that process must have caused the search field to lose focus. It's not doing so now though from my testing. So I'm not aware of any reason we need this. Would welcome a second opinion though.

@mtias
Copy link
Member

mtias commented Dec 10, 2015

It's not doing so now though from my testing.

In my testing, loading a page with the s=test query doesn't focus the search input, not in this PR but neither in master.

@mtias
Copy link
Member

mtias commented Dec 10, 2015

@nylen this seems like a good change, I can't figure out why we had that piece of code, it doesn't seem to be doing anything productive, and simpler code is always welcome :)

If someone wanted to focus the main search in a page based on the query string it seems to me they should be explicit about it and set the autoFocus prop based on the query string.

@rralian
Copy link
Contributor

rralian commented Dec 10, 2015

In my testing, loading a page with the s=test query doesn't focus the search input, not in this PR but neither in master.

I'm not positive that's functionally equivalent to typing in the text field. Anyway, I can't account for a reason for this code right now and so I support removing it. This code became really complex, so could use some simplification and refactoring in general. For example, it aint great that we aren't sure why/whether this clause was ever needed.

@mtias mtias added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 10, 2015
@nylen
Copy link
Contributor Author

nylen commented Dec 10, 2015

If someone wanted to focus the main search in a page based on the query string it seems to me they should be explicit about it and set the autoFocus prop based on the query string.

Agreed

it aint great that we aren't sure why/whether this clause was ever needed

and agreed :0

nylen added a commit that referenced this pull request Dec 10, 2015
Components: Prevent <Search> from stealing the focus when typing elsewhere
@nylen nylen merged commit 920a0fe into master Dec 10, 2015
@nylen nylen deleted the fix/editor/1389 branch December 10, 2015 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor: location field steals focus
4 participants