Warn on close multiple does not work when tabs are closed using the keyboard

[BobVul]

Short description

When the keyboard (Ctrl+W) is used to close a parent tab of a collapsed group, the warning for multiple tabs being closed does not appear. All the tabs in the group are immediately closed.

Steps to reproduce

1. Start Firefox with clean profile.
2. Install TST.
3. Create a tree with at least one child.
4. Focus on the root (parent) of the tree.
5. Collapse the tree.
6. Press Ctrl+W to close the parent tab of the tree.

Observe that the entire tree is closed without warning.

GIF screen recording:

Expected result

There should be a warning before closing multiple tabs, as per the default setting.

Actual result

There is no warning. All tabs in the tree are closed.

Environment

• Platform (OS): Windows 10 v1803
• Version of Firefox: 66.0.3
• Version (or revision) of Tree Style Tab: 3.0.9

[BobVul]

Looks like there's a bit of confusion here:

treestyletab/webextensions/background/background.js

Lines 443 to 467 in df9f430

	export async function confirmToCloseTabs(tabIds, options = {}) {
	tabIds = tabIds.filter(id => !configs.grantedRemovingTabIds.includes(id));
	const count = tabIds.length;
	log('confirmToCloseTabs ', { tabIds, count, options });
	if (count <= 1 ||
	!configs.warnOnCloseTabs ||
	Date.now() - configs.lastConfirmedToCloseTabs < 500)
	return true;
	const tabs = await browser.tabs.query({
	active: true,
	windowId: options.windowId
	}).catch(ApiTabs.createErrorHandler());
	const granted = await Permissions.isGranted(Permissions.ALL_URLS);
	if (!granted ||
	/^(about|chrome|resource):/.test(tabs[0].url) ||
	(!options.showInTab &&
	SidebarConnection.isOpen(options.windowId) &&
	SidebarConnection.hasFocus(options.windowId)))
	return browser.runtime.sendMessage({
	type: Constants.kCOMMAND_CONFIRM_TO_CLOSE_TABS,
	tabIds: tabs.map(tab => tab.id),
	windowId: options.windowId
	}).catch(ApiTabs.createErrorHandler());

tabIds is the array of tabs to close, but the message sent to the confirm dialog uses tabs instead. tabs is the single currently active tab (used to display a message within the tab?).

I would suggest renaming tabs to activeTab and tabIds to closingTabIds. Then it's clearer that line 465 should instead be tabIds: closingTabIds

treestyletab/webextensions/background/background.js

Line 465 in df9f430

tabIds: tabs.map(tab => tab.id),

[piroor]

Thanks, this also should be fixed with recent commits.

[BobVul]

Thanks for the quick response. Unfortunately this one does not seem to be completely fixed yet: the confirmation prompt now appears correctly, but the tabs are still removed from TST even if cancelled. The parent tab is completely removed (maybe okay, though a bit counterintuitive) and the children remain in the Firefox tab bar.

GIF recording:

[BobVul]

Hold on - it's working now, after I tried debugging it. Interesting. I'll see if I can find a consistent way to reproduce.

[BobVul]

Looks like it's fine when testing with real websites. The scenario in the above recording only appears with blank (about:newtab) tabs, probably because of how Firefox's tab history/restore works with them (i.e. it ignores them completely). So this is probably fine in real-world use.

There is another quirk in that if a blank parent tab is closed, the reopened tab can be a previous one from the history. I'm not sure what can be done about this, unless about:newtab is special-cased and restored differently by TST. But that's probably not worth the effort.

Thanks for the fix!

[piroor]

Thank you for researching. As you know Firefox doesn't allow to us to restore closed blank tabs. On the other hand TST expected that closed tabs are always restorable.

• Basically, on an WebExtensions-based addon we cannot do anything before a tab is removed.
• So TST cannot show confirmation dialog before tabs are closed with Ctrl-W or other methods outside of TST.
• Instead, TST detects already removed tabs and show a confirmation regardless they are actually closed. And if the "Cancel" button is chosen TST restores closed tabs for rolling back.

So I've introduced one more change: TST now doesn't show any confirmation if all closed tabs are unrestorable. This is an unavoidable edge-case restriction of WebExtensions-based addon.

[BobVul]

Thanks, that looks like a good fix for the case of all-blank tabs. I just tried the dev build and it works well :)