Page MenuHomePhabricator

Make "/" focus the search input again
ClosedPublic

Authored by epriestley on May 27 2016, 6:03 PM.
Tags
None
Referenced Files
F14058929: D15984.id39113.diff
Sun, Nov 17, 4:04 PM
F14058927: D15984.id38479.diff
Sun, Nov 17, 4:00 PM
F14058915: D15984.id39115.diff
Sun, Nov 17, 3:53 PM
F14058888: D15984.id39114.diff
Sun, Nov 17, 3:40 PM
F14056937: D15984.id.diff
Sat, Nov 16, 11:25 PM
F14056784: D15984.diff
Sat, Nov 16, 10:26 PM
F14056758: D15984.id.diff
Sat, Nov 16, 10:14 PM
F14056708: D15984.diff
Sat, Nov 16, 9:54 PM
Subscribers
Tokens
"Love" token, awarded by avivey."Dat Boi" token, awarded by chad."Pirate Logo" token, awarded by epriestley.

Details

Summary

See D1902, T989, T11263, D15984, T4103 , D15976, https://secure.phabricator.com/w/changelog/2016.22/, T2527, T11231, T8286, T11264 for discussion!

When we get another copy of T989, I will rename it to "Build a complicated keybinding settings page like a cool video game" and leave it open forever.

Test Plan

Pressed "/" in Firefox, had my pristine browsing experience inexplicably hijacked by this horrible application.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 12988
Build 16590: Run Core Tests
Build 16589: arc lint + arc unit

Event Timeline

avivey retitled this revision from to Restore quicksearch for non-OSX users.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a reviewer: epriestley.

This patch is reasonable, but I'd like to see more interest in this before restoring it upstream, since I think the utility is marginal.

It's presumably easy to write an extension to accomplish the same thing, and from the Chrome ticket (https://bugs.chromium.org/p/chromium/issues/detail?id=31177) it sounds like you can set webkit.webprefs.tabs_to_links=false in some secret settings files to get the OSX behavior, although this will be browser-wide.

This would lock us out of using "q" for other things, and I could imagine we might want to -- maybe some keyboards don't have escape keys, and we'd want to alias q to escape? (Maybe weird mobile workflows?)

I think I'd expect this to throw a JS exception for logged-out users on non-public installs, who won't have a search box. I think the old code avoided this, although possibly by accident.

This also isn't translatable (although the old code wasn't either).

I actually consider the mac behavior to be bad, because it makes navigating via keyboard harder/impossible - I wouldn't enable it even if it is an option.

@epriestley: Do you still don't want this feature upstreamed?

It seems like a few users love this feature and a few users hate it, but the vast majority of users don't use it / care about it.

I also considered just restoring the / behavior for browsers other than Firefox, but elsewhere someone who uses Firefox wanted it back and I think we do no browser detection anywhere now (but maybe I'm just forgetting). I don't think there's a way to feature-detect this behavior.

I looked at Firefox/Mozilla's sites for evidence that they violate this behavior themselves, but couldn't find any. The Firefox default start page actually does focus search when you type / and I was sure I had a slam dunk there, but it just does that for any keystroke.

I'd also forgotten about T2527, but that was additional related mess related to this.

In PHP, I think we now generally deal with cases like this through extensions. Although I'm very hesitant to encourage users to write JS extensions, maybe that's generally a cleaner pathway forward here. There's no real technical reason this couldn't be provided by a server-side extension that emits Javascript, and then we wouldn't have to deal with the preference to turn it off or special-casing escape or a preference to special-case escape.

epriestley edited reviewers, added: avivey; removed: epriestley.
epriestley awarded a token.
epriestley edited edge metadata.
  • Only install the handler if a search box exists.
  • Translate the help strings.
epriestley retitled this revision from Restore quicksearch for non-OSX users to Make "/" focus the search input again.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
  • Flowery prose.

I also tested some other keyboard layouts but none of the ones I checked required Alt to type a "/". For example, in Hebrew the key is just the key labeled "Q" on my US keyboard, with no modifiers.

avivey edited edge metadata.
This revision is now accepted and ready to land.Jul 8 2016, 9:17 PM

(My project-reject from earlier was stuck.)

This revision was automatically updated to reflect the committed changes.