Switching tabs via search sets last tab incorrectly

[jchang504]

Because switching tabs via search occurs in a separate page (the tab_search.html popup) with a separate JS script, it's a little tricky to update the global last_tab_id which resides in the background script (probably need to send a message), so I left this out of 2f6caec and just implemented the core functionality of #14. But we should fix it to work after tab search too.

[jchang504]

A little trickier than I thought initially because once you open up a tab search tab, it becomes the current tab and you don't want to save it as the last tab since it gets closed once the user makes a selection. So we actually need to save the tab right before we open up the tab search tab (but also keep the tab before that, in case tab search gets cancelled/not used).

[jchang504]

Even trickier: there's another separate case if the user clicks the KeepTabs icon to open up the tab search popup, rather than using the hotkey to open up tab search in a separate tab. In this latter case, the background script isn't involved at all -- the tab search page is brought up because it is specified as the popup behavior.

We can solve both of these cases and reuse code by doing the actual navigation in the background script, just sending over the tab and window ids from the tab search page as a message. This way the tab search page closes itself in either case, and then the background script can use the standard behavior (save current tab as last_tab_id) for navigating to a new tab.

[jchang504]

The problem with my previous suggestion is that when you close the tab search (in the case when it's in a new tab, called by hotkey) the browser defaults to going to the next left tab, not the one you opened tab search from. This messes up the last_tab_id when you navigate to the tab selected in search.

Separately, I also forgot to address the issue mentioned in my other comment: that it would be nice to preserve the old last_tab_id in the case that the user cancels the search (i.e. closes the search tab). (We don't have to worry about the popup case for this; the last tab will naturally be preserved because the popup isn't a tab.) Here are my new thoughts for fixing both of these. Unfortunately, I can't come up with a solution that doesn't require explicitly checking if the tab search is performed from a tab or from the popup, which is done by comparing the current tab as viewed by the tab search page and the background script after the tab search page is closed. There's gotta be a more elegant solution.

Hotkey flow

1. The user presses the hotkey to open tab search in a new tab. The background script first saves the current tab as search_launch_tab_id before opening the tab search tab.
2. 1. The user closes the tab search tab without making a selection. last_tab_id is unaffected and still correct. (Note: the user cannot close the tab search tab with a hotkey, as KeepTabs isn't running on its own pages. So they cannot close the tab in a way that updates last_tab_id.)
   2. The user selects a tab to navigate to. The tab search page sends a message to the background script with the tab and window id of the selection, as well as the current tab's id (in this flow, it is the id of the tab search tab). The background script navigates to the selected tab with the common function navigateToTab (this wrongly sets last_tab_id to whatever tab happened to be active after the tab search tab closed), then corrects by setting last_tab_id = search_launch_tab_id. All is well.

Popup flow

1. The user clicks the KeepTabs extension icon to open tab search in a popup window.
2. 1. The user clicks outside the popup window to close it without making a selection. No KeepTabs code runs as a result of this so everything remains fine.
   2. The user selects a tab to navigate to. The tab search page sends a message to the background script with the tab and window id of the selection, as well as the current tab's id (in this flow, it is the id of the tab on which the popup is open). The background script navigates to the selected tab with the common function navigateToTab (this correctly sets last_tab_id). By comparing the current tab id sent by the tab search page and the current tab id now obtained by another query (actually this must occur before the navigateToTab call of course) and seeing that they are equal, the background script knows the search was performed in the popup and doesn't correct the last_tab_id.

[jchang504]

After implementing the above, I observe one small problem. Because the navigateToTab call involves an asynchronous API call, it's actually setting the last_tab_id (incorrectly) after we've made the correction to search_launch_tab_id. I should've thought it's dangerous to put an asynchronous call anywhere but last in a code block if I want to assume it's done by the next line. To fix this, I'll actually navigate to search_launch_tab_id before making the navigateToTab call. This is essentially the idea of two comments above -- having the tab search tab close and return back to the original tab it was opened from.

[jchang504]

Okay, fixed the problem immediately above. I'm now realizing that because I used createNewTab which updates last_tab_id to open the tab search tab, we actually lose the correct last_tab_id from before we launched tab search. But, now that I see it, I actually like this behavior better. Because when the user manually closes the tab search tab, the browser plops them down on whatever tab was to the left of it, and then they can return to the original launching tab with the previous tab hotkey, which makes sense. The alternative is weird because they would return to the tab before the launching tab with the previous tab hotkey, and then not be able to get back to the launching tab (last_tab_id would now be the random tab they were plopped down on).

[carbon-steel]

This comment moved to a new issue #44.