Ctrl+Shift+B swaps between the background

[boothym]

As described here #2492 (comment)

using Ctrl+Shift+B as described in the keyboard shortcuts swaps between the background like when using Ctrl+B.

[kepta]

I got some time to look into this bug.

There are two d3keybinding namespaces involved, info and background.

info namespace registers Ctrl+⇧+B

{
    "event": {
      "key": "b",
      "keyCode": 66,
      "modifiers": {
        "shiftKey": true,
        "ctrlKey": false,
        "altKey": false,
        "metaKey": true
      }
    }
  }

and background registers ⇧+B

{
    "event": {
      "key": "b",
      "keyCode": 66,
      "modifiers": {
        "shiftKey": false,
        "ctrlKey": false,
        "altKey": false,
        "metaKey": true
      }
    }
  },

I guess problem seems to lie here https://github.com/openstreetmap/iD/blob/master/modules/lib/d3.keybinding.js#L25
The background's callback gets called when pressed Ctrl+⇧+B, since line gets executed, believing Ctrl+B was pressed.

@bhousel what do you think?

[bhousel]

I guess problem seems to lie here https://github.com/openstreetmap/iD/blob/master/modules/lib/d3.keybinding.js#L25
The background's callback gets called when pressed Ctrl+⇧+B, since line gets executed, believing Ctrl+B was pressed.
@bhousel what do you think?

Yeah, it does seem like something not quite right in that code. I would expect didMatch should be true after the ⌘⇧B, so that ⌘B would not be tested.

[kepta]

@bhousel I might be wrong though, but having a separate namespace would have separate instances of d3keybinding?
And if that is true they would likely have their own didMatch variables?

[bhousel]

didMatch is a local variable to the testBindings() function, so every time a key is pressed and iD tries to match a handler, it should start out being false. I think the code can be changed to make it more clear or exit early as soon as a match happens.

[bhousel]

I found out what was happening, the key event just gets sent to all of the listeners unless we specifically stop it. This is normal.

I added some stopImmediatePropagation() calls to stop the event from firing after one of the listeners handles it.

I'm a bit uneasy about this because it means that the code is probably order-dependent. So it works as long as ⌘⇧B and ⌘B are registered in the right order, otherwise the ⌘B could potentially override the ⌘⇧B.

I'll think more about this but closing the issue for now..