fix(select): Allow non-english characters for keyboard selection. #8893

topherfangio · 2016-06-28T21:04:40Z

Our previous method for checking whether or not the typed character matched the first letter of an <md-option> was constrained to the letters [a-zA-Z].

Fix by allowing more characters and checking the validity via the String.fromCharCode() method.

Fixes #7730.

topherfangio · 2016-06-28T21:11:19Z

@ThomasBurleson Not sure who would be best to review this besides yourself.

clshortfuse · 2016-07-01T00:20:34Z

Your PR improves the interaction, and should probably go through, but I'm going to be honest, I'm not sure how I feel about keyCode.

We're translating keyCode to map to character, but I've seen bad results with different operating systems and browsers.

keyCode looks deprecated as well, unless I'm reading it wrong: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

Also, á is a Latin character, and still part of ASCII. I believe it's 160. You should put a test for Unicode.

topherfangio · 2016-07-01T01:31:08Z

I looked at switching it out from using keyCode, but the replacement is supposed to be KeyboardEvent.key which as far as I can tell from the Browser Compatibility list is extremely unsupported at the moment (it has 0 support from Safari and only became available in Chrome 51...the current version).

I'm certainly open to changing it if we can find a better approach, but I'm honestly thinking we might just wait until .key becomes more widely supported and replace it then.

clshortfuse · 2016-07-01T01:39:48Z

Darn Safari. We can use keyCode as a fallback. To be honest, keyboard input parsing is very messy. I've had to write a while service/module in the past to deal with getting the right keys. If Unicode works with keyCode, we can backlog moving to ‘key‘. I still would want to do it, since I don't like the idea of swallowing inputs. For example F11 would trigger Fullscreen and Ctrl+ would zoom in. I don't want those actions to be unavailable when somebody triggersa popup

topherfangio · 2016-07-01T02:03:52Z

I added a unicode test. We could add a bit more logic (inside a function) which filters out some of the known ones like the function keys.

I didn't think about that. I'll go ahead an work on that and see if I can add some more tests.

clshortfuse · 2016-07-01T03:03:40Z

You can also just only stopPropagation if a match is found. I can't think of an instance where that would have a negative effect.

clshortfuse · 2016-07-05T19:15:31Z

src/components/select/select.js

+  var isNonUsefulKey = (ev.keyCode <= 31);
+  var isLeftRightSelectKey = (ev.keyCode >= 91 && ev.keyCode <= 93);
+  var isFnLockKey = (ev.keyCode >= 112 && ev.keyCode <= 145);
+


I think it's safe to abort if the user press Ctrl or Meta or Alt. I say this because master locked me out from refreshing the page with Ctrl+Shift+R.

Shift alone should be okay. I tried with with AltGr and special Latin characters (ñÑéóö) and Alt is true while holding AltGr but gets set to false when I press AltGr+N. ñ is detected as it's own key with Alt set to false, so it looks safe.

I'm really confused by what you mean here 😄

You're saying we should also test for Ctrl, Meta or Alt and return false too?

Yes return false. So users can refresh (Ctrl+R) or ctrl/alt tab while it's open

ThomasBurleson · 2017-01-01T18:43:45Z

@topherfangio - please confirm this PR is valid and still needed. Then please rebase. Thank you.

topherfangio · 2017-01-04T21:51:47Z

@ThomasBurleson Rebased and ready for a final review.

I do believe it is relevant because the old functionality was

Allow only certain keys

whereas the new approach says

Filter known bad keys, allow everything else

In particular, this means that non-english characters are allowed.

clshortfuse

Looks a lot cleaner than I remember! Just needs one more minor fix.

clshortfuse · 2017-01-19T01:33:17Z

src/components/select/select.js

+function shouldHandleKey(ev, $mdConstant) {
+  var char = String.fromCharCode(ev.keyCode);
+  var isNonUsefulKey = (ev.keyCode <= 31);
+


We need a check to make sure the user isn't performing a keyboard combination. For example, we don't want to stop users from being able to close a window (Ctrl+W) or stop a Refresh (Ctrl+R) simply because one of the combination keys is valid. Basically, we should abort if Alt, Ctrl, or Meta is being held.

This isn't the same as the isMetaKey function, which is only checking for single key presses of meta keys.

Our previous method for checking whether or not the typed character matched the first letter of an `<md-option>` was constrained to the letters `[a-zA-Z]`. Fix by allowing more characters and checking the validity via the `String.fromCharCode()` method. Add tests for Spanish and Simplified Chinese, as well as not swallowing certain keys like function keys (F3), meta, ctrl and comma (`,`). Fixes angular#7730.

clshortfuse

LGTM!

topherfangio added the needs: review This PR is waiting on review from the team label Jun 28, 2016

topherfangio force-pushed the fix-select-english-only-7730 branch from 42e6a7a to bea3979 Compare July 1, 2016 02:00

topherfangio added needs: work and removed needs: review This PR is waiting on review from the team labels Jul 1, 2016

topherfangio force-pushed the fix-select-english-only-7730 branch from bea3979 to 6269dc5 Compare July 4, 2016 16:19

clshortfuse reviewed Jul 5, 2016
View reviewed changes

ThomasBurleson added the needs: group review label Sep 7, 2016

topherfangio self-assigned this Sep 9, 2016

topherfangio removed needs: group review labels Sep 9, 2016

clshortfuse mentioned this pull request Oct 13, 2016

mdSelect: Focus state swallows command+keypress, preventing command+R and some other shortcuts #9717

Closed

ThomasBurleson added needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved and removed needs: work labels Jan 1, 2017

ThomasBurleson modified the milestones: 1.1.3, 1.2.0 Jan 1, 2017

topherfangio force-pushed the fix-select-english-only-7730 branch from 6269dc5 to b6dfd1a Compare January 4, 2017 20:23

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Jan 4, 2017

topherfangio added needs: review This PR is waiting on review from the team and removed needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Jan 4, 2017

topherfangio force-pushed the fix-select-english-only-7730 branch 2 times, most recently from 620abd7 to cb6d563 Compare January 4, 2017 21:19

clshortfuse suggested changes Jan 19, 2017

View reviewed changes

topherfangio force-pushed the fix-select-english-only-7730 branch from cb6d563 to 969c5ba Compare January 26, 2017 22:33

clshortfuse approved these changes Jan 26, 2017

View reviewed changes

topherfangio added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Jan 27, 2017

mmalerba merged commit 89538d6 into angular:master Feb 23, 2017

devversion mentioned this pull request Mar 7, 2017

fix(select): Fix command+keypress being intercepted during focus #9718

Closed

Splaktar modified the milestones: 1.2.0, 1.1.6 Nov 18, 2017

Splaktar modified the milestones: 1.1.6, 1.1.4 Jul 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(select): Allow non-english characters for keyboard selection. #8893

fix(select): Allow non-english characters for keyboard selection. #8893

Uh oh!

topherfangio commented Jun 28, 2016

Uh oh!

topherfangio commented Jun 28, 2016

Uh oh!

clshortfuse commented Jul 1, 2016

Uh oh!

topherfangio commented Jul 1, 2016

Uh oh!

clshortfuse commented Jul 1, 2016

Uh oh!

topherfangio commented Jul 1, 2016

Uh oh!

clshortfuse commented Jul 1, 2016

Uh oh!

clshortfuse Jul 5, 2016

Uh oh!

topherfangio Jul 10, 2016

Uh oh!

clshortfuse Jul 10, 2016

Uh oh!

ThomasBurleson commented Jan 1, 2017

Uh oh!

topherfangio commented Jan 4, 2017

Uh oh!

clshortfuse left a comment

Uh oh!

clshortfuse Jan 19, 2017

Uh oh!

clshortfuse left a comment

Uh oh!

Uh oh!

fix(select): Allow non-english characters for keyboard selection. #8893

fix(select): Allow non-english characters for keyboard selection. #8893

Uh oh!

Conversation

topherfangio commented Jun 28, 2016

Uh oh!

topherfangio commented Jun 28, 2016

Uh oh!

clshortfuse commented Jul 1, 2016

Uh oh!

topherfangio commented Jul 1, 2016

Uh oh!

clshortfuse commented Jul 1, 2016

Uh oh!

topherfangio commented Jul 1, 2016

Uh oh!

clshortfuse commented Jul 1, 2016

Uh oh!

clshortfuse Jul 5, 2016

Choose a reason for hiding this comment

Uh oh!

topherfangio Jul 10, 2016

Choose a reason for hiding this comment

Uh oh!

clshortfuse Jul 10, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasBurleson commented Jan 1, 2017

Uh oh!

topherfangio commented Jan 4, 2017

Uh oh!

clshortfuse left a comment

Choose a reason for hiding this comment

Uh oh!

clshortfuse Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

clshortfuse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!