Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tab close and left-right nav hotkeys sometimes fail to send hold key release to other tabs #15

Closed
jchang504 opened this issue Apr 5, 2017 · 13 comments

Comments

@jchang504
Copy link
Owner

We immediately send the tab close hotkey to the background script (like the left-right navigation, we don't wait for keyup on the hold key). In some cases, if the user releases the hold key right in-between when the tab closes and Chrome switches to the adjacent tab, the keyup won't be handled either on the closed tab or on the new active one, causing no hold key released message to be broadcast and all tabs to be stuck in the holding = true state until the hold key is deliberately pressed and released to fix it. The solution I'm thinking of right now isn't perfect, it would be: don't treat the tab close hotkey like left-right nav; i.e. don't send it until the hold key is released. There's an additional decision whether to treat it exactly like a regular user-specified hotkey, or to treat it as special so that if a user enters ; anywhere in the hotkey sequence, we count it as a close tab. Pro: Fixes the bug; also, makes closing a tab a slightly more deliberate and cancellable (if we treat it like a regular hotkey requiring exact match) action, which may be good. Con: Prevents rapid closing of multiple tabs in the same spirit as rapid left-right nav. This doesn't seem like a super common use case, but it does exist.

@jchang504
Copy link
Owner Author

Could this also happen when using left-right nav, if the hold key is released between switching tabs? I haven't experienced it, so I'm guessing not -- but I'm not sure what's different between left-right nav and this bug. My best guess would be that the keyup event gets triggered on the tab to be closed (so not on the new tab moved to), but the tab gets closed before it actually handles the event. Whereas with left-right nav, the keyup event must either land on the original tab or the tab just navigated to.

@jchang504
Copy link
Owner Author

This might be a better fix: explicitly switch the active tab to another tab before we close the intended tab. Though if it's just a matter of timing, it could still be possible for the keyup to get triggered on the tab to close, switch to the new tab, and then the old tab gets closed before it executes the keyup handler.

@jchang504
Copy link
Owner Author

Another idea. In tab_search.js we close the tab search page with window.close(); strangely, that call doesn't seem to take effect until the script finishes the rest of the function, where we send a message to the background script. I've googled around and can't find concrete semantics for window.close(), but in practice it looks like a less aggressive way of closing a tab than the chrome.tabs.remove we're currently using. Maybe if we use this method instead, we'll fix this bug. This requires having the background script send a message to the content script of the tab to close, but we already have the content script listening for hold key messages, so it's not adding much complexity.

@jchang504
Copy link
Owner Author

Darn, I tried it but apparently there is a security restriction (I should've paid more heed to mentions of this I saw in MDN documentation) not allowing you to window.close() a page that was not launched by the extension. Specifically, the console warning says: Scripts may close only the windows that were opened by it. I'm going to try the solution proposed two comments up; although it doesn't provide theoretical guarantees against the problem, it might work in practice.

@jchang504
Copy link
Owner Author

After thinking a bit, there's an edge case this solution can't quite handle: when the tab is the last one in a window (with other windows existing; if it was the global last tab, we don't have to worry about the hold key getting "stuck"). In this case, we don't have another tab to switch to (if we switched to a tab in another window, that would disagree with Chrome's default behavior and be really unexpected). At the same time, I've come to realize that my original proposed solution isn't good either, because I do in fact often use left-right nav and tab close hotkeys in close succession without releasing the hold key (e.g. moving over to close a bunch of old tabs, or closing a tab and then moving left to a different one than the default tab active after closing). I'm out of good solutions.

As a side note, I tried just broadcasting a hold key release whenever we close a tab. I figured that the repeated keydown events generated by holding the hold key down would relatively quickly set holding = true on the new active tab and the user might not notice any delay. However, I'm finding that while keeping the hold key held down, after hitting ";" to close the current tab, keydown events are not getting fired on the new active tab. This baffles me because if I disable KeepTabs, open two tabs of this, hold down escape on the right tab, and close the right tab with the mouse, I see keydown events getting fired on the new active tab. There aren't many differences between this test and what I'm trying to do in KeepTabs...

@jchang504
Copy link
Owner Author

Okay, with more careful examination, I've found that what I brought up in this comment is true. You can miss the hold key release when using left-right nav if you release it quickly "between" tabs. We need a solution that generalizes to both of these. Updating issue title and complexity.

@jchang504 jchang504 changed the title Tab close hotkey sometimes fails to send hold key release to other tabs Tab close and left-right nav hotkeys sometimes fail to send hold key release to other tabs Jun 5, 2017
@jchang504 jchang504 added complex and removed involved labels Jun 5, 2017
@jchang504
Copy link
Owner Author

jchang504 commented Jun 5, 2017

With some more tinkering, I've figured out the behavior of a held down key and why my idea two comments up didn't work. The difference between what I was trying to do in KeepTabs and the test I did is that I used the mouse to switch tabs. When key A is held, keydown events for it continue to get fired rapidly, but if another key B is pressed, they stop getting fired (even if A is still held down). (You can easily observe this with two alphabet keys. Try holding 'a' and then press 's'. You'll see that the repeated a's stop getting typed.) Thus when we pressed [ or ] or ; to do left-right nav or close a tab, the new active tab didn't receive repeated keydown events for the hold key because another key being pressed had interrupted the repeated events.

@carbon-steel
Copy link
Collaborator

New discovery: The hold key behaves perfectly fine when there are one or two tabs when you use the hotkeys in question. When the hotkeys are pressed when there are three tabs or more, they consistently make the hold key stay active when it's not supposed to.

@carbon-steel
Copy link
Collaborator

I don't think that the reason why the hold key is misbehaving is that the release event is being triggered "in between tabs" since the problem still occurs when I release the hold key after waiting for the tab switch to occur.

@jchang504
Copy link
Owner Author

jchang504 commented Jul 1, 2017

I find it extremely hard to believe that the bug has any direct relation to the number of tabs you have open. Note that it occurs very infrequently to begin with; the timing has to be just right -- in particular with left-right navigation (it happens more with tab close, I find). So any such pattern sounds purely coincidental, as I can't imagine any reason why this behavior would depend on the number of open tabs itself. Though, I have noticed that it can occur more when switching between many tabs or to an older tab; it's probably related to Chrome's memory limiting functionality that takes old tabs out of memory when they've been unused a while -- probably a tab that is being loaded back into memory takes longer to be ready to listen for key events, creating a larger window of opportunity for the hold key release to be missed.

As for the latter comment, can you provide minimal steps to reproduce? I've never seen this occur, and I've tinkered with this a lot.

@carbon-steel
Copy link
Collaborator

carbon-steel commented Jul 16, 2017

Reiteration

On my Macbook, this issue occurs very frequently. I agree that this has most likely to do with Chrome's memory management. I reaffirm that the problem is not that the keyup event is being missed "in between tabs" as the issue happens whether I release the hold key quickly or after a pause.

Observations

Furthermore, I have observed that, in the instances where this issue occurs, no keyup event is being registered at all when I release the hold key. During my testing, the hold key was set to the escape key. This test was done by printing console logs whenever the content script detected a keyup event (Note that you can't listen for the keyup events from the webpage itself because the extension prevents the event from getting to the webpage).

Possible recourse

If this issue is specific to the escape key

However, I have noticed that the problem disappears when I change the hold key to be my control key. This issue may be a problem specific to the escape key. If this is the case, we may want to consider removing the escape key from the list of possible hold keys. Macs can easily use the alt or control key as the hold key, and windows and linux can use the alt key (they can also map any of the possible hold keys to their caps lock key).

Else

Even if this issue still occurs with the other modifier keys, there is still reason to consider removing the escape key from the list of possible hold keys. The escape key is not a true modifier key like control or alt (we've just been hacking it into one). Javascript actually tells us which (true) modifier keys were being held down when a click event or a keyboard event occurs (the code for this looks like e.altKey where e is an event object). If we run into this issue again even when the escape key is not the hold key, we can resolve the issue like so:
Changes

  • Check all keydown events to see if they say that the hold key was down when the key was pressed
  • If a keydown event occurs for a built-in hotkey and the event (NOT the extension!!!) showed that the hold key was held down, then execute the built-in hotkey and then send a message to the background script that the hold key is NOT being pressed down.

Benefits

  • Since the hold key is always being set to not being pressed down after any built-in hotkeys execute, the hold key will never get stuck.
  • Since the built-in hotkeys don't rely on the extension to know when the hold key is being held down (instead, it just reads that information straight from the keydown events), we can continuously press the built-in hotkey over and over without needing to reclick the hold key multiple times.

NOTE: These changes only impact the built-in hotkeys and have only minimal impact on regular hotkeys!
Cons

  • The extension does not always know when the hold key is being pressed down. It will think that the hold key is released even if it is being pressed continuously from a built-in hotkey.

However, this does not actually cause any problem because, instead of keeping track of when the hold key is being held down, the extension will keep track of a similar abstraction: when the extension is listening for a non-built-in hotkey to be typed in. The visual overlay would reflect this change out-of-the-box.

If we go with this approach, then the visual overlay wouldn't represent that the hold key is being pressed down. Rather, it would represent that the extension is now listening for a non-built-in hotkey to be entered.

I say "non-built-in" hotkey because the extension will always execute a built-in hotkey when the hold key is being held down since it reads whether the hold key is being held down straight from the event object. It does not rely on the state of the extension where as "non-built-in" hotkeys do.

Side Effects

This is related to #1 . Using only the true modifier keys can make it a lot easier to write clean code for all the hold keys.

Also, if we remove the Escape from our list of possible hold keys, then #13 is not a problem any more because escape is the only one of the possible hold keys that had any function that occurred when you pressed it alone.

#35 would also no longer be a problem since keys pressed with modifier keys don't trigger any listeners.

@jchang504
Copy link
Owner Author

After further investigation, I feel I've gotten to a much better understanding. I at least know why we observe a difference in behavior between escape, control, and Cmd as hold keys on Mac, as well as confirming that Mac and Ubuntu Chrome behaviors differ (even using the same version of Chrome). From now on, polished long-form investigation should be added to the wiki page I just started (investigation in the history section; updated conclusions in the current hypothesis). I still have some TODOs to finish filling in tomorrow.

@jchang504
Copy link
Owner Author

In conclusion, Roger's proposed solution is the best way to go. We just need to determine which hold key choices offer the best combination of 1) convenient location (or convenient method for remapping to convenient location) and 2) lack of interference with browser and OS shortcuts (or ways to remap them to remove interference). To this end, I've created a wiki page for collecting this information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants