1908417 - [meta] Audit gBrowser.tabs and gBrowser.visibleTabs usage with regards to tab groups

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task)

Comment 1

[Stephen Thompson [:sthompson]]

Raw notes

gBrowser.tabs

• Browser/base/content/browser.js
  • getTabCount — how is it used?
  • _closeDeviceConnectedTab checks tab count and opens new tab
• ext-browser.js *getTabs, getTabAtIndex
• ext-tabs move calculating insertion
• ext-tabs highlight sets selected tabs directly
• aboutSessionRestore.js — top.gBrowser.tabs.length == 1, will restore into current window. Does that work if the window has 1 tab in 1 tab group?
• Browser/components/tabbrowser/TabsList.sys.mjs — populates tabs list based on gBrowser.tabs
• browser-ctrlTab.js — filters recently used tabs from gBrowser.tabs based on not closing and not hidden
• tabbrowser.js updateContextMenu disabled state based on gBrowser.tabs.length == 1
• tabs.js on_drop adds range to multi-selected tabs based on indexes in gBrowser.tabs — might this pick up tabs from a collapsed tab group in the range? I’m assuming those should not be selected
• tabs.js on_dragend checks tabs.length == 1
• tabs.js _updateHiddenTabsStatus sets “hashiddentabs” if length of .tabs greater than length of .visibleTabs
• Services/sync/modules/engines/tabs.sys.mjs gets all tabs from .tabs
• tabbrowser.js getDuplicateTabsToClose excludes some tabs (pinned, multi selected) from being treated as duplicates. Perhaps tabs in groups should also not be treated as duplicates, e.g. if the user has mozilla.com open in two different tab groups?
• tabbrowser.js createTabsForSessionRestore has special handling for pinned and hidden tabs; probably needs special handling for tab groups
• tabbrowser.js _beginRemoveTab browsingContext gets hasSiblings = false if there are 2 tabs and 1 is being closed. Any edge cases for tab groups? I don’t think so
• tabbrowser.js _endRemoveTab updating tabs[I]._tPos indexes, not sure if edge cases for tab groups
• tabbrowser.js moveTabTo indexes into .tabs to determine the neighboring tab. It looks like it might not be handling the case there the neighbor is in a tab group.

gBrowser.visibleTabs

• tabbrowser.js updateContextMenu disabled state based on gBrowser.visibleTabs.length == 1
• tabs.js _updateHiddenTabsStatus sets “hashiddentabs” if length of .tabs greater than length of .visibleTabs
• browser-commands.js closeTabOrWindow will select the first non-pinned tab. Should this select the first tab in a tab group if that’s the first non-pinned tab? Should that also happen if the tab group is collapsed?
• browser-places.js bookmarkTabs only bookmarks .visibleTabs
• browser-places.js toggleHiddenTabs determines whether the hidden tabs menu should be shown based on .visibleTabs.length < .tabs.length. This would show the hidden tabs menu if there were collapsed tab groups
• browser.js warnAboutClosingWindow gives the user a message about the number of tabs that will be closed, but it’s .visibleTabs, so it doesn’t count the tabs in collapsed tab groups
• Browser.js handleEvent TabSelect makes gBrowser.visibleTabs[0] focusable — this skips tabs in collapsed tab groups right now
• ASRouterTargeting.sys.mjs hasPinnedTabs only looks for pinned tabs within .visibleTabs. If we allow pinned tabs into collapsed tab groups in the future, this would break
• Browser/components/firefoxview/opentabs.mjs moveMenuTemplate only uses .visibleTabs to determine whether to give the user options to move to the start and/or end of the tab bar.
• ProfilesParent.sys.mjs receiveMessage Profiles:GetDeleteProfileContent counts tabs as .visibleTabs.length — should this be including the count of tabs in collapsed tab groups?
• tabbrowser.js replaceTabsWithWindow selects all .visibleTabs in the new window. This may have odd results if tabs in collapsed tab groups can be selected to be extracted to a new window
• tabbrowser.js updateContextMenu uses .visibleTabs to compute a lot of logic
• Browser/components/tabbrowser/content/tabs.js attributeChangedCallback only updates sound/media indicators on .visibleTabs. This should be fine if tabs in collapsed tab groups are not in .visibleTabs since they should not be rendered to the user.
• Browser/components/tabbrowser/content/tabs.js _groupSelectedTabs is indexing into .visibleTabs to determine animation; there may be issues if there are tab groups involved
• BrowserWindowTracker.sys.mjs getAllVisibleTabs
• URILoadingHelper.sys.mjs guessUserContextId —
• tabbrowser.js getTabsToTheStartFrom/getTabsToTheEndFrom uses .visibleTabs, so it will skip tabs in collapsed tab groups that might be in between. Need to decide whether those tabs should be included.
• tabbrowser.js removeAllTabsBut does some handling of pinned and multi selected tabs. Probably does not need special handling for tab groups
• tabbrowser.js removeTab isLastTab is calculated based on .visibleTabs.length == 1. This only seems to affect animation behavior. Tabs that are in collapsed tab groups should probably be counted in this case, but .visibleTabs currently does not include them
• tabbrowser.js _beginRemoveTab decides to close window based on the tab being closed !hidden and there only being 1 visible tab. If there are tabs open in collapsed tab groups, then we should not close the window.
• tabbrowser.js _findTabToBlurTo will only switch to a .visibleTab. This will never blur to a tab in a collapsed tab group. I assume we never want to do that, but maybe there is a use case?
• Tabbrowser.js selectTabAtIndex only considers .visibleTabs.
• tabbrowser.js addRangeToMultiSelectedTabs only considers .visibleTabs. The range will therefore not include tabs in collapsed tab groups, even if those tabs are technically between tab1 and tab2. Do we expect tabs in collapsed tab groups to get selected in this use case?
• tabbrowser.js selectAllTabs only selects .visibleTabs. Do we expect tabs in collapsed tab groups to be selected?
• tabbrowser.js allTabsSelected only checks .visibleTabs. Do we expect tabs in collapsed tab groups to matter for this status check?
• Scenario: 2 standalone tabs, plus 1 tab group with 2 tabs. Select all tabs will select all 4 tabs. If you collapse the tab group, the tabs in the collapsed tab group are still selected. If you uncollapse the tab group, the tabs are still selected as expected. Should they be unselected on collapse?

Comment 2

[Stephen Thompson [:sthompson]]

I think a good number of these will be handled naturally in the course of doing already-planned work.

session restore 1907100

• aboutSessionRestore.js — top.gBrowser.tabs.length == 1, will restore into current window. Does that work if the window has 1 tab in 1 tab group?
• tabbrowser.js createTabsForSessionRestore has special handling for pinned and hidden tabs; probably needs special handling for tab groups

based on the fate of the tab overflow menu

• Browser/components/tabbrowser/TabsList.sys.mjs — populates tabs list based on gBrowser.tabs

Firefox View support 1907102

• Browser/components/firefoxview/opentabs.mjs moveMenuTemplate only uses .visibleTabs to determine whether to give the user options to move to the start and/or end of the tab bar.

tab group drag + drop support 1907101

• tabs.js on_drop adds range to multi-selected tabs based on indexes in gBrowser.tabs — might this pick up tabs from a collapsed tab group in the range? I’m assuming those should not be selected
• tabs.js on_dragend checks tabs.length == 1
• tabbrowser.js moveTabTo indexes into .tabs to determine the neighboring tab. It looks like it might not be handling the case there the neighbor is in a tab group.
• Browser/components/tabbrowser/content/tabs.js _groupSelectedTabs is indexing into .visibleTabs to determine animation; there may be issues if there are tab groups involved

Comment 3

[Stephen Thompson [:sthompson]]

Based on what I've seen, my hunch is that the behavior with .tabs generally works fine without changes.

My hunch is that .visibleTabs should continue not to include tabs in collapsed tab groups. All call sites for .visibleTabs that may be sensitive to collapsed tab groups should check for them. There are a number of places that have additional special handling of .visibleTabs, usually to account for things like pinned tabs. We could have a gBrowser.tabsInCollapsedGroups accessor or, to be more like pinned tabs, a new tab.collapsed property to check on individual tabs.

Some things we probably need to deal with

• browser-ctrlTab.js — filters recently used tabs from gBrowser.tabs based on not closing and not hidden. We need to work with UX to determine the expected behavior of Ctrl + Tab with respect to tab groups. Should Ctrl + Tab go from a standalone tab to the first tab of a tab group or to the next standalone tab? Should that be different if the tab group is collapsed? Should Ctrl + Tab go from the last tab of a tab group to the next tab or should it go to the first tab of the tab group?
• tabs.js _updateHiddenTabsStatus sets “hashiddentabs” if length of .tabs greater than length of .visibleTabs. If tabs are excluded based on both .hidden and .collapsed, then we can't assume there are only "hidden" tabs based on the state of .visibleTabs. This may need to change to check the hidden property on each tab.
• browser-places.js bookmarkTabs is supposed to bookmark all of the visible tabs. It uses .visibleTabs for this. Would we expect this functionality to also bookmark all of the tabs within collapsed tab groups, or not?
• browser-places.js toggleHiddenTabs determines whether the hidden tabs menu should be shown based on .visibleTabs.length < .tabs.length. This would show the hidden tabs menu if there were collapsed tab groups. Should the hidden tabs menu feature also surface tabs in collapsed tab groups?
• Browser/components/firefoxview/opentabs.mjs moveMenuTemplate only uses .visibleTabs to determine whether to give the user options to move to the start and/or end of the tab bar. If the tab bar is a collapsed tab group and then a standalone tab, the standalone tab will be index 0 in .visibleTabs, but I think that tab should still be given the option to move to the start. Similar situation for when the last thing in the tab bar is a collapsed tab group.
• tabbrowser.js getTabsToTheStartFrom/getTabsToTheEndFrom uses .visibleTabs, so it will skip tabs in collapsed tab groups that might be in between. tabbrowser.js selectAllTabs only selects .visibleTabs. tabbrowser.js addRangeToMultiSelectedTabs only considers .visibleTabs. Need to decide whether tabs in collapsed tab groups should be included in selections like this.
• tabbrowser.js selectTabAtIndex indexes into .visibleTabs. browser/components/tabbrowser/content/tabs.js _groupSelectedTabs is indexing into .visibleTabs to determine animation.
• tabbrowser.js updateContextMenu uses .visibleTabs to compute a lot of logic
• tabbrowser.js replaceTabsWithWindow selects all .visibleTabs in the new window. I think this may have odd results if tabs in collapsed tab groups can be selected to be extracted to a new window
• browser.js handleEvent TabSelect makes gBrowser.visibleTabs[0] focusable if Firefox View is open — this skips tabs in collapsed tab groups right now. Not 100% sure what the user scenario is for this logic.
• tabbrowser.js _endRemoveTab updates tabs[I]._tPos indexes on all tabs. I'm not sure if there are edge cases for tab groups. Tabs in collapsed tab groups would be indexed along with all other tabs, so if there is any logic using the _tPos indexes, I am not sure if they are sensitive to the introduction of tab groups, particularly tabs in collapsed tab groups
• tabbrowser.js getDuplicateTabsToClose excludes some tabs (pinned, multi selected) from being treated as duplicates. I would expect tabs in groups not to be treated as duplicates, e.g. the user should be able to have mozilla.com open in each of two different tab groups.
• browser.js warnAboutClosingWindow gives the user a message about the number of tabs that will be closed, but it’s .visibleTabs, so it doesn’t count the tabs in collapsed tab groups. I think this count should include tabs in collapsed tab groups. Honestly, I think it should probably use the length of .tabs, since I assume hidden tabs will also be closed if you close the window. It is possible that users may be confused about the count. Perhaps we should update this message to something like "17 tabs and 2 tab groups will be closed" if there are tab groups present?
• ProfilesParent.sys.mjs receiveMessage Profiles:GetDeleteProfileContent counts tabs as .visibleTabs.length from every window. This seems to get reported to the user on the "delete profile" page. I think this should count all tabs. This is another scenario where it may be helpful to show the number of tab groups to the user in addition to the number of tabs.
• browser-commands.js closeTabOrWindow will select the first non-pinned tab. Should this select the first tab in a tab group if that’s the first non-pinned tab? Should that also happen if the tab group is collapsed?

Comment 4

[Stephen Thompson [:sthompson]]

Worked through many of the UX questions in a shared document with the Mozilla team working on tab groups https://docs.google.com/document/d/1b8QKfAVycSLCzMAqRNvTOVlCGxHFrR04EChzFfdmH1A/edit

I created a number of bugs based on the issues that came out of the UX discussions.

After we triage and prioritize those bugs, I recommend that we figure out as a team the best engineering approach for dealing with .tabs and .visibleTabs. Based on that approach, we can circle back to this audit list and create bugs for any items that need special handling.