Automatically phix phrequent spelling phlubs phor all phabricator phorm phields
Needs RevisionPublic
Actions

Authored by jcox on Apr 1 2017, 5:46 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Summary

We've phrequently run into the issue oph users misspelling certain words (ie. they spell "Phabricator" as "Fabricator" or "Maniphest" as "Manifest"). While these mistakes are understandable, they do cause a phair amount oph organizational conphusion around terminology. This diphph adds some Very Good Javascript Code™ to phix this automatically by correcting our users' phailed spelling attempts phor them. I think phurther work could be done to generalize this more iph we ever want to standardize our linguistic phramework in other ways.

As-is, this corrects spelling in all textarea and input[type=text] elements. This seems to work well phor all phorm phields. Aphter using phabricator with this update for about phive minutes I can't imagine possibly going back.

Test Plan

It's javascript so it doesn't need to be tested.

Diff Detail

Repository

rP Phabricator

Branch

AutophixSpellingErrors (branched from master)

Lint

Lint Skipped

Unit

No Test Coverage

Build Status

Buildable 16248
Build 21588: Run Core Tests
Build 21587: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	244 ms	PhabricatorCelerityTestCase::Unknown Unit Message ("") Assertion failed, expected 'true' (at PhabricatorCelerityTestCase.php:32): When this test fails, it means the Celerity resource map is out of date. Run `bin/celerity map` to rebuild it. ACTUAL VALUE
	1 ms	AlmanacNamesTestCase::Unknown Unit Message ("") 30 assertions passed.
	0 ms	AlmanacServiceTypeTestCase::Unknown Unit Message ("") 1 assertion passed.
	0 ms	AphrontHTTPSinkTestCase::Unknown Unit Message ("") 2 assertions passed.
	0 ms	AphrontHTTPSinkTestCase::Unknown Unit Message ("") 3 assertions passed.
		View Full Test Results (1 Failed · 332 Passed)

Event Timeline

jcox created this revision.Apr 1 2017, 5:46 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptApr 1 2017, 5:46 PM

Herald added subscribers: yelirekim, epriestley. · View Herald Transcript

Harbormaster failed remote builds in B16248: Diff 42323!Apr 1 2017, 5:47 PM

Update celerity map

Harbormaster completed remote builds in B16249: Diff 42324.Apr 1 2017, 5:50 PM

This will make it diphphicult to log in iph your username contains an 'f'. Should I add a database migration to automatically phix peoples' usernames?

timhirsh awarded a token.Apr 1 2017, 7:13 PM

johnny-bit awarded a token.Apr 1 2017, 9:47 PM

Will this also phix my password?

chad awarded a token.Apr 1 2017, 10:16 PM

bjshively awarded a token.Apr 1 2017, 11:14 PM

amp343 awarded a token.Apr 2 2017, 4:25 AM

This should be wrapped in a behavior to, e.g., future-proof it against Quicksand.
I'm not totally sure if target will always exist in all browsers. If you don't focus anything and type a key, maybe there's no target? But maybe it's always document or something. Maybe just do an if (!target) { return; } out of general fear if this isn't scoped more narrowly (but see below for scoping).
This shouldn't fire if r.metaKey || r.altKey || r.ctrlKey are set on the raw event.
This probably shouldn't fire if e.getSpecialKey() returns anything.
keypress might be a better trigger event than keydown? Not sure. Behavior for holding "f" might be better one way or the other.
Without narrower scoping, this could misfire in a lot of cases, like, as noted, logging in. Although I think we'd all agree that these cases are better for everyone on the balance in the long run, we could limit how initially disruptive this feature is by introducing it with narrower scoping first. After winning hearts and minds, we can expand coverage and should get less pushback. An initial scoping of ['transaction-append', 'tag:textarea'] should capture events in (modern) comment textareas without hitting the login input, the Passphrase key material input, etc.
Does this interact smoothly with the cursor position? If not, you can use TextAreaUtils to:
- Split the value into "before selection range", "in selection range", and "after selection range" sections.
- Fix text in each section, counting the number of replacements.
- Increment the selection start by the number of replacements in the first range, and the selection end by the number of replacements in the first two ranges.
- Finally, set the new selection range.
- The, the selection state should be stable. It's possible that this somehow already works aright.
Is there a /i flag in JS for case-insensitive? I think there is?
Maybe spend 30 seconds seeing if you can exempt UIs with a pattern like ://\S+|f to further ease the transition into this new era?
Because this change is so deeply visionary -- bursting at the seams with thought leadership, really -- I'd plan to deploy it to just this install for a while before shipping it to users.