582116 - Provide a way to show certain tabs and get visible tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Ed Lee :Mardak]

Basically a port of bug 579222 and bug 581612 to mozilla-central's tabbrowser.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Fyi tabcandy guys, you can do this to run one test that you're adding:

TEST_PATH=browser/base/content/test/browser_visibleTabs.js make -C objdir mochitest-browser-chrome

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 460381 [details] [diff] [review]
v1

- I think the tabList and onKeyPress callers of .tabs in browser-tabPreviews.js want .visibleTabs.
- the tabs.length check in replaceTabWithWindow probably want visibleTabs as well, since the goal is to prevent the creation of a window with no (visible) tabs
- showOnlyTheseTabs should probably make sure selectedTab is a visible tab after hiding tabs (otherwise the tabstrip has no tab selected, with the hidden selectedTab's content displayed)
- similarly, it probably shouldn't be possible to close the last visible tab?
- TabContextMenu's updateContextMenu probably also wants visibleTabs

Otherwise this looks good to me, though I suspect there are other edge cases I haven't thought of. It would be good to get Dao to sign off on this as well.

Comment 4

[Ed Lee :Mardak]

(In reply to comment #2)
> - callers of .tabs in browser-tabPreviews.js
I'm not really sure how the behavior of ctrl-tab/tab-previews is supposed to interact with tabcandy/visible tabs. Should the MRU list of tabs only show those that are visible? It should probably still track MRU-ness across all tabs but perhaps only show the set of currently visible tabs?

> - showOnlyTheseTabs should probably make sure selectedTab is a visible tab
> - similarly, it probably shouldn't be possible to close the last visible tab?
Yeah.. Those are currently handled by TabCandy as it has the concepts of what group of tabs to show next. Whereas the showOnlyTheseTabs api only takes an array of tabs with no regard of what the other tabs might be.

There was some discussion about getting groups into tabbrowser for bug 577151. But this is assuming showOnlyTheseTabs works with groups (and groups imply only a subset of visible tabs).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Should the MRU list of tabs only show those that are visible?

I think so, yes.
 
> Yeah.. Those are currently handled by TabCandy as it has the concepts of what
> group of tabs to show next. Whereas the showOnlyTheseTabs api only takes an
> array of tabs with no regard of what the other tabs might be.

I'd like tabbrowser to not depend on the additional TabCandy code. If we need to provide hooks for tabcandy to function correctly, we can do that.

Comment 6

[Ed Lee :Mardak]

Sounds like the desired behavior of tabcandy when closing the last visible tab of a group is to open up tabcandy.

So the question is what's the behavior of tabbrowser by itself when the last visible tab is closed? Is tabbrowser going to be "by itself" even after tabcandy lands?

Comment 7

[Ian Gilman [:iangilman]]

How about if it sends out an event when the last visible tab is closed, and if the event doesn't get handled (say, by Tab Candy), it does some default behavior, such as showing all tabs?

Comment 8

[Ed Lee :Mardak]

Attachment: v2

Makes sure the selected tab stays visible when showOnlyTheseTabs-ing and make sure a tab is visible when closing from the last visible tab. Also update more uses of tabs -> visibleTabs.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 461394 [details] [diff] [review]
v2

>--- a/browser/base/content/browser-tabPreviews.js
>+++ b/browser/base/content/browser-tabPreviews.js

>   get tabList () {
>     if (this._tabList)
>       return this._tabList;
> 
>-    var list = Array.slice(gBrowser.tabs);
>+    let list = gBrowser.visibleTabs;
> 
>     if (this._closing)
>       this.detachTab(this._closing, list);
> 
>     for (let i = 0; i < gBrowser.tabContainer.selectedIndex; i++)
>       list.push(list.shift());

This loop looks wrong.

What happens when a hidden tab gets selected?

Comment 10

[Ed Lee :Mardak]

Attachment: v3

Fix up ctrlTab list ordering with comment! And tabPreviews to only show visible tabs.

Comment 11

[Ed Lee :Mardak]

Comment on attachment 461613 [details] [diff] [review]
v3

Mm seems like while tabbox will skip hidden tabs, tabbrowser doesn't when bluring tabs.

Comment 12

[Ed Lee :Mardak]

Attachment: v4

Update _blurTab to skip past hidden tabs when picking a new tab.

Comment 13

[YUKI "Piro" Hiroshi]

-----------------------------
@@ -744,16 +746,17 @@
              if (!oldBrowser ||
                  (oldBrowser.pageReport && !newBrowser.pageReport) ||
                  (!oldBrowser.pageReport && newBrowser.pageReport))
                updatePageReport = true;

              newBrowser.setAttribute("type", "content-primary");
              this.mCurrentBrowser = newBrowser;
              this.mCurrentTab = this.selectedTab;
+            this.mCurrentTab.hidden = false;
----------------------------

I think this possibly breaks groups of tabs. Modules controlling tab groups doesn't expect that just one tab in a background group becomes shown, so, it is required that a way to notify what's happen to such modules. For example...

----------------------------
-            this.mCurrentTab.hidden = false;
+            this.ensureTabVisible(this.mCurrentTab);
----------------------------

and

<method name="ensureTabVisible">
  <parameter name="aTab"/>
  <body><![CDATA[
    this.showTab(aTab);
  ]]></body>
</method>

<method name="showTab">
  <parameter name="aTab"/>
  <body><![CDATA[
    if (!aTab.hidden) return;
    aTab.hidden = false;
    var event = document.createEvent('Events');
    event.initEvent('TabShown', true, false);
    aTab.dispatchEvent(event);
  ]]></body>
</method>

<method name="hideTab">
  <parameter name="aTab"/>
  <body><![CDATA[
    if (aTab.hidden) return;
    aTab.hidden = true;
    var event = document.createEvent('Events');
    event.initEvent('TabHidden', true, false);
    aTab.dispatchEvent(event);
  ]]></body>
</method>

Comment 14

[YUKI "Piro" Hiroshi]

Oops, this could cause too much recursion... Please ignore the method hideTab, and showTab should be unified to ensureTabVisible. We only need a way to know unexpected showing of tabs from not a group manipulations. (BTW, showOnlyTheseTabs also possibly should dispatch some events from same issue.)

Comment 15

[Aza Raskin [:aza]]

Hey Piro-san, thanks for the patch. Would you mind moving this patch to a new bug? We are worried about adding anything more until after review and landing this. After that, I think we should apply your patch.

Thanks!

Comment 16

[YUKI "Piro" Hiroshi]

Sorry, I thought about it again, and I realized that the worrying is needless. Even without new APIs, when a hidden tab (member of a background group) get focus we can show other members of the group, by TabSelect event.

However, changing of "hidden" attribute of <tab> element can break its DOM structure, so we still need notifications about changing of tab visibility.

Comment 17

[YUKI "Piro" Hiroshi]

Bug 584263 – Need a way to notify changing of tab's visibility
https://bugzilla.mozilla.org/show_bug.cgi?id=584263

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> However, changing of "hidden" attribute of <tab> element can break its DOM
> structure

Break it how? What relies on tab elements having frames (or bindings attached)?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Actually, let's discuss that in your new bug.

Comment 20

[Aza Raskin [:aza]]

(In reply to comment #17)
> Bug 584263 – Need a way to notify changing of tab's visibility
> https://bugzilla.mozilla.org/show_bug.cgi?id=584263

Thanks, Piro-san. Very much appreciated.

Comment 21

[Ed Lee :Mardak]

Attachment: v4.1

Port over a fix to the tabbrowser ui from bug 581894.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Port over a fix to the tabbrowser ui from bug 581894.

Is this just bug 579869? If so, don't fiddle with it here.

Comment 23

[Ed Lee :Mardak]

It might just be bug 579869 depending on the fix for that bug because it's possible that showOnlyTheseTabs will re-introduce that bug. But if the fix is general enough to always make sure app tabs aren't partially covered so that the fix from bug 581894 is unnecessary, then it should be removed yeah.

Comment 24

[Ed Lee :Mardak]

Attachment: v4.2

Some context bitrotted causing it to apply with fuzz.

Comment 25

[Ed Lee :Mardak]

Oh and that latest attachment has the fix from bug 581894 removed now that bug 579869 landed into m-c and tabcandy-central.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao: do you think this patch is close enough that we can get it into beta 4? Not sure how much more work you feel like it needs.

If it's easier to give high level comments than a specific review, feel free to do that.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 464509 [details] [diff] [review]
v4.2

This isn't a complete review. Commenting on issues as I spot them.

>+          if (aIndex >= 0 && aIndex < tabs.length) {
>+            let nextTab = tabs[aIndex];
>+            if (nextTab != this.selectedTab)
>+              this.selectedTab = nextTab;
>+          }

The selectedTab setter is a no-op if the tab is already selected, so you can simplify this.

>             else {
>               let tab = this.childNodes.item(this.tabbrowser._numPinnedTabs);
>+              if (tab && tab.hidden)
>+                tab = this.tabbrowser.visibleTabs[0];
>               if (tab && tab.getBoundingClientRect().width > this.mTabClipWidth)
>                 this.setAttribute("closebuttons", "alltabs");
>               else
>                 this.setAttribute("closebuttons", "activetab");

tab can be pinned with your change, which is wrong

Comment 28

[Ed Lee :Mardak]

(In reply to comment #27)
> >               let tab = this.childNodes.item(this.tabbrowser._numPinnedTabs);
> >+              if (tab && tab.hidden)
> >+                tab = this.tabbrowser.visibleTabs[0];
> tab can be pinned with your change, which is wrong
This should just always grab the last visible tab? It doesn't have to be the first tab after the pinned ones, yeah?

Comment 29

[Dão Gottwald [:dao]]

What does Ctrl+PageUp/Down do with hidden tabs?

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #28)
> (In reply to comment #27)
> > >               let tab = this.childNodes.item(this.tabbrowser._numPinnedTabs);
> > >+              if (tab && tab.hidden)
> > >+                tab = this.tabbrowser.visibleTabs[0];
> > tab can be pinned with your change, which is wrong
> This should just always grab the last visible tab? It doesn't have to be the
> first tab after the pinned ones, yeah?

yep

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 464509 [details] [diff] [review]
v4.2

>+            let ignoreRemoving = function(tab) removing.indexOf(tab) == -1;

function ignoreRemoving(tab) removing.indexOf(tab) == -1;

Also maybe s/ignore/strip/, as arrays don't really have a concept of ignoring something :)

Comment 32

[Ed Lee :Mardak]

(In reply to comment #29)
> What does Ctrl+PageUp/Down do with hidden tabs?
Same thing as Browser:NextTab/PrevTab -> tabbox.xml:advanceSelectedTab -> _selectNewTab. It already skips past hidden tabs. (We changed from collapsed tabs to hidden tabs to leverage this.)

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 464509 [details] [diff] [review]
v4.2

># HG changeset patch
># User Edward Lee <[e-mail]>
># Date 1280446797 25200
># Node ID fc8836588c634ab0e37227f7bf13641194f52404
># Parent  2a0b67004c8d2013ae02f93c42d03fd614e89668
>Bug 582116 - Provide a way to show certain tabs and get visible tabs
>Add showOnlyTheseTabs and visibleTabs to tabbrowser and update various uses such as tab selection. Test that tabs get hidden/shown when using this API and tab selection only deal with visible tabs while making sure there's always a visible tab.
>
>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -475,19 +475,19 @@ var PlacesCommandHook = {
>    * only the first instance of each URI will be returned.
>    *
>    * @returns a list of nsIURI objects representing unique locations open
>    */
>   _getUniqueTabInfo: function BATC__getUniqueTabInfo() {
>     var tabList = [];
>     var seenURIs = {};
> 
>-    var browsers = gBrowser.browsers;
>-    for (var i = 0; i < browsers.length; ++i) {
>-      let uri = browsers[i].currentURI;
>+    let tabs = gBrowser.visibleTabs;
>+    for (let i = 0; i < tabs.length; ++i) {
>+      let uri = tabs[i].linkedBrowser.currentURI;
> 
>       // skip redundant entries
>       if (uri.spec in seenURIs)
>         continue;
> 
>       // add to the set of seen URIs
>       seenURIs[uri.spec] = null;
>       tabList.push(uri);
>diff --git a/browser/base/content/browser-tabPreviews.js b/browser/base/content/browser-tabPreviews.js
>--- a/browser/base/content/browser-tabPreviews.js
>+++ b/browser/base/content/browser-tabPreviews.js
>@@ -210,22 +210,23 @@ var ctrlTab = {
>   get canvasWidth () Math.min(tabPreviews.width,
>                               Math.ceil(screen.availWidth * .85 / this.tabPreviewCount)),
>   get canvasHeight () Math.round(this.canvasWidth * tabPreviews.aspectRatio),
> 
>   get tabList () {
>     if (this._tabList)
>       return this._tabList;
> 
>-    var list = Array.slice(gBrowser.tabs);
>+    let list = gBrowser.visibleTabs;
> 
>     if (this._closing)
>       this.detachTab(this._closing, list);
> 
>-    for (let i = 0; i < gBrowser.tabContainer.selectedIndex; i++)
>+    // Rotate the list until the selected tab is first
>+    while (!list[0].selected)
>       list.push(list.shift());
> 
>     if (this.recentlyUsedLimit != 0) {
>       let recentlyUsedTabs = this._recentlyUsedTabs;
>       if (this.recentlyUsedLimit > 0)
>         recentlyUsedTabs = this._recentlyUsedTabs.slice(0, this.recentlyUsedLimit);
>       for (let i = recentlyUsedTabs.length - 1; i >= 0; i--) {
>         list.splice(list.indexOf(recentlyUsedTabs[i]), 1);
>@@ -457,21 +458,22 @@ var ctrlTab = {
>     switch (event.keyCode) {
>       case event.DOM_VK_TAB:
>         if (event.ctrlKey && !event.altKey && !event.metaKey) {
>           if (isOpen) {
>             this.advanceFocus(!event.shiftKey);
>           } else if (!event.shiftKey) {
>             event.preventDefault();
>             event.stopPropagation();
>-            if (gBrowser.tabs.length > 2) {
>+            let tabs = gBrowser.visibleTabs;
>+            if (tabs.length > 2) {
>               this.open();
>-            } else if (gBrowser.tabs.length == 2) {
>-              gBrowser.selectedTab = gBrowser.selectedTab.nextSibling ||
>-                                     gBrowser.selectedTab.previousSibling;
>+            } else if (tabs.length == 2) {
>+              let index = gBrowser.selectedTab == tabs[0] ? 1 : 0;
>+              gBrowser.selectedTab = tabs[index];
>             }
>           }
>         }
>         break;
>       default:
>         if (isOpen && event.ctrlKey) {
>           if (event.keyCode == event.DOM_VK_DELETE) {
>             this.remove(this.selected);
>@@ -659,26 +661,26 @@ var allTabs = {
> 
>     this._currentFilter = this.filterField.value;
> 
>     var filter = this._currentFilter.split(/\s+/g);
>     this._visible = 0;
>     Array.forEach(this.previews, function (preview) {
>       var tab = preview._tab;
>       var matches = 0;
>-      if (filter.length) {
>+      if (filter.length && !tab.hidden) {
>         let tabstring = tab.linkedBrowser.currentURI.spec;
>         try {
>           tabstring = decodeURI(tabstring);
>         } catch (e) {}
>         tabstring = tab.label + " " + tab.label.toLocaleLowerCase() + " " + tabstring;
>         for (let i = 0; i < filter.length; i++)
>           matches += tabstring.indexOf(filter[i]) > -1;
>       }
>-      if (matches < filter.length) {
>+      if (matches < filter.length || tab.hidden) {
>         preview.hidden = true;
>       }
>       else {
>         this._visible++;
>         this._updatePreview(preview);
>         preview.hidden = false;
>       }
>     }, this);
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -6784,17 +6784,17 @@ var gBookmarkAllTabsHandler = {
>   init: function () {
>     this._command = document.getElementById("Browser:BookmarkAllTabs");
>     gBrowser.tabContainer.addEventListener("TabOpen", this, true);
>     gBrowser.tabContainer.addEventListener("TabClose", this, true);
>     this._updateCommandState();
>   },
> 
>   _updateCommandState: function BATH__updateCommandState(aTabClose) {
>-    var numTabs = gBrowser.tabs.length;
>+    let numTabs = gBrowser.visibleTabs.length;
> 
>     // The TabClose event is fired before the tab is removed from the DOM
>     if (aTabClose)
>       numTabs--;
> 
>     if (numTabs > 1)
>       this._command.removeAttribute("disabled");
>     else
>@@ -7810,17 +7810,17 @@ function switchToTabHavingURI(aURI, aOpe
>   return false;
> }
> 
> var TabContextMenu = {
>   contextTab: null,
>   updateContextMenu: function updateContextMenu(aPopupMenu) {
>     this.contextTab = document.popupNode.localName == "tab" ?
>                       document.popupNode : gBrowser.selectedTab;
>-    var disabled = gBrowser.tabs.length == 1;
>+    let disabled = gBrowser.visibleTabs.length == 1;
> 
>     // Enable the "Close Tab" menuitem when the window doesn't close with the last tab.
>     document.getElementById("context_closeTab").disabled =
>       disabled && gBrowser.tabContainer._closeWindowWithLastTab;
> 
>     var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>     for (var i = 0; i < menuItems.length; i++)
>       menuItems[i].disabled = disabled;
>@@ -7832,17 +7832,17 @@ var TabContextMenu = {
>       getClosedTabCount(window) == 0;
> 
>     // Only one of pin/unpin should be visible
>     document.getElementById("context_pinTab").hidden = this.contextTab.pinned;
>     document.getElementById("context_unpinTab").hidden = !this.contextTab.pinned;
> 
>     // Disable "Close other Tabs" if there is only one unpinned tab and
>     // hide it when the user rightclicked on a pinned tab.
>-    var unpinnedTabs = gBrowser.tabs.length - gBrowser._numPinnedTabs;
>+    let unpinnedTabs = gBrowser.visibleTabs.length - gBrowser._numPinnedTabs;
>     document.getElementById("context_closeOtherTabs").disabled = unpinnedTabs <= 1;
>     document.getElementById("context_closeOtherTabs").hidden = this.contextTab.pinned;
>   }
> };
> 
> XPCOMUtils.defineLazyGetter(this, "HUDConsoleUI", function () {
>   Cu.import("resource://gre/modules/HUDService.jsm");
>   try {
>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -83,16 +83,18 @@
>                 onget="return this.tabContainer.contextMenu;"/>
> 
>       <field name="tabContainer" readonly="true">
>         document.getElementById(this.getAttribute("tabcontainer"));
>       </field>
>       <field name="tabs" readonly="true">
>         this.tabContainer.childNodes;
>       </field>
>+      <property name="visibleTabs" readonly="true"
>+                onget="return Array.filter(this.tabs, function(tab) !tab.hidden);"/>
>       <field name="mURIFixup" readonly="true">
>         Components.classes["@mozilla.org/docshell/urifixup;1"]
>                   .getService(Components.interfaces.nsIURIFixup);
>       </field>
>       <field name="mFaviconService" readonly="true">
>         Components.classes["@mozilla.org/browser/favicon-service;1"]
>                   .getService(Components.interfaces.nsIFaviconService);
>       </field>
>@@ -753,16 +755,17 @@
>                 (oldBrowser.pageReport && !newBrowser.pageReport) ||
>                 (!oldBrowser.pageReport && newBrowser.pageReport))
>               updatePageReport = true;
> 
>             newBrowser.setAttribute("type", "content-primary");
>             newBrowser.docShell.isActive = true;
>             this.mCurrentBrowser = newBrowser;
>             this.mCurrentTab = this.selectedTab;
>+            this.mCurrentTab.hidden = false;
> 
>             if (updatePageReport)
>               this.mCurrentBrowser.updatePageReport();
> 
>             // Update the URL bar.
>             var loc = this.mCurrentBrowser.currentURI;
> 
>             var webProgress = this.mCurrentBrowser.webProgress;
>@@ -1238,17 +1241,17 @@
> 
>       <method name="warnAboutClosingTabs">
>       <parameter name="aAll"/>
>       <body>
>         <![CDATA[
>           var tabsToClose = this.tabs.length;
> 
>           if (!aAll)
>-            tabsToClose -= 1 + gBrowser._numPinnedTabs;
>+            tabsToClose = this.visibleTabs.length - (1 + this._numPinnedTabs);
>           if (tabsToClose <= 1)
>             return true;
> 
>           const pref = "browser.tabs.warnOnClose";
>           var shouldPrompt = Services.prefs.getBoolPref(pref);
> 
>           if (!shouldPrompt)
>             return true;
>@@ -1289,21 +1292,22 @@
>       <method name="removeAllTabsBut">
>         <parameter name="aTab"/>
>         <body>
>           <![CDATA[
>             if (aTab.pinned)
>               return;
> 
>             if (this.warnAboutClosingTabs(false)) {
>+              let tabs = this.visibleTabs;
>               this.selectedTab = aTab;
> 
>-              for (let i = this.tabs.length - 1; i >= 0; --i) {
>-                if (this.tabs[i] != aTab && !this.tabs[i].pinned)
>-                  this.removeTab(this.tabs[i]);
>+              for (let i = tabs.length - 1; i >= 0; --i) {
>+                if (tabs[i] != aTab && !tabs[i].pinned)
>+                  this.removeTab(tabs[i]);
>               }
>             }
>           ]]>
>         </body>
>       </method>
> 
>       <method name="removeCurrentTab">
>         <parameter name="aParams"/>
>@@ -1559,31 +1563,32 @@
> 
>             if (aTab.owner &&
>                 this._removingTabs.indexOf(aTab.owner) == -1 &&
>                 Services.prefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
>               this.selectedTab = aTab.owner;
>               return;
>             }
> 
>+            let removing = this._removingTabs;
>+            let ignoreRemoving = function(tab) removing.indexOf(tab) == -1;
>+
>+            // Switch to a visible tab unless there aren't any remaining
>+            let remainingTabs = this.visibleTabs.filter(ignoreRemoving);
>+            if (remainingTabs.length == 0)
>+              remainingTabs = Array.filter(this.tabs, ignoreRemoving);
>+


>+            // Try to find a remaining tab that comes after the given tab
>             var tab = aTab;
>-
>             do {
>               tab = tab.nextSibling;
>-            } while (tab && this._removingTabs.indexOf(tab) != -1);
>-
>-            if (!tab) {
>-              tab = aTab;
>-
>-              do {
>-                tab = tab.previousSibling;
>-              } while (tab && this._removingTabs.indexOf(tab) != -1);
>-            }
>-
>-            this.selectedTab = tab;
>+            } while (tab && remainingTabs.indexOf(tab) == -1);
>+
>+            // If no tab was found, give the end of the remaining tabs
>+            this.selectedTab = tab ? tab : remainingTabs.pop();

This looks like a behavior change. If so, keep the current algorithm and replace this._removingTabs.indexOf(tab) != -1 with with remainingTabs.indexOf(tab) > -1.

Comment 34

[Dão Gottwald [:dao]]

Ugh, I quoted too much. Last comment was solely about _blurTab.

Comment 35

[Ed Lee :Mardak]

(In reply to comment #33)
> >             do {
> >               tab = tab.nextSibling;
> >-            } while (tab && this._removingTabs.indexOf(tab) != -1);
> >-
> >-            if (!tab) {
> >-              tab = aTab;
> >-
> >-              do {
> >-                tab = tab.previousSibling;
> >-              } while (tab && this._removingTabs.indexOf(tab) != -1);
> >-            }
> >-
> >-            this.selectedTab = tab;
> >+            } while (tab && remainingTabs.indexOf(tab) == -1);
> >+
> >+            // If no tab was found, give the end of the remaining tabs
> >+            this.selectedTab = tab ? tab : remainingTabs.pop();
> This looks like a behavior change.

The original code started at the tab and moved to the next siblings while it still found tabs-being-removed. If nothing is found, it searches previous siblings.

The new code starts at the tab and moves to next siblings until it finds a remaining tab. If it doesn't find any, it'll get the right-most tab of what was previous of the tab.

This should be the same behavior, no?

Comment 36

[Ed Lee :Mardak]

Attachment: v5

Comment 37

[Ed Lee :Mardak]

Attachment: v5

Comment 38

[Dão Gottwald [:dao]]

(In reply to comment #35)
> (In reply to comment #33)
> > >             do {
> > >               tab = tab.nextSibling;
> > >-            } while (tab && this._removingTabs.indexOf(tab) != -1);
> > >-
> > >-            if (!tab) {
> > >-              tab = aTab;
> > >-
> > >-              do {
> > >-                tab = tab.previousSibling;
> > >-              } while (tab && this._removingTabs.indexOf(tab) != -1);
> > >-            }
> > >-
> > >-            this.selectedTab = tab;
> > >+            } while (tab && remainingTabs.indexOf(tab) == -1);
> > >+
> > >+            // If no tab was found, give the end of the remaining tabs
> > >+            this.selectedTab = tab ? tab : remainingTabs.pop();
> > This looks like a behavior change.
> 
> The original code started at the tab and moved to the next siblings while it
> still found tabs-being-removed. If nothing is found, it searches previous
> siblings.
> 
> The new code starts at the tab and moves to next siblings until it finds a
> remaining tab. If it doesn't find any, it'll get the right-most tab of what was
> previous of the tab.
> 
> This should be the same behavior, no?

Except that remainingTabs.pop() can be aTab itself, depending on who's calling _blurTab and when.

Comment 39

[Ed Lee :Mardak]

The tab being removed will be in _removingTabs, so remainingTabs won't have it. But it seems like _blurTab could be called directly from removeTab and skip the _removingTabs stuff?

Comment 40

[Ed Lee :Mardak]

Attachment: v5.1

removingTabs.indexOf != -1 --> remainingTabs.indexOf == -1

Comment 41

[Dão Gottwald [:dao]]

Anything can call _blurTab, including code we don't control.

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 464945 [details] [diff] [review]
v5.1

>   _updateCommandState: function BATH__updateCommandState(aTabClose) {
>-    var numTabs = gBrowser.tabs.length;
>+    let numTabs = gBrowser.visibleTabs.length;
> 
>     // The TabClose event is fired before the tab is removed from the DOM
>     if (aTabClose)
>       numTabs--;

The tab being removed can be a hidden one, right?

>+            let removing = this._removingTabs;
>+            function stripRemoving(tab) removing.indexOf(tab) == -1;
>+
>+            // Switch to a visible tab unless there aren't any remaining
>+            let remainingTabs = this.visibleTabs.filter(stripRemoving);
>+            if (remainingTabs.length == 0)
>+              remainingTabs = Array.filter(this.tabs, stripRemoving);

Also compare with aTab in stripRemoving so that (remainingTabs.length == 0) works without _removingTabs containing aTab. At this point you'll probably want to rename stripRemoving to something like stripUnselectable as well (better suggestions welcome).

Comment 43

[Dão Gottwald [:dao]]

(In reply to comment #42)
> >   _updateCommandState: function BATH__updateCommandState(aTabClose) {
> >-    var numTabs = gBrowser.tabs.length;
> >+    let numTabs = gBrowser.visibleTabs.length;
> > 
> >     // The TabClose event is fired before the tab is removed from the DOM
> >     if (aTabClose)
> >       numTabs--;
> 
> The tab being removed can be a hidden one, right?

In fact this code is already wrong because multiple tabs can close simultaneously. What you should probably do here is drop any _removingTabs from visibleTabs.

Comment 44

[Ed Lee :Mardak]

Attachment: v6

Comment 45

[Dão Gottwald [:dao]]

Looking at the latest interdiff. Why did you not just add "&& tab != aTab" after removing.indexOf(tab) == -1?

Comment 46

[Ed Lee :Mardak]

Attachment: v7

Still waiting on the build..

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 464999 [details] [diff] [review]
v7

Update _canScrollToElement to return false for hidden tabs.

File a follow-up on getting the bookmark-all-tabs handler updated when tabs are hidden or shown.

Again, if you have the intermediate steps as commits, don't push them, as this history isn't going to be useful for tabbrowser.xml, browser.js & co.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 464999 [details] [diff] [review]
v7

a=beltzner

Comment 49

[Ed Lee :Mardak]

(In reply to comment #47)
> File a follow-up on getting the bookmark-all-tabs handler updated when tabs are
> hidden or shown.
There's a bug for the failing test: bug 585855

Comment 50

[Ed Lee :Mardak]

Attachment: v7.1

Comment 51

[tabmix.onemen]

don't forget to fix beforeselected, afterselected , first-tab, last-tab attributes. Bug 585558

Comment 52

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/457ffad14bbd
Add showOnlyTheseTabs and visibleTabs to tabbrowser and update various uses such as tab selection. Test that tabs get hidden/shown when using this API and tab selection only deal with visible tabs while making sure there's always a visible tab.