[JIMT][SL-566] - calls _onblur when creating p5 obj - #51377
Conversation
thomasoniii
changed the title
mikeharv
left a comment
mikeharv
left a comment
There was a problem hiding this comment.
This change looks very safe and fixes the bug that is impacting students. I looked into _onblur and found it's very simple:
/**
* The onblur function is called when the user is no longer focused
* on the p5 element. Because the keyup events will not fire if the user is
* not focused on the element we must assume all keys currently down have
* been released.
*/
p5.prototype._onblur = function (e) {
downKeys = {};
};
It seems very reasonable that we'd want to assume all keys currently down have been released when a program starts, so this feels like a good fit. One could maybe argue that the name of the function isn't a perfect fit for our use case, but considering that a rename would require updating both repos and doing a version bump, that seems out of scope.
I recommend we move forward with this change.
bencodeorg
left a comment
bencodeorg
left a comment
There was a problem hiding this comment.
🎉 Nice work Jim! Makes me a little nervous to use undocumented function every time we execute (ie, could this regress if we bump p5 versions?), but I guess it would be pretty obvious if it did, and we do use undocumented p5 functions elsewhere, so not a first. I think it's worth adding a comment as to why this is here before merging.
|
Two questions for my understanding:
|
The current version of p5 still has an equivalent function (though it does something slightly different since the I also added a comment just explaining what it does, and of course the |
Pretty much. I was first looking at the
I'd added a few breakpoints to try and narrow down where the actual keydown event handler was and walked back up the stack that way. The |
thomasoniii
changed the title
| new window.p5( | ||
| function (p5obj) { | ||
| this.p5 = p5obj; | ||
| this.p5._onblur(); // This is to explicitly wipe out the downKeys object over in p5.js so no old keys hang around. |
There was a problem hiding this comment.
nit: this comment should go above the function call, to match the styling in the rest of the file (and is our general practice, too).
4 participants
Summary
This is a proposed fix, since it'll resolve it, but I'm hoping people more knowledgeable with the system will take a look at it and comment as to whether it's viable or if it points to a deeper issue. It feels like a bandaid.
If you're in sprite lab/p5 lab in the cdc mapping landmarks module, at least, you can make the character fly off the screen.
Repro is just to be holding down an arrow key when the timer resets. When you re-start the app, the character will fly off the edge in whatever direction you had previously pressed. However, if you re-press the same key, then motion will stop.
While investigating, I found additional ways to repro - the simplest one is to put a breakpoint in
p5.prototype._onkeydown, and run with the debugger. You don't need to do anything, just let the debugger pause and immediately resume and the issue will reproduce.The root cause is that inside of
p5, it's storing a global object calleddownKeyskeeping track of whatever keys are currently pressed. This is completely decoupled from thep5object itself.When a user presses a key, it fires off
_onkeydownand sticks the key into that global object. BUT - if the key is still being held down when execution stops, the p5 object is promptly destroyed. That means that when the user finally lets go of the key, thep5object no longer exists and cannot fire the_onkeyupaction that clears out the global object. Hence it sticks around in that global object and re-appears later.Similar things happen in the debugger - if you hop out via this, you're naturally going to let go of the key. So you resume execution, the
_onkeyupnever fires to clear it, and your character keeps running forever.So. How to clear it? Fortunately,
p5provides an_onblurevent, which clears out thedownKeysobject. The comments claim it fires when the user is no longer focused on the p5 element, but that's not applicable here. Regardless, we can sneak in and call it right after ourp5object is created to explicitly wipe out any pre-existingdownKeys.Also - please note that this fix will not fix the debugger case, and I don't think that one's fixable with the way
p5is has this implemented. That's a trap where you can hop out of its expected control flow so the keyup doesn't fire (until the user presses a key again), but presumably most of our users won't be adding a breakpoint there so I think we're good.And that's it.
Alternatives could include:
_onblurwhen the p5 object was destroyed, but I opted to move it to the create case so it resets at the beginning - just in case there are more destroy cases I don't know about.downKeysglobal into a_downKeysattribute, so are presumably unaffected. That'd also allow multiple canvases on screen at once.Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: