Closed Bug 582116 Opened 12 years ago Closed 12 years ago

Provide a way to show certain tabs and get visible tabs

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b4

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file, 11 obsolete files)

Basically a port of bug 579222 and bug 581612 to mozilla-central's tabbrowser.
Attached patch v1 (obsolete) — Splinter Review
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
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #460381 - Flags: review?(gavin.sharp)
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.
Duplicate of this bug: 582302
(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).
(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.
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?
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?
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #460381 - Attachment is obsolete: true
Attachment #461394 - Flags: review?(gavin.sharp)
Attachment #460381 - Flags: review?(gavin.sharp)
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?
Attached patch v3 (obsolete) — Splinter Review
Fix up ctrlTab list ordering with comment! And tabPreviews to only show visible tabs.
Attachment #461394 - Attachment is obsolete: true
Attachment #461613 - Flags: review?(gavin.sharp)
Attachment #461394 - Flags: review?(gavin.sharp)
Comment on attachment 461613 [details] [diff] [review]
v3

Mm seems like while tabbox will skip hidden tabs, tabbrowser doesn't when bluring tabs.
Attachment #461613 - Flags: review?(gavin.sharp)
Attached patch v4 (obsolete) — Splinter Review
Update _blurTab to skip past hidden tabs when picking a new tab.
Attachment #461613 - Attachment is obsolete: true
Attachment #461625 - Flags: review?(gavin.sharp)
-----------------------------
@@ -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>
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.)
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!
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.
Bug 584263 – Need a way to notify changing of tab's visibility
https://bugzilla.mozilla.org/show_bug.cgi?id=584263
(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)?
Actually, let's discuss that in your new bug.
(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.
Attached patch v4.1 (obsolete) — Splinter Review
Port over a fix to the tabbrowser ui from bug 581894.
Attachment #461625 - Attachment is obsolete: true
Attachment #462963 - Flags: review?(gavin.sharp)
Attachment #461625 - Flags: review?(gavin.sharp)
(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.
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.
Attachment #461625 - Attachment is obsolete: false
Attachment #461625 - Flags: review?(dao)
Attachment #462963 - Attachment is obsolete: true
Attachment #462963 - Flags: review?(gavin.sharp)
Attached patch v4.2 (obsolete) — Splinter Review
Some context bitrotted causing it to apply with fuzz.
Attachment #461625 - Attachment is obsolete: true
Attachment #464509 - Flags: review?(dao)
Attachment #461625 - Flags: review?(dao)
Oh and that latest attachment has the fix from bug 581894 removed now that bug 579869 landed into m-c and tabcandy-central.
Attachment #464509 - Attachment description: v4.1 → v4.2
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.
Blocks: 586147
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
Attachment #464509 - Flags: review?(dao) → review-
(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?
What does Ctrl+PageUp/Down do with hidden tabs?
(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 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 :)
(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 on attachment 464509 [details] [diff] [review]
v4.2

># HG changeset patch
># User Edward Lee <edilee@mozilla.com>
># 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.
Ugh, I quoted too much. Last comment was solely about _blurTab.
(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?
Attached patch v5 (obsolete) — Splinter Review
Attachment #464509 - Attachment is obsolete: true
Attachment #464939 - Flags: review?(dao)
Attached patch v5 (obsolete) — Splinter Review
Attachment #464939 - Attachment is obsolete: true
Attachment #464940 - Flags: review?(dao)
Attachment #464939 - Flags: review?(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.
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?
Attached patch v5.1 (obsolete) — Splinter Review
removingTabs.indexOf != -1 --> remainingTabs.indexOf == -1
Attachment #464940 - Attachment is obsolete: true
Attachment #464945 - Flags: review?(dao)
Attachment #464940 - Flags: review?(dao)
Anything can call _blurTab, including code we don't control.
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).
(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.
Attached patch v6 (obsolete) — Splinter Review
Attachment #464945 - Attachment is obsolete: true
Attachment #464984 - Flags: review?(dao)
Attachment #464945 - Flags: review?(dao)
Attachment #464984 - Attachment is obsolete: true
Attachment #464984 - Flags: review?(dao)
Looking at the latest interdiff. Why did you not just add "&& tab != aTab" after removing.indexOf(tab) == -1?
Attached patch v7 (obsolete) — Splinter Review
Still waiting on the build..
Attachment #464999 - Flags: review?(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.
Attachment #464999 - Flags: review?(dao) → review+
Attachment #464999 - Flags: approval2.0?
Comment on attachment 464999 [details] [diff] [review]
v7

a=beltzner
Attachment #464999 - Flags: approval2.0? → approval2.0+
(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
Blocks: 585855
Attached patch v7.1Splinter Review
Attachment #464999 - Attachment is obsolete: true
don't forget to fix beforeselected, afterselected , first-tab, last-tab attributes. Bug 585558
Depends on: 585558
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
Depends on: 586666
Depends on: 609700
Depends on: 667177
Depends on: 667314
No longer depends on: 667177
You need to log in before you can comment on or make changes to this bug.