P5.js: multiple sketches: keyPressed in only one of them, but keyTyped in all of them

[vogtadi]

related to Issue #1492

Nature of issue?

• [X] Found a bug

Most appropriate sub-area of p5.js?

• [X] Events

Which platform were you using when you encountered this?

• [X] Desktop/Laptop

Details about the bug:

• p5.js version: 0.7.2
• Web browser and version: Chrome Version 68.0.3440.106 (Official Build) (64-bit)
• Operating System: MacOS 10.13.6
• Steps to reproduce this:
  See example in https://codepen.io/anon/pen/bxYWKj

If there is more than one sketch, the keyPressed events are received only by one of them. Issue #1492 mentions, that is working correct as per design (keyPressed is global), but the related event keyTyped does fire in all sketches.

Is it possible, that the "global nature" of the keyEvents changed since June 2016, but has only been adapted for some keyboard related events in p5.js?

Feature enhancement details:

keyPressed should fire for all sketches on a web page, like other events related to mouse or keyTyped for regular characters.

New feature details:

[vogtadi]

workaround:
In the first instance catch the keyPressed & keyReleased event. For each other instance, call a delegateFunction, which copies over the state of the p5 members "isKeyPressed", "keyIsPressed", "keyCode" and "key", then call the keyPressed or keyReleased event handler.

[Spongman]

this is caused by the fact that downKeys is global, shared between all sketches, and for each event only one sketch makes it past this check:
https://github.com/processing/p5.js/blob/497dac295f33c97539ed82fa87c51fa30e7f99eb/src/events/keyboard.js#L177-L180

downKeys was made global in this commit: https://github.com/processing/p5.js/commit/3e311d6617e085a91fa1b397178a533751e15637

I'm not sure about the rationale behind this change, but IMHO we should try to eliminate all non-const globals wherever possible.

[limzykenneth]

The idea probably was to eliminate public variable instances inside the p5 prototype. If that variable should be separate for each instance like here then it probably should be reverted.

[Zalastax]

https://github.com/processing/p5.js/commit/3e311d6617e085a91fa1b397178a533751e15637 doesn't make it more global than it was before so it is a variable that has always been global. Before, it was a global variable via the prototype chain.

To fix this I'd prefer if we start by writing a test that detects the inappropriate behaviour by running two sketches where events in one sketch affects the other. Writing that test is annoying but in my opinion valuable. The actual code change is hopefully not very difficult.

[Spongman]

oops, you're right. i was confused by the prototype variable thing.

my vote is for this to become instance-local.

[Ajayneethikannan]

Can I work on this issue please @Spongman @Zalastax ? I too believe that downKeys should be present separately for every instance.

[Ajayneethikannan]

Can I please know the status of this issue ? I would love to contribute to this issue, and the step I will take is

• converting var downKeys = {}; to be separate for every instance this.downkeys = {}

As mentioned by @Zalastax , about writing tests before implementing the feature,
the tests that I thought which needed to be done are ,
considering two sketch instances, ( for the keyPressed function only ),
when some key is pressed,

• both sketches' keyPressed functions should have run the same number of times,
• both should have the same keyCode.

considering one sketch instance,

• keyPressed function should execute only once for every distinct key, which is pressed.
• keyCode is in sync with the latest key which was pressed.

[Ajayneethikannan]

Also this is my first time writing an unit test , I would be really grateful if someone could help me out regarding the bug in this test :sweat_smile: ,

suite('Keyboard events', function() {
  suite('keyPressed', function() {
    let p5one, p5two;
    let count1 = 0,
      count2 = 0;
    setup(function(done) {
      let done_count = 0;
      function predone() {
        done_count++;
        if (done_count === 2) {
          done(); //call only after both the instances are properly initialized
        }
      }
      new p5(function(p) {
        p.setup = function() {
          p5one = p;
          predone();
        };
      });
      new p5(function(p) {
        p.setup = function() {
          p5two = p;
          predone();
        };
      });
    });

    teardown(function() {
      p5one.remove();
      p5two.remove();
    });

    test('both keyPressed functions must run the same number of times', function(done) {
      p5one.keyPressed = function() {
        count1++;
      };
      p5two.keyPressed = function() {
        count2++;
      };
      let e = new KeyboardEvent('keydown');
      window.dispatchEvent(e);
      setTimeout(() => {
        //inside setTimeout so that it can be executed after the events have triggered _onkeydown.
        assert.equal(count1, count2);
        done();
    }, 0);
    });
  });
});

This is the test I tried to check if both the keyPressed functions are executing same number of times.
But neither of the keyPressed functions are called , even though the p5.prototype._onkeydown function is called. Somehow the 'this' variable , in _onkeydown function doesn't seem to be pointing at p5one or p5two, when invoked. The same code works well when written as a normal sketch (I tried to approximate done function behavior) .
My doubts are:

• why do we call the done function, and set the sketch variable myp5 inside the setup (generally)? (my understanding was that to make sure that all the properties and functions are properly initialized, global attributes set ,etc. so I applied it here)
• I would be very thankful, if some one pointed what I was doing here wrong.

Please tell me whether I can work on this issue, or please let me know if I am making any mistake

Thank you!

[meiamsome]

@Ajayneethikannan I suspect that the case is that there is already another sketch on the page from other tests that haven't properly cleaned up after themselves, and as the tests aren't isolated we get that one of those sketches gets the key press and not one of the two in your test. Therefore, both count1 and count2 are equal.

Indeed, if we change your codepen to launch another instance first, you can see that the test passes:

let p5one, p5two;
let done_count = 0;
let count1 = 0;
let count2 = 0;
function predone() {
  console.log('called pre');
  done_count++;
  if(done_count == 2) done();
}
new p5(() => {});
new p5(function(p) {
  p.setup = function() {
    p5one = p;
    predone();
  };
});
new p5(function(p) {
  p.setup = function() {
    p5two = p;
    predone();
  };
}); 
function done(){
  console.log('set methods');
  p5one.keyPressed = function() {
    count1++;
  };
  p5two.keyPressed = function() {
    count2++;
  };
  let e = new KeyboardEvent('keydown');
  window.dispatchEvent(e);
  window.addEventListener('assert', function() {
    console.log(count1, count2);
  });
  setTimeout(function() {
    console.log(count1, count2);
    if(count1 !== count2){
      console.log("test failed");
    }
  }, 0);
}

Instead of testing count1 == count2, I'd suggest testing both count1 === 1 and count2 === 1 as these should both be true in a working implementation, and indeed if both are true this implies count1 == count2:

      setTimeout(() => {
        //inside setTimeout so that it can be executed after the events have triggered _onkeydown.
        assert.strictEqual(count1, 1);
        assert.strictEqual(count2, 1);
        done();
      }, 0);

Do note however, that asserting like that is generally considered bad form because the exception gets thrown in an anonymous function, so maybe this is better:

      setTimeout(() => {
        //inside setTimeout so that it can be executed after the events have triggered _onkeydown.
        try {
          assert.strictEqual(count1, 1);
          assert.strictEqual(count2, 1);
          done();
        } catch (e) {
          done(e);
        }
      }, 0);

I'd recommend trying both of these out if you want to see how the error message outputs differ, and why it's important to handle asynchronous errors if you can in tests.

The rest of your code (setup, teardown, etc.) look logically sound to me, but manually managing the instances like that is always quite prone to bugs (Particularly, this is why there are still p5.js sketches on the page). Here is the (roughly) equivalent code converted to the Promise features of Mocha & using the promisedSketch helper:

suite('Keyboard events', function() {
  suite('keyPressed', function() {
    test('multiple keyPressed functions must run once', async function() {
      const sketches = 2;
      let ready = 0;
      const counts = await Promise.all(
        Array.from({ length: sketches }, function(_, i) {
          return promisedSketch(function(sketch, resolve) {
            let count = 0;
            sketch.setup = function() {
              setTimeout(() => resolve(count), 10);
              ready += 1;
              if (ready === sketches) {
                const e = new KeyboardEvent('keydown');
                window.dispatchEvent(e);
              }
            };

            sketch.keyPressed = function() {
              count += 1;
            };
          });
        })
      );
      assert.deepEqual(counts, Array.from({ length: sketches }, () => 1));
    });
  });
});

I'll admit that is not very easy to ready, but it would be possible to refactor to be cleaner than it currently is.

why do we call the done function, and set the sketch variable myp5 inside the setup (generally)? (my understanding was that to make sure that all the properties and functions are properly initialized, global attributes set ,etc. so I applied it here)

You're right: You could do myp5 = new p5(...) and that would be valid, but by setting it in setup you make sure that the reference only exists when the p5 instance is setup and ready and no earlier, so an incorrectly written test will fail with myp5 being undefined, rather than having a strange error because myp5 is not yet initialized correctly.

The done function of setup stops any tests in the suite from running before the sketch is fully setup in general because otherwise the first test will be called immediately after the mocha setup function finishes. This could cause a problem if, for example, the sketch that has been set up is waiting for the page to load completely.

Hopefully that answers your questions, let me know if otherwise.

[Ajayneethikannan]

Thank you very much for clearing my doubts, @meiamsome !

[Ajayneethikannan]

@meiamsome I tried to refactor the code suggested by you, and these are the changes I made :

suite('Keyboard Events', function() {
  suite('keyPressed', function() {
    test('multiple keyPressed functions must run once', async function() {
      const sketches = 2;
      let ready = 0;
      let fulfil_array = [];
      function sketch_function(sketch, resolve) {
        let count = 0;
        sketch.setup = function() {
          ready++;
          fulfil_array.push(() => resolve(count));
          if (ready === sketches) {
            window.dispatchEvent(new KeyboardEvent('keydown'));
            setTimeout(resolveSketches, 0);
          }
        };
        sketch.keyPressed = function() {
          count += 1;
        };
      }
      function resolveSketches() {
        fulfil_array.forEach(resolve => resolve());
      }
      const counts = await Promise.all(
        Array.from({ length: sketches }, () => promisedSketch(sketch_function))
      );
      assert.deepEqual(counts, Array.from({ length: sketches }, () => 1));
    });
  });
});

• removed the i variable in Array.from function, as it was not being used any where .
• Tried to remove the dependence on specific setTimeout time parameter, by calling all resolve functions of every promisedSketch after the keyboard event has been dispatched.

I also have one doubt regarding the promisedSketch() function :

var promise = new Promise(function(resolve, reject) {
    myInstance = new p5(function(sketch) {
      return sketch_fn(sketch, resolve, reject);
    });

What is the purpose of returning the sketch_fn result here, when it can be called without any return statement ?

I have made a PR , with the same , can you please provide your suggestions on it ? (#3582 )

Thank you very much for all your help !