Closed Bug 670684 Opened 13 years ago Closed 11 years ago

Remove all tabs panel code

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 11 obsolete files)

Attached patch patch (obsolete) — Splinter Review
I see no chance for this to get enabled anymore, so we might as well remove it.
Attachment #545191 - Flags: review?(gavin.sharp)
Assignee: nobody → dao
Attached patch patch (obsolete) — Splinter Review
missed firefox.js
Attachment #545191 - Attachment is obsolete: true
Attachment #545518 - Flags: review?(gavin.sharp)
Attachment #545191 - Flags: review?(gavin.sharp)
Comment on attachment 545518 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser-tabPreviews.js b/browser/base/content/browser-tabPreviews.js

>+    document.getElementById("menu_showAllTabs").hidden =
>+      !enable || !allTabs.canOpen;
> 
>     // Also disable the <key> to ensure Shift+Ctrl+Tab never triggers
>     // Show All Tabs.
>     var key_showAllTabs = document.getElementById("key_showAllTabs");
>     if (enable)

Don't these conditions need to match?

I found a few references that seem to have been missed:
./base/content/browser.css - various allTabs-* references
./themes/winstripe/browser/browser-aero.css:  #allTabs-panel
KUI-panel-closebutton references
Attachment #545518 - Flags: review?(gavin.sharp) → review-
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #2)
> Comment on attachment 545518 [details] [diff] [review] [review]
> patch
> 
> >diff --git a/browser/base/content/browser-tabPreviews.js b/browser/base/content/browser-tabPreviews.js
> 
> >+    document.getElementById("menu_showAllTabs").hidden =
> >+      !enable || !allTabs.canOpen;
> > 
> >     // Also disable the <key> to ensure Shift+Ctrl+Tab never triggers
> >     // Show All Tabs.
> >     var key_showAllTabs = document.getElementById("key_showAllTabs");
> >     if (enable)
> 
> Don't these conditions need to match?

They don't need to match. The key handler will just do nothing if and when the button isn't around.
Attachment #545518 - Attachment is obsolete: true
Attachment #548136 - Flags: review?(gavin.sharp)
Comment on attachment 548136 [details] [diff] [review]
patch v2

It looks like this needs to be updated to account for bug 714281 (allTabs.canOpen won't return the correct value when the button is hidden) and bug 701212. I'll review quickly after that...
Attachment #548136 - Flags: review?(gavin.sharp)
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #548136 - Attachment is obsolete: true
Attachment #618769 - Flags: review?(gavin.sharp)
Comment on attachment 618769 [details] [diff] [review]
patch v3

Review of attachment 618769 [details] [diff] [review]:
-----------------------------------------------------------------

Here's an unbitrotted diff:
http://people.mozilla.org/~fyan/tmp/removealltabs.diff
Attachment #618769 - Flags: feedback+
Please, don't land this! My workflow heavily depends on this feature, and several people I know and told about it, all think it's great. I know it's useless code in the Firefox tree if it isn't used, but this feature just hasn't been given a chance.

I have made a similar comment in bug 515095 comment 9 about browser.ctrlTab.previews, which should be enabled by default in my opinion. This one should, too!

In that comment, I have already mentioned one reason why browser.allTabs.previews should be enabled (the search box), and one why it wouldn't harm even for the people who don't use it. I would like to add:

- The added screenshot really helps to remember which page is inside the tab.

- You don't have to scroll between all open tabs

As far as I know, the only reason to remove it, is "It's not enabled by default, so dead code for most of the users".

Especially in combination with browser.ctrlTab.previews, it's a killer feature. As I asked in the other bug: please reconsider! :)
Attached patch patch v3, updated to tip (obsolete) — Splinter Review
Attachment #618769 - Attachment is obsolete: true
Attachment #618769 - Flags: review?(gavin.sharp)
Attachment #659268 - Flags: review?(gavin.sharp)
Attached patch patch v4 (obsolete) — Splinter Review
Now showing/hiding menu_showAllTabs in a popupshowing event listener, since allTabs.canOpen isn't static.
Attachment #659268 - Attachment is obsolete: true
Attachment #659268 - Flags: review?(gavin.sharp)
Attachment #659270 - Flags: review?(gavin.sharp)
Attached patch patch v4, updated to tip (obsolete) — Splinter Review
Attachment #659270 - Attachment is obsolete: true
Attachment #659270 - Flags: review?(gavin.sharp)
Attachment #660000 - Flags: review?(gavin.sharp)
I’ve been using Dão’s ctrl-tab extension, which provides a panel and the MRU order, and just found that I have an extension installed for the features, both features are in Firefox, and one of the features is disabled by default.

I’ve disabled the extension and enabled browser.ctrlTab.previews, but if this bug is fixed, I’ll have to reverse that?
Attached patch patch v4, updated to tip (obsolete) — Splinter Review
Beyond context differences, the only significant change is an additional removal of "[type=menu]" in pinstripe's browser.css due to HiDPI landing.
Attachment #660000 - Attachment is obsolete: true
Attachment #660000 - Flags: review?(gavin.sharp)
Comment on attachment 695818 [details] [diff] [review]
patch v4, updated to tip

I only found one additional change needed: KUI-close.png is now unused and can be removed.
Attachment #695818 - Flags: review+
Attached patch patch v5 (obsolete) — Splinter Review
This should be ready to land, I think.
https://tbpl.mozilla.org/?tree=Try&rev=ddeec16b17a2
Attachment #695830 - Flags: review+
Some changes are needed to browser/base/content/test/browser_ctrlTab.js.
Attachment #695818 - Attachment is obsolete: true
Attachment #695830 - Attachment is obsolete: true
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> Some changes are needed to browser/base/content/test/browser_ctrlTab.js.

While trying to fix browser_ctrlTab.js, I discovered that the ctrl+tab panel is broken in OS X on Retina Display Macs (HiDPI mode). (It gets stuck after the first press of [tab].) While I verified the reproducibility of this with :felipe and :dolske, they suggested that I get rid of the ctrl tabs panel altogether, since it is a non-default piece of UI that is unlikely to become the default.

tl;dr of below paragraph: I actually like ctrl+tab MRU but only if tab strip itself is also MRU, in which ctrl+tab would both be MRU and intuitively linear, but that idea is a bit too radical for Firefox desktop perhaps.

My thinking around tabbed browsing has evolved over the years, and I actually support the idea of ctrl+tab switching to the most recently selected tab, but only if the tab strip itself reflected this, i.e. the tab strip always kept itself arranged in most recently used order. This would match the ordering of the primary app switchers in Windows 8, iOS, and Android. The difference is that in those platforms, the app switcher (analogous to the tab strip) is not always visible. The tab strip being always visible makes this more problematic, and Firefox desktop is probably not the best place for this kind of experimentation.
Attachment #712320 - Flags: review?(gavin.sharp)
Comment on attachment 712320 [details] [diff] [review]
part 2 to remove ctrl tab code (depends on patch v5)

Please don't hijack my bug :/
Attachment #712320 - Attachment is obsolete: true
Attachment #712320 - Flags: review?(gavin.sharp)
Sorry.
Thanks for updating the patch. I got tired of it after having it kept up to date for a year or so.
Attachment #712318 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/729fe49f9e1f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Now Panorama is getting removed, there is absolutely NO alternative in Firefox to get a visual overview of tabs (not even a less good one). Also, it might not have been enabled by default, but there are sites that wrote articles about it (e.g. http://www.ghacks.net/2009/11/04/tab-preview-features-in-windows-7/, http://www.favbrowser.com/how-to-enable-firefox-3-6-tab-preview/, http://lifehacker.com/5813650/enable-thumbnails-in-firefoxs-list-all-tabs-menu and more), so I'm sure it is better known than you think.
This is not the same thing as Panorama.
I know. Panorama will be removed in bug 836758. I love browser.allTabs.previews for having a nice, graphical overview of my tabs with a search box. I was saying Panorama *could* do this too (although a hell of a lot slower), but without even that alternative, it'll be even worse for me that I lost this functionality. In the end, browser.allTabs.previews is what I really want...
Blocks: 839442
Seriously? It was great feature, and IMO, just as Tim said, it should be enabled by default.
Even if it's not default, it still should be available for more advanced users. That's what was great in Firefox: customizability.
Please, stop making Firefox a simple little browser for older people and little children.
*downgrading*
I agree... this is a great feature! Keep it.. 
Landis.

(In reply to Tim from comment #8)
> Please, don't land this! My workflow heavily depends on this feature, and
> several people I know and told about it, all think it's great. I know it's
> useless code in the Firefox tree if it isn't used, but this feature just
> hasn't been given a chance.
> 
> I have made a similar comment in bug 515095 comment 9 about
> browser.ctrlTab.previews, which should be enabled by default in my opinion.
> This one should, too!
> 
> In that comment, I have already mentioned one reason why
> browser.allTabs.previews should be enabled (the search box), and one why it
> wouldn't harm even for the people who don't use it. I would like to add:
> 
> - The added screenshot really helps to remember which page is inside the tab.
> 
> - You don't have to scroll between all open tabs
> 
> As far as I know, the only reason to remove it, is "It's not enabled by
> default, so dead code for most of the users".
> 
> Especially in combination with browser.ctrlTab.previews, it's a killer
> feature. As I asked in the other bug: please reconsider! :)
(In reply to piotr.kunicki from comment #26)
> Seriously? It was great feature, and IMO, just as Tim said, it should be
> enabled by default.
> Even if it's not default, it still should be available for more advanced
> users. That's what was great in Firefox: customizability.
> Please, stop making Firefox a simple little browser for older people and
> little children.
> *downgrading*

AGREE! Keep Tab Thumbs.
Landis.
Blocks: 844952
Really ?! ... This was so **** useful .... Now I have to search for an addon that is doing the same (if it exists)....
Using FF 21. browser.allTabs.previews;true does not work any more. 

Really this was removed? I used it a lot. 

I'm 50 years old man and I love FF. I hate the lack of configuration that  G chrome has.

Is FF losing its features? The world already has G Chrome (or MSIE), does not need other. The world  already has FF, does not want to loose it.
I too would like this feature back, I agree that panorama was very poor, but this was simple and effective.

I understand the need to clean up a code base but please pass the code on to others so they can create an add on for those of us who want it back.  Just like the plan for panorama.

Or point us to the replacement plan...

Just secretly killing features isn't good enough for mozilla.
(In reply to berryman.michael from comment #31)
> I understand the need to clean up a code base but please pass the code on to
> others so they can create an add on for those of us who want it back.  Just
> like the plan for panorama.

All the removed code is available in the attached patch. Feel free to take it.
This is bad, this was the best feature in firefox. I have more than 200 tabs at once. Since firefox only loads the tab when clicked . I crash my firefox and restore the session back. With this i have 200 tabs but only when i open something using search field from all tabs preview, it will get loaded, does not use any CPU or memory . Chrome will reload every tab when you do a session restore and you know what happens when you restore more than 50 tabs on a midrange laptop. This was the only feature that made me to stick with Firefox, Now i think mozilla wants me to move to chrome. I tried the addon but it does not work. It only generates preview when i click the tab which is bad. This is the best feature compared to tab groups which i think no one uses.
browser.allTabs.previews=true was one of the best features! please return it back!
Hi, This is one of the best features - I cannot live without it and also this is one of the two reasons why I;m not on Chrome. The other is TabMixPlus.

I have just downloaded version 20 and will downgrade.
Please return the best firefox feature - I do not want Chrome :P
Is Mozilla copying the Gnome project roadmap: removing usefull advanced functionnalities without user agreement just because there are not broadly used? It's not an evolution, it's a regression.

Yes, you can make a plugin to retrieve it, but it's not the easy way.

There's in fact only two choices: either you listen to users and restore this, either nobody in the core team wants to care about this piece of code and you definitely remove it.
(In reply to visar.zejnullahu from comment #35)

+1 A little for me and especially for other people.
So if I read this correctly you dumped the great feature of showing thumbnail previews of open tabs?
I heavily rely upon this and thought it was just broke in release 2x.  So no chances of it coming back?

I agree, browser.allTabs.previews=true was one of the best features! please return it back!
I appreciate it's annoying to have to use a plugin for this but the plugin is very good - development and support have been excellent, it includes options for hotkeys, cursor focus location etc, so is now arguably better than the original feature:
https://addons.mozilla.org/en-US/firefox/addon/all-tabs-restorer/
smug_master@hotmail.com, thanks for the add-on, it's almost as good.
Is there any code for this previous feature?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: