Use of shadowRoot in page makes each focus event register another focus handler, potentially building up to massive slowdowns

[mgsloan]

I believe this line is the culprit: https://github.com/philc/vimium/blob/master/content_scripts/mode_insert.coffee#L69

Repro:

1. Open http://openfin.github.io/fin-hypergrid-polymer-demo/components/fin-hypergrid/demo.html
2. Add a breakpoint in the else if (event.target.shadowRoot) { block of mode_insert.js
3. Click a cell, observe that the event handler gets invoked a couple times (not sure why it's already more than 1). Click another cell, observe that it now gets invoked 3 times. Click another, 4 times, etc etc.

Vimium 1.59

[smblott-github]

Thanks, @mgsloan.

The problem seems to be that the test here is always false, and so the handler is not being removed, as intended. So they build up, as you say.

[smblott-github]

@mgsloan... I think we can just remove the test mentioned above: #2505.

Mention, @mrmr1993.

[mrmr1993]

The shadow DOM spec has changed now... 😡

• Events are dispatched with event.target as the outermost shadow DOM host, and with event.path listing all nodes that the event bubbles through (including those participating in an open shadow DOM).
• We don't get blur/focus events for a change of focus inside the shadow DOM (eg. in the example above, or this simpler one) outside the host (eg. on the window).
  • This still justifies attaching the additional listeners, otherwise we get into an inconsistent state.
  • In fact, we should be adding exactly one for every shadow DOM in the event.path.
• We then only want to remove the shadow DOM listeners if the event fired from a scope above their host.

There's possibly also mileage in something like (to get the active element):

active = document.activeElement
shadow = active.shadowRoot
while shadow?
  break unless shadow.activeElement?
  active = shadow.activeElement
  shadow = active.shadow

But doing this doesn't alert us to transitions to/from a focused element's insert mode.

I'll see if I can throw something together.

(@smblott-github do we need to support the fix for old versions too? I can't remember how event.path used to be spec'ed/implemented so there's a possibility that the right behaviour now will break things in older Chrome versions.)

[mgsloan]

Great, thanks for working on this @mrmr1993 and @smblott-github !

[smblott-github]

@mrmr1993. What about this...

Leaving the event listener in place when the focus is lost is not, in itself, a problem. So we don't actually have to remove the event listeners.

The problem is just that we must not install lots of listeners.

So why not just make it a singleton? When adding a new listener, remove the previous listener (if any).

That's considerably simpler. This is a bit of a corner case, to which we shouldn't really be devoting large amounts of (hard-to-maintain) logic.

[mrmr1993]

So why not just make it a singleton?

I despair. Try the example with such an implementation (edit: example implementation).

That's considerably simpler. This is a bit of a corner case, to which we shouldn't really be devoting large amounts of (hard-to-maintain) logic.

If we want to play nicely with polymer elements (and other users of shadow DOMs), we should do this properly.

[gdh1995]

I can't remember how event.path used to be spec'ed/implemented so there's a possibility that the right behaviour now will break things in older Chrome versions.

@mrmr1993 Chrome shows real target in event.path since version 55 (tested on stable versions).

Vimium has dropped the support for event.keyCode (except pages/options.coffee:257, pages/options.coffee:300), and uses event.key to check keyboard events. In my tests, Chrome began to support event.key since stable 51.

So there would be a trade-off if using event.path.

[mgsloan]

I think that this has been resolved in recent versions of vimium. Not 100% sure since I don't recall exactly how the flamegraph looked. Based on @smblott-github 's comment on #2399 (comment) , the related code has changed a lot, so not surprising this has been fixed as a consquence.