-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Replaced deprecated keyCode functionality and docs with KeyboardEvent.code & KeyboardEvent.key also updates the keyIsDown function to accept alphanumerics as parameters #7472
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on, this is a great start!
I think we might need to keep track of two downkey objects, one for code
s and one for key
s, in order to fully support querying of both.
/** | ||
* @typedef {18} ALT | ||
* @typedef {'AltLeft' | 'AltRight'} ALT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the typedef is both of these while the constant is just one?
src/events/keyboard.js
Outdated
console.log('Current key:', this.key); | ||
|
||
// For backward compatibility - if code is a number | ||
if (typeof code === 'number') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work if our key event handlers only set _downkeys[e.code]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @limzykenneth do you think we need backwards compatibility? We might need to put a lookup table like this https://stackoverflow.com/a/66180965 into our code to map old key codes to keys, although it looks like it's hard to get complete backwards compatibility. That's probably enough though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the numerical code has been deprecated as a standard, we can remove it since its the point of this implementation to use the easier code
property instead.
src/events/keyboard.js
Outdated
if (code.length === 1) { | ||
if (/[A-Za-z]/.test(code)) { | ||
// For letters, we need to check the actual key value | ||
return this.key === code; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this only checks if the last pressed key was the one passed in, rather than if that key is currently down. We might need to make two objects, _downKeyCodes
and _downKeys
, and update both on keydown/keyup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, got it
src/events/keyboard.js
Outdated
// For letters, we need to check the actual key value | ||
return this.key === code; | ||
} else if (/[0-9]/.test(code)) { | ||
return this._downKeys[`Digit${code}`] || false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably be consistent and check for keys if code.length === 1
and check for key codes otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we are currently checking with key
if code.length === 1
. and checking for code
otherwise. I'm assuming that we have to completely replace the usage the keyCode ig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully replacing event.keyCode
is correct! I think ideally we could tell users that if they pass in a code (e.g. 'Digits3'
), then it will check if that physical key is pressed, and if the user passes in a character (e.g. '3'
), it checks whether whatever key produces that character is pressed. I think that would mean we have to detect whether they passed in an event.key
or an event.code
, and then handling each branch separately.
The slight inconsistency right now is if the user passes in the string '3'
, it is checking for the physical key via the code
still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, got it.
I think this logic might work, for allowing the user to enter both key and code and seperate classification of them
fn.keyIsDown = function(input){
if (typeof input ==='string'){
if (input.length === 1) {
return this._downKeys[input] ||
(/[0-9]/.test(input) && this._downKeyCodes[`Digit${input}`])
|| false;
}
return this._downKeyCodes[input] || this._downKeys[input] || false;
}
return false;
};
here _downKeyCodes is storing e.code and _downKeys is storing e.key.
now if user passes 'a'
or '3'
, this will be checked by key and returned true,
and if passes 'Digit3'
, this will be checked by code and returned true. and for special keys such as Enter, ' ', Shift, ShiftLeft, this will give true as they are getting checked for key or code.
Hence, user can use either key or code for the keyisDown function,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! It might makes sense to make a isCode()
function that we can call, so you can do:
fn.keyIsDown = function(input) {
if (isCode(input)) {
return this._downCodes[input];
} else {
return this._downKeys[input];
}
}
...and possibly also unit test isCode
separately too.
src/events/keyboard.js
Outdated
// For string inputs (new functionality) | ||
if (typeof code === 'string') { | ||
// Handle single character inputs | ||
if (code.length === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also add an else if branch to handle the multi-character possibilities for key
, for things like Enter and the arrow keys: https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values If it's one of those, we would need to check against down keys as opposed to down key codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some overlap between key
and code
, we'll need to decide which to prefer in case of overlap I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, we are using key
only for the single characters(because of same code for characters i.e. keyA for 'a' and 'A') and for rest of the inputs we are using code
src/events/keyboard.js
Outdated
@@ -97,6 +97,8 @@ function keyboard(p5, fn){ | |||
*/ | |||
fn.isKeyPressed = false; | |||
fn.keyIsPressed = false; // khan | |||
fn._code = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only used in this module and not used by other modules through the instance, it should be a local variable and not attached to fn
if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, currently user is able to do
text(key, 50, 50)
giving the key
of the key pressed.
Maybe the same can be done for code
with added documentation.
src/events/keyboard.js
Outdated
fn.keyIsDown = function(code) { | ||
// p5._validateParameters('keyIsDown', arguments); | ||
return this._downKeys[code] || false; | ||
p5.prototype.keyIsDown = function(code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be fn.keyIsDown
and the indentation fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is almost there! I left a few things to clean up, and then as a last thing we can delete the keyCode
property from here:
Line 292 in 9cfef5a
fn.keyCode = 0; |
* @property {CONTROL} CONTROL | ||
* @final | ||
*/ | ||
export const CONTROL = 17; | ||
export const CONTROL = 'ControlLeft'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each constant can only be defined as one thing, so we might need to make two constants instead of a single one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davepagurek So, I'm not entirely sure what to do for this.
Should I make two constants one for key and code, like...
export const CONTROL_CODE = 'Control';
export const CONTROL_KEY = 'ControlLeft';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking originally having a const CONTROL_LEFT
and CONTROL_RIGHT
, but on second thought, the use case before of having a single CONTROL
implies that it doesn't matter which. So it probably makes most sense to just export const CONTROL = 'Control
so that it ends up using the key
instead of the keyCode
so that it would check for both.
I think this might mean we need to update isCode
so that isCode('Control')
returns false though. And for everything we do for Control
, we would also do for Alt
and Shift
, since they have a left/right version too.
hello @davepagurek , there has been a commit in the dev-2.0 branch for using meta key. That has been made assuming the usage of keycode. In that implementation, a new object of |
That's fine as long as the original issue is resolved! Try using the instructions in this comment #7282 (comment) to see what the original issue was, and then if that works on your branch already, we can take out the special casing that was added in main. |
Yeah, its working. In the issue #7282, you tested for event.metaKey && event.keyCode == 90, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the merge, it's great that we can simplify the meta key situation with your code!
* @property {CONTROL} CONTROL | ||
* @final | ||
*/ | ||
export const CONTROL = 17; | ||
export const CONTROL = 'ControlLeft'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking originally having a const CONTROL_LEFT
and CONTROL_RIGHT
, but on second thought, the use case before of having a single CONTROL
implies that it doesn't matter which. So it probably makes most sense to just export const CONTROL = 'Control
so that it ends up using the key
instead of the keyCode
so that it would check for both.
I think this might mean we need to update isCode
so that isCode('Control')
returns false though. And for everything we do for Control
, we would also do for Alt
and Shift
, since they have a left/right version too.
src/events/keyboard.js
Outdated
@@ -916,6 +925,15 @@ function keyboard(p5, fn){ | |||
* </div> | |||
*/ | |||
function isCode(input) { | |||
const leftRightKeys = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that we're defining isCode
twice -- I think we can remove this one, and just use the export
ed one above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, sure.
test/unit/events/keyboard.js
Outdated
@@ -189,6 +189,11 @@ suite('Keyboard Events', function() { | |||
assert.isTrue(isCode('Control')); | |||
assert.isTrue(isCode('ab')); | |||
}); | |||
|
|||
test('returns false for strings for letright keys', function() { | |||
assert.isFalse(isCode('AltLeft')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want AltLeft
to return true actually, and just Alt
to return false. I'm testing out the demo on MDN here: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code#try_it_out
Hitting the alt key, it outputs: KeyboardEvent: key='Alt' | code='AltLeft'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed this, returning false for keys and true for codes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience making all these changes, great work! I think we're good to go!
@all-contributors please add @Vaivaswat2244 for code |
@Vaivaswat2244 already contributed before to code |
Resolves #7436 and #6798
Changes:
This PR modernizes keyboard event handling by transitioning from the deprecated
keyCode
property to the modernKeyboardEvent.code
andKeyboardEvent.key
properties. This change improves keyboard input reliability and brings p5.js in line with current web standards.KeyboardEvent.code
instead ofe.which
_code
propertyconstants
to use modern string valuesScreenshots of the change:
/preview/index.html test sketch is:
PR Checklist
npm run lint
passes