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

Update jquery.hotkeys.js #54

Merged
merged 1 commit into from
Jan 13, 2015
Merged

Update jquery.hotkeys.js #54

merged 1 commit into from
Jan 13, 2015

Conversation

abolishme
Copy link

should also escape text-accepting inputs like search fields unless explicitly bound

should also escape text-accepting inputs like search fields unless explicitly bound
@rpocklin
Copy link
Collaborator

rpocklin commented Jan 7, 2015

I've reviewed the history in these lines in the original source and it has been that way almost forever.

I have before asked myself why it doesn't exclude input by default (unless specifically bound), since there are a bunch of specific input types we avoid (eg. input[tel] input[text]).

My only rationale is that currently, people can customise this via textAcceptingInputTypes and to hard-wire it to ignore all inputs unless specifically bound may not be everyone's cup of tea.

Can you give me your specific use case where your change will fix an issue?

I'm quite open to this PR, btw.

@abolishme
Copy link
Author

use case: i have the spacebar bound, i have an input[search] that filters out rows of a table based on the input, i want to be able to type a word space while typing in the input without triggering the bound behavior.

reasoning: i was about to escape binding in just input[search], i thought for a moment, i couldn't imagine a single instance where i would want bound behavior by default in any input element.

@rpocklin
Copy link
Collaborator

Great explanation, thanks. I'm all for accepting this, it will actually simplify the code, but is a breaking change. I'll extend this PR and make the ignored input types configurable.

Thanks again!

@abolishme
Copy link
Author

thank you!

rpocklin added a commit that referenced this pull request Jan 13, 2015
Adding `input` type to be unbound unless explicitly specified.
@rpocklin rpocklin merged commit 46b23fb into jeresig:master Jan 13, 2015
@abolishme abolishme deleted the patch-1 branch January 13, 2015 10:34
@pbodnar
Copy link

pbodnar commented Mar 26, 2017

Hi there, correct me if I'm wrong, but IMHO the original code (in tag 0.2.0) was pretty straightforward, although the misleading comment line should have been fixed (as for example 'search' IS actually included in the checks):

// excludes: button, checkbox, file, hidden, image, password, radio, reset, search, submit, url
textAcceptingInputTypes: [
  "text", "password", "number", "email", "url", "range", "date", "month", "week", "time", "datetime",
  "datetime-local", "search", "color", "tel"],

reasoning: i was about to escape binding in just input[search], i thought for a moment, i couldn't imagine a single instance where i would want bound behavior by default in any input element.

Think about buttons or radio buttons for example - do we really want NOT to have a shortcut active when a radio button has got focus (as introduced by this pull request)? And make this the default behavior?

Let me know if I missed something, but I think the changes done should be re-considered (or documented at minimum, right in the code). Until then, I will better stick with the tagged version of this handy plugin...

Regards, Petr

@rpocklin
Copy link
Collaborator

Happy to have a counter-PR if it makes sense and re-visit the defaults, esp. for non-text input fields.

pbodnar added a commit to pbodnar/jquery.hotkeys that referenced this pull request May 1, 2017
This is de-facto a revert of the pull request "jeresig#54 from
abolishme/patch-1" and follow-up commits, while also clarifying how the
original code was meant and bringing back some "reasonable defaults".

Plus fixing related (structure) typos in the README file.
@pbodnar
Copy link

pbodnar commented May 1, 2017

Done, PR #61, looking forward to your review... :)

@rpocklin
Copy link
Collaborator

rpocklin commented May 22, 2017

@pbodnar I re-read all this. I don't see why @abolishme couldn't just override textAcceptingInputTypes to get the desired behaviour. A spacebar is a quirky hotkey to bind.

@pbodnar
Copy link

pbodnar commented Jun 7, 2017

@rpocklin, I've re-read it too and I agree with you on this...

rpocklin pushed a commit that referenced this pull request Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants