search

atom / atom-keymap

Atom's selector-based keymap system
MIT License
105 stars 58 forks source link

#130 target document.activeElement when replayed #132

Closed t9md closed 8 years ago

t9md commented 8 years ago

Fix #130 To correctly simulate user input in replaying keystroke, key event should be sent to activeElement instead of original event.target.

Confirmed original problem was solved by cheking vim-mode-plus's f command with latest Atom core master.

t9md commented 8 years ago

CI error is eslint error which I couldn't solve in local, it is not caused by my change. Before adding change, I couldn't build this module without skipping eslint part. This is because eslint version is v2.2.0, old but updating it introduce another error.

So I modify package.json to skip eslint part.

From

   "lint": "coffeelint -r src spec && eslint src spec",

To

    "lint": "coffeelint -r src spec",

And confirmed npm spec all pass without any error.

So pls check this PR.

nathansobo commented 8 years ago

Taking a look at the CI issues.

nathansobo commented 8 years ago

Looks like a linter error. The code looks good. I'll merge and work it out on master. Sorry for the delay @t9md. We're making process adjustments to try to merge things more quickly and I'm sorry this sat around so long.

t9md commented 8 years ago

Thanks for taking care of this!! I almost given up for this PR. What I was not sure about this PR is

nathansobo commented 8 years ago

You mean always using document.activeElement rather than ever using event.target, rather than just using the active element during replay? I suppose that might work. It would cause us to rely on even more global state though it would reduce conditional logic between the initial handling and the replay. I'm okay with the more conservative approach used in this PR for now, but don't feel super strongly because it seems like the activeElement should technically work. You can't really fire a keydown event without having an element focused... but what might we be missing?

t9md commented 8 years ago

I'm okay with the more conservative approach used in this PR for now

Me too. Thanks for comment. I think this library is already very complex, I don't want to introduce big change unless necessary. If we changed to use activeElement it might make direct keyevent test difficult. Thanks a lot.

nathansobo commented 8 years ago

I had to revert this in 3df42f668a8c4c423f0563c0e0af58e0ccd6cd59 because it's causing https://github.com/atom/atom/issues/13181. In retrospect I'm sort of realizing that the pending partial matches depend on the target being stable for all queued keystrokes. If this isn't the case, during replay we may end up failing to disable a set of bindings that causes a partial match because their selector didn't match the original target but does match the current active element. This causes an infinite recursion in the issue mentioned above.

If you have behavior that depends on this not being true, we're going to have to find a different solution. Sorry.