Client Side JavaScript [aka Greasemonkey] Key Event Handler

Question

Description

The code captures keypress events that do not occur in a text field and dispatches on the properties of a $command object in the application execution context. I have provided a sketch of the execution context in order to assist your review of the code of interest.

Execution Context Sketch

This is a sketch the dependencies of the code for review. The larger context is a user side script running in Firefox using Greasemonkey. The Greasemonkey script is part of a suite of user side tools. This code is not intended for review.

(function () {

  // There is additional application context for making this global
  window.$snatch = function ($class) {
    return document.getElementsByClassName($class)[0];
  };

  // There is additional application context for making this global
  window.$commands = {};

  /* Define links */
  // There is additional application context supporting defining links
  // independently of the code utilizing them.
  var yes = $snatch('yes');
  var maybe = $snatch('rmaybe');
  var no = $snatch('no');

  /* Create Commands */
  // There is additional application context that makes multiple
  // keypresses a more practical approach than it might appear
  $commands.ccc = () => { close(); };
  $commands.yyy = () => { location = yes };
  $commands.mmm = () => { location = maybe };
  $commands.nnn = () => { location = no };
})();

JavaScript for Review

This code for handling key events originated with a copy-paste of this example on MDN. The modifications have been primarily to implement dispatch.

/* Key Event Handler */
(function () {
  const commandLength = 3;
  var nOffset = 0;
  var keyQueue = "";

  // Add a new property listing all the properties
  $commands.keys = Object.keys($commands);

  document.onkeypress = function (oPEvt) {
    var oEvent = oPEvt || window.event, nChr = oEvent.charCode, sNodeType = oEvent.target.nodeName.toUpperCase();
    if (nChr === 0 || oEvent.target.contentEditable.toUpperCase() === "TRUE" || sNodeType === "TEXTAREA" || sNodeType === "INPUT" && oEvent.target.type.toUpperCase() === "TEXT") { return true; }
    if (keyQueue.length < commandLength) {
      keyQueue += String.fromCharCode(nChr);
    }
    else if (keyQueue.length == commandLength) {
      if ($commands.keys.includes(keyQueue)) {
        $commands[keyQueue]();
        keyQueue = "";
      }
      else {
        keyQueue = keyQueue.slice(-2);
      }
    }
    else { alert("too many characters!"); }
    return true;
  }
})();

Answer 1

Future-proofing

The unreachable else that's not entirely unreachable

In your final else-statement you have an alert that screams at the user "too many characters!". First of all, don't use alert(..), especially for something the user has no control over. Use console.error(..) instead, or just ignore it.

If each handler is executed sequentially after the last one is executed, the else statement can not be reached. There is however no guarantee that this is the case as far as I know. Consider writing at least a bit defensively and notice that the situation where more than commandLength character actually means that your character was added before the other handler cut the characters.

The keys command that is actually not a command

You are abusing the $commands variable to store the keys of that array. If in the future you decide that you want to have commands of length 4... you have a problem. Instead use let keys = Object.keys($commands);.

Clearing the keyQueue

You are clearing the queue after you are executing $commands[keyQueue]();. Depending on how long executing that function takes, that can take a long time. Do it as soon as you can.

Magic number

The -2 in keyQueue.slice(-2); is a magic number. What you actually meant to say is likely keyQueue.slice(-commandLength + 1);. Future-proof your code by eliminating that number and replace it by the actual calculation. You can probably even move this code to the top to group together with the other code to determine the key combination.

Issues with style

ES6 and var

You are using const in your code. If you decide to use const, also use let for non-const variables, as they behave much better than variables declared with var.

Comma-seperated declaration of variables

I am not a fan of comma-seperated declaration of variables. If the declaration is split over multiple lines, it is error-prone and easily allows you to drop variables in the global scope. If it is on one line, like you did, it becomes unreadable and falls off the right side of the screen. Just declare each variable on a single line.

Braces

Your usage of braces is inconsistent. I recommend always having a newline after an opening brace.

Mixing of logical or and logical and

Know which operator is evaluated first, the logical or operator or the logical and operator? Yes? Congratulations, because many developers don't. To avoid ambiguity and possible errors in your code, add parenthesis when you have both a logical or and a logical and in a boolean expression.

Unused variable

nOffset does not seem to be used. Lets just delete it.

---

I would rewrite your code like this:

/* Key Event Handler */
(function () {
  const commandLength = 3;

  //Last pressed keys
  let keyQueue = "";

  let keys = Object.keys($commands);

  document.onkeypress = function (oPEvt) {
    let oEvent = oPEvt || window.event;
    let nChr = oEvent.charCode;
    let sNodeType = oEvent.target.nodeName.toUpperCase();

    if (nChr === 0 ||
        oEvent.target.contentEditable.toUpperCase() === "TRUE" ||
        sNodeType === "TEXTAREA" ||
        (sNodeType === "INPUT" && oEvent.target.type.toUpperCase() === "TEXT")
       ) {
      return true;
    }

    keyQueue = keyQueue.slice(-commandLength + 1) + String.fromCharCode(nChr);
    if (keys.includes(keyQueue)) {
      keyQueue = "";
      $commands[keyQueue]();
    }

    return true;
  }
}());