Closed
Bug 582116
Opened 14 years ago
Closed 14 years ago
Provide a way to show certain tabs and get visible tabs
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 4.0b4
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
Attachments
(1 file, 11 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Basically a port of bug 579222 and bug 581612 to mozilla-central's tabbrowser.
Assignee | ||
Comment 1•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
||
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?
Assignee | ||
Comment 8•14 years ago
|
||
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 9•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
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)
Assignee | ||
Comment 12•14 years ago
|
||
Update _blurTab to skip past hidden tabs when picking a new tab.
Attachment #461613 -
Attachment is obsolete: true
Attachment #461625 -
Flags: review?(gavin.sharp)
Comment 13•14 years ago
|
||
----------------------------- @@ -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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Bug 584263 – Need a way to notify changing of tab's visibility https://bugzilla.mozilla.org/show_bug.cgi?id=584263
Comment 18•14 years ago
|
||
(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•14 years ago
|
||
Actually, let's discuss that in your new bug.
Comment 20•14 years ago
|
||
(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.
Assignee | ||
Comment 21•14 years ago
|
||
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)
Comment 22•14 years ago
|
||
(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.
Assignee | ||
Comment 23•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #461625 -
Attachment is obsolete: false
Attachment #461625 -
Flags: review?(dao)
Assignee | ||
Updated•14 years ago
|
Attachment #462963 -
Attachment is obsolete: true
Attachment #462963 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 24•14 years ago
|
||
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)
Assignee | ||
Comment 25•14 years ago
|
||
Oh and that latest attachment has the fix from bug 581894 removed now that bug 579869 landed into m-c and tabcandy-central.
Assignee | ||
Updated•14 years ago
|
Attachment #464509 -
Attachment description: v4.1 → v4.2
Comment 26•14 years ago
|
||
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•14 years ago
|
||
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-
Assignee | ||
Comment 28•14 years ago
|
||
(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•14 years ago
|
||
What does Ctrl+PageUp/Down do with hidden tabs?
Comment 30•14 years ago
|
||
(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•14 years ago
|
||
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 :)
Assignee | ||
Comment 32•14 years ago
|
||
(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•14 years ago
|
||
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.
Comment 34•14 years ago
|
||
Ugh, I quoted too much. Last comment was solely about _blurTab.
Assignee | ||
Comment 35•14 years ago
|
||
(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?
Assignee | ||
Comment 36•14 years ago
|
||
Attachment #464509 -
Attachment is obsolete: true
Attachment #464939 -
Flags: review?(dao)
Assignee | ||
Comment 37•14 years ago
|
||
Attachment #464939 -
Attachment is obsolete: true
Attachment #464940 -
Flags: review?(dao)
Attachment #464939 -
Flags: review?(dao)
Comment 38•14 years ago
|
||
(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.
Assignee | ||
Comment 39•14 years ago
|
||
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?
Assignee | ||
Comment 40•14 years ago
|
||
removingTabs.indexOf != -1 --> remainingTabs.indexOf == -1
Attachment #464940 -
Attachment is obsolete: true
Attachment #464945 -
Flags: review?(dao)
Attachment #464940 -
Flags: review?(dao)
Comment 41•14 years ago
|
||
Anything can call _blurTab, including code we don't control.
Comment 42•14 years ago
|
||
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•14 years ago
|
||
(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.
Assignee | ||
Comment 44•14 years ago
|
||
Attachment #464945 -
Attachment is obsolete: true
Attachment #464984 -
Flags: review?(dao)
Attachment #464945 -
Flags: review?(dao)
Assignee | ||
Updated•14 years ago
|
Attachment #464984 -
Attachment is obsolete: true
Attachment #464984 -
Flags: review?(dao)
Comment 45•14 years ago
|
||
Looking at the latest interdiff. Why did you not just add "&& tab != aTab" after removing.indexOf(tab) == -1?
Assignee | ||
Comment 46•14 years ago
|
||
Still waiting on the build..
Assignee | ||
Updated•14 years ago
|
Attachment #464999 -
Flags: review?(dao)
Comment 47•14 years ago
|
||
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+
Assignee | ||
Updated•14 years ago
|
Attachment #464999 -
Flags: approval2.0?
Comment 48•14 years ago
|
||
Comment on attachment 464999 [details] [diff] [review] v7 a=beltzner
Attachment #464999 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 49•14 years ago
|
||
(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
Assignee | ||
Comment 50•14 years ago
|
||
Attachment #464999 -
Attachment is obsolete: true
Comment 51•14 years ago
|
||
don't forget to fix beforeselected, afterselected , first-tab, last-tab attributes. Bug 585558
Assignee | ||
Comment 52•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
You need to log in
before you can comment on or make changes to this bug.
Description
•