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

Editor: Fix bug adding tags on Hebrew keyboards #2511

Merged
merged 2 commits into from
Jan 18, 2016

Conversation

nylen
Copy link
Contributor

@nylen nylen commented Jan 18, 2016

Fixes #740. The Hebrew letter "ת" is in the position of the comma key on US keyboards. As a result, pressing this letter in the editor tags control will add the currently typed tag instead of adding the letter.

Handling special keys like arrows and Enter in onKeyDown based on their keyCode works well, but for comma and other keys that result in typed characters, we should use onKeyPress and charCode to correctly determine the character that was typed.

This PR also fixes a bonus related bug: currently it is possible to add tags consisting of only whitespace by pressing the comma key.

To test

Switch your keyboard layout to Hebrew. For those unfamiliar with this process, like I was, here's a poor-quality GIF demonstrating how to do it on OS X:

keyboard-hebrew-cropped

Navigate to the post editor and activate the tags control. Type a few letters and then press the comma key to send "ת". Verify that the letter is typed instead of adding the currently typed tag.

Also, type a tag consisting of only one or more spaces, then press the comma key and verify that this does nothing instead of adding an empty tag.

The "comma" key (code 188) is a letter in Hebrew and Turkish and perhaps
other languages.  So we need to handle it by `charCode` in `onKeyPress`
instead of `keyCode` in `onKeyDown`.
@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 Jan 18, 2016
@nylen nylen self-assigned this Jan 18, 2016
@aduth
Copy link
Contributor

aduth commented Jan 18, 2016

Confirmed that this works well, including existing comma behavior for both U.S. and Hebrew keyboards (where comma is in place of ' on U.S. keyboard).

Found this to be a pretty handy tool for testing varied behavior of keyboard events: http://www.asquare.net/javascript/tests/KeyCode.html

One question: Should we expect similar conflicts with other keys with specialized behavior, such as tab or arrow keys? If so, should we move most of our handling out of onKeyDown and into onKeyPress?

If the above is not a concern, I think this is good to merge 👍

@aduth aduth 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 Jan 18, 2016
@nylen
Copy link
Contributor Author

nylen commented Jan 18, 2016

The keys you mentioned (tab and arrows) do not generate keypress events according to your test page or this similar one: http://unixpapa.com/js/testkey.html

A good rule of thumb seems to be this: for typed characters (that keyboard layouts could rearrange), use keypress; for special keys (that keyboard layouts do not rearrange, as far as I know), use keydown.

nylen added a commit that referenced this pull request Jan 18, 2016
Editor: Fix bug adding tags on Hebrew keyboards
@nylen nylen merged commit 0d32061 into master Jan 18, 2016
@nylen nylen deleted the fix/editor/tags-hebrew-comma branch January 18, 2016 17:36
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.

Bug: issue with hebrew character ת when adding a tag to a post
3 participants