Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

select: doesn't autofill correctly when pressing period character #11294

Closed
geertjansen opened this issue May 23, 2018 · 4 comments · Fixed by #11313
Closed

select: doesn't autofill correctly when pressing period character #11294

geertjansen opened this issue May 23, 2018 · 4 comments · Fixed by #11313
Assignees
Labels
for: internal contributor The team will address this issue and community PRs are not requested. has: Pull Request A PR has been created to address this issue P3: important Important issues that really should be fixed when possible. resolution: fixed type: bug
Milestone

Comments

@geertjansen
Copy link

geertjansen commented May 23, 2018

Bug, enhancement request, or proposal:

Bug

CodePen and steps to reproduce the issue:

CodePen Demo which demonstrates the issue:

https://codepen.io/geertjansen/pen/QrPoVa

Detailed Reproduction Steps:

  1. Focus in on the MdSelect
  2. Start typing "255.2"

What is the expected behavior?

  1. The select should focus to the mdOption containing the first match which should be "255.255.0.0"

What is the current behavior?

  1. Instead the option "255¾255¾0¾0" is targeted

What is the use-case or motivation for changing an existing behavior?

This is not how the select autofill is supposed to work.

Which versions of AngularJS, Material, OS, and browsers are affected?

  • AngularJS: 1.6.9
  • AngularJS Material: 1.1.8
  • OS: Mac OS X 10.12.6
  • Browsers: Chrome 66.0.3359.181

Is there anything else we should know? Stack Traces, Screenshots, etc.

The problem lies in the fact the MdSelect listens for the keyCode of the keydown event which is then used as a parameter for the String.fromCharCode method which returns "¾" for the keyCode 190 instead of a "." character.

@Splaktar Splaktar self-assigned this May 26, 2018
@Splaktar Splaktar added needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community type: bug labels May 26, 2018
@Splaktar
Copy link
Member

Hrm, I tested on Chrome and Firefox and the md-select did not change focus at all no matter what I typed. I'm on macOS 10.13.4.

I updated the CodePen to 1.1.9 and added a 0.0.0.0 option. I do see the md-select change focus when I type 0 in this case.

I added another value of 01.0.0.0 and when I type 01 in the md-select, the second character doesn't seem to match. But if I then type 2, it will match the first entry with 255. So it seems like it only supports matching on the first character.

Let me look into the code a bit more...

@Splaktar
Copy link
Member

Splaktar commented May 26, 2018

OK, for the keyboard search on md-select there is a 300ms timeout. Keys typed quickly within that timeout get used together for a search. After that timeout, the search is reset and a new key starts over a new search.

Typing quickly, I'm able to get to 01 but not 255.2 (stays on first element).

The root cause appears to be a bug caused by the use of keydown events which aren't suitable to use with String.fromCharCode according to https://stackoverflow.com/questions/18238523/string-fromcharcodee-which-doesnt-recognize-dot-and-comma. I've tested this and using keypress event instead does give a valid code point that translates to the . char.

@Splaktar Splaktar added P3: important Important issues that really should be fixed when possible. needs: Pull Request needs: unit tests This PR needs unit tests to cover the changes being proposed and removed needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community labels May 26, 2018
@Splaktar Splaktar added this to the 1.1.10 milestone May 26, 2018
@Splaktar Splaktar changed the title md-select doesn't autofill correctly when pressing period character select: doesn't autofill correctly when pressing period character May 26, 2018
@Splaktar
Copy link
Member

I managed to reproduce this in a unit test and fix that test in 1535ce5, but this causes about 48 other tests to break. It's going to need a good deal more time and effort invested to refine this solution.

If someone else wants to pick it up from here and provide a PR, please feel free. I need to focus on some higher priority issues.

@Splaktar Splaktar added for: external contributor for: internal contributor The team will address this issue and community PRs are not requested. labels May 26, 2018
@feloy
Copy link
Contributor

feloy commented Jun 3, 2018

Hi @Splaktar , I'll try with the use of e.key instead of e.charCode, as exlpained in this page for example: https://developer.mozilla.org/fr/docs/Web/Events/keypress

feloy added a commit to feloy/material that referenced this issue Jun 4, 2018
feloy added a commit to feloy/material that referenced this issue Jun 4, 2018
@Splaktar Splaktar added has: Pull Request A PR has been created to address this issue and removed needs: Pull Request needs: unit tests This PR needs unit tests to cover the changes being proposed labels Jun 6, 2018
jelbourn pushed a commit that referenced this issue Jun 22, 2018
Splaktar pushed a commit that referenced this issue Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
for: internal contributor The team will address this issue and community PRs are not requested. has: Pull Request A PR has been created to address this issue P3: important Important issues that really should be fixed when possible. resolution: fixed type: bug
Projects
None yet
3 participants