Closed Bug 436304 Opened 16 years ago Closed 16 years ago

Implement "All Tabs" panel with previews

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.1b2

People

(Reporter: dao, Assigned: dao)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [icon-3.1])

Attachments

(1 file, 33 obsolete files)

64.13 KB, patch
Details | Diff | Splinter Review
Current plan:

The "All Tabs" panel ...
* is one big grid that shows previews for all tabs from
  all Firefox windows concurrently.
* has a search field that filters the tabs based on their
  title and address.
* replaces the "All Tabs" dropdown list.
* can also be opened with F4.
> * is one big grid that shows previews for all tabs from
>   all Firefox windows concurrently.
IMO, the ability to change sort order and grouping (by recently used, unread, same order as in windows) is necessary if this is done for all windows.

> * has a search field that filters the tabs based on their
>   title and address.
Could Places data (tags, descriptions, etc.) be integrated into this?

> * replaces the "All Tabs" dropdown list.
This looks like it will have almost as much negative reactions as the awesomebar and full page zoom. For example, what if you want to use Fx on your old <=700 MHz, <=512 MiB RAM machine (which is pretty fast with 3.0 unless lots of images are scaled)?

For IE parity: close buttons on each preview.
Depends on: 363861
What about just adding tabs to the awesomebar results, as per the mockups here:
http://madhava.com/egotism/archive/005020.html ?

Seems like it'd fit into users' workflows better. (At least mine.)
(In reply to comment #1)
> IMO, the ability to change sort order and grouping (by recently used, unread,
> same order as in windows) is necessary if this is done for all windows.

I don't think sorting and grouping are core requirements. Using the recently-used-order is an interesting idea, though.

> > * has a search field that filters the tabs based on their
> >   title and address.
> Could Places data (tags, descriptions, etc.) be integrated into this?

Not sure if that's worthwhile or even desirable. How often do you open a bookmark and *stay* on that page? As soon as you navigate to a sub or followup page, you wouldn't find that tab by looking for the tag or description.

> > * replaces the "All Tabs" dropdown list.
> This looks like it will have almost as much negative reactions as the
> awesomebar and full page zoom. For example, what if you want to use Fx on your
> old <=700 MHz, <=512 MiB RAM machine (which is pretty fast with 3.0 unless lots
> of images are scaled)?

I always have a couple of dozen tabs open and my notebook often runs with 600 MHz in its battery saving mode. Things are still workable.

> For IE parity: close buttons on each preview.

No core requirement IMHO but something to keep in mind.

(In reply to comment #2)
> What about just adding tabs to the awesomebar results, as per the mockups here:
> http://madhava.com/egotism/archive/005020.html ?
> 
> Seems like it'd fit into users' workflows better. (At least mine.)

I don't think that's a question of either / or; we could have both. My initial thoughts, biased by the all-tabs panel design, are that:

- it allows you to see only a subset of your tabs and only if you make a query, rather than giving you a complete overview of your tabs that /may/ be narrowed down (personally I don't use the filter feature from the extension, but I know others do),

- doesn't prevent you from creating duplicate tabs by opening bookmarks, following hyperlinks, etc. (so we're missing stuff if we want to "intercede when we see tab duplication beginning -- as a user begins to tell the browser where to go"),

- and that the thumbnails are so tiny that they could be useless (maybe the mockups don't even show thumbnails but generic page icons, I'm not sure about that).
(In reply to comment #0)
> * can also be opened with F4.

A user says: "I have the Mac Slim Keyboard and then Dashboard is coded to F4"

My current plan is to use Ctrl-Alt-T / Cmd-Option-T, similar to <http://blogs.activestate.com/ericp/2008/07/tabhunter---fin.html>.
We could also add Ctrl-Q on Windows for consistency with IE7 Quick Tabs.
Attached patch WIP (obsolete) — Splinter Review
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #330249 - Attachment is obsolete: true
Attachment #330255 - Flags: review?(mconnor)
Comment on attachment 330249 [details] [diff] [review]
WIP

>+      let enumerator = Cc["@mozilla.org/appshell/window-mediator;1"]
>+                         .getService(Ci.nsIWindowMediator)
>+                         .getEnumerator("navigator:browser");
>+      while (enumerator.hasMoreElements()) {
>+        let win = enumerator.getNext();

Application.windows? (not sure it exposes enough stuff to be useful here). There could be a few other FUEL opportunities elsewhere.
Attachment #330249 - Attachment is obsolete: false
Attachment #330249 - Attachment is obsolete: true
Attached patch patch v2 (obsolete) — Splinter Review
forgot to localize a string
Attachment #330255 - Attachment is obsolete: true
Attachment #330259 - Flags: review?(mconnor)
Attachment #330255 - Flags: review?(mconnor)
(In reply to comment #7)
> (From update of attachment 330249 [details] [diff] [review])
> >+      let enumerator = Cc["@mozilla.org/appshell/window-mediator;1"]
> >+                         .getService(Ci.nsIWindowMediator)
> >+                         .getEnumerator("navigator:browser");
> >+      while (enumerator.hasMoreElements()) {
> >+        let win = enumerator.getNext();
> 
> Application.windows? (not sure it exposes enough stuff to be useful here).

Yeah, it's actually the same, except we'd have two loops (fuel creating an array and we using it) instead of one. Doesn't really matter, I guess.
(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 330249 [details] [diff] [review] [details])
> > >+      let enumerator = Cc["@mozilla.org/appshell/window-mediator;1"]
> > >+                         .getService(Ci.nsIWindowMediator)
> > >+                         .getEnumerator("navigator:browser");
> > >+      while (enumerator.hasMoreElements()) {
> > >+        let win = enumerator.getNext();
> > 
> > Application.windows? (not sure it exposes enough stuff to be useful here).
> 
> Yeah, it's actually the same, except we'd have two loops (fuel creating an
> array and we using it) instead of one. Doesn't really matter, I guess.

Err, wait, fuel wraps each of the window objects in a "new Window" thingy, which we don't really need/want...
Attached patch v2, updated and merged (obsolete) — Splinter Review
you need 'git-apply' rather than 'patch' to apply this patch, otherwise the images are missing
Attachment #330259 - Attachment is obsolete: true
Attachment #330363 - Flags: review?(mconnor)
Attachment #330259 - Flags: review?(mconnor)
Attached patch v2, updated (obsolete) — Splinter Review
Attachment #330363 - Attachment is obsolete: true
Attachment #330478 - Flags: review?(mconnor)
Attachment #330363 - Flags: review?(mconnor)
Attached patch patch v3 (obsolete) — Splinter Review
Ctrl+Alt+A isn't available on all keyboard layouts. Now using Shift+Ctrl+A.
Attachment #330478 - Attachment is obsolete: true
Attachment #332253 - Flags: review?(mconnor)
Attachment #330478 - Flags: review?(mconnor)
Attached patch patch v3 (obsolete) — Splinter Review
I accidentally killed some code that I didn't want to kill...
Attachment #332253 - Attachment is obsolete: true
Attachment #332254 - Flags: review?(mconnor)
Attachment #332253 - Flags: review?(mconnor)
Comment on attachment 332254 [details] [diff] [review]
patch v3

There are two questions I have for the moment: 

>   tabPreviews.init();
>   if (gPrefService.getBoolPref("browser.ctrlTab.mostRecentlyUsed"))
>     ctrlTab.init();
>+  allTabs.init();

So allTabs is initialized anyway? JFI what should happen if the pref is set to false? 

>--- a/browser/locales/en-US/chrome/browser/tabbrowser.dtd
>+++ b/browser/locales/en-US/chrome/browser/tabbrowser.dtd
> <!ENTITY  reloadTab.label        "Reload Tab">
> <!ENTITY  reloadTab.accesskey         "r">
> <!ENTITY  listAllTabs.label      "List all tabs">
>+<!ENTITY  listAllTabs.commandkey "a">

Shouldn't we follow the correct case of the label here?
> So allTabs is initialized anyway? JFI what should happen if the pref is set to
> false?

The pref controls whether the panel lists tabs from all windows or from the current window only.

> > <!ENTITY  listAllTabs.label      "List all tabs">
> >+<!ENTITY  listAllTabs.commandkey "a">
> 
> Shouldn't we follow the correct case of the label here?

I don't understand the question.
(In reply to comment #16)
> > > <!ENTITY  listAllTabs.label      "List all tabs">
> > >+<!ENTITY  listAllTabs.commandkey "a">
> > 
> > Shouldn't we follow the correct case of the label here?
> 
> I don't understand the question.

After discussion on IRC I filed that as bug 449114. 

Attached patch patch v4 (obsolete) — Splinter Review
using the search textbox widget
Attachment #332254 - Attachment is obsolete: true
Attachment #332436 - Flags: review?(mconnor)
Attachment #332254 - Flags: review?(mconnor)
Attached image screenshot on Windows (obsolete) —
I was asked to provide a screenshot, here it is.
Attached patch patch v4.1 (obsolete) — Splinter Review
cleaned up some of the magic numbers
Attachment #332436 - Attachment is obsolete: true
Attachment #333223 - Flags: review?(mconnor)
Attachment #332436 - Flags: review?(mconnor)
Attached patch patch v4.1 (obsolete) — Splinter Review
Attachment #333223 - Attachment is obsolete: true
Attachment #333226 - Flags: review?(mconnor)
Attachment #333223 - Flags: review?(mconnor)
Attached patch patch v5 (obsolete) — Splinter Review
- updated to trunk
- in the empty search field, Up/Down/Left/Right keys move focus to the previews
Attachment #333226 - Attachment is obsolete: true
Attachment #334671 - Flags: review?(mconnor)
Attachment #333226 - Flags: review?(mconnor)
Attached image screenshot on Windows (obsolete) —
this is a version using box-shadow
Attached patch patch v5.1 (obsolete) — Splinter Review
with box-shadow

tryserver builds are here:
https://build.mozilla.org/tryserver-builds/2008-08-20_07:18-dgottwald@mozilla.com-try-8094802268a/
Attachment #333219 - Attachment is obsolete: true
Attachment #334671 - Attachment is obsolete: true
Attachment #334746 - Flags: review?(mconnor)
Attachment #334671 - Flags: review?(mconnor)
One comment right at the start: I don't like the fact that, if the textbox is empty, left and right arrows move onto the buttons. Screen reader users who do not have the benefit of a braille display usually use the left and right arrow keys to quickly discover what they recently typed. Throwing them out of the field and onto buttons, then back into the field is not only confusing, but also creates a lot of speech overhead which needs to be processed and slows down workflow performance.

On the positive side: I like the new Search widget. I also like the fact that the buttons are filtered and that they're this easily accessible.
Connor: review ETA or should we try to get someone else?
Attached patch patch v5.2 (obsolete) — Splinter Review
without changing the focus with the left/right keys in the textbox, per comment 25
Attachment #334746 - Attachment is obsolete: true
Attachment #335156 - Flags: review?(mconnor)
Attachment #334746 - Flags: review?(mconnor)
Depends on: 447896
Depends on: 445595
Depends on: 448186
Depends on: 450140
Depends on: 445458
Attached patch patch v5.2 (obsolete) — Splinter Review
updated to trunk
Attachment #335156 - Attachment is obsolete: true
Attachment #336215 - Flags: review?(mconnor)
Attachment #335156 - Flags: review?(mconnor)
Depends on: 453573
Just for information or discussion: This is the way how Google Chrome uses this preview: http://www.google.com/googlebooks/chrome/images/22.jpg
(In reply to comment #29)
> Just for information or discussion: This is the way how Google Chrome uses this
> preview: http://www.google.com/googlebooks/chrome/images/22.jpg

Nope... Google Chrome displays favorite / most visited sites in new tabs (instead of about:blank), similar to Opera's speed dial, <https://addons.mozilla.org/en-US/firefox/addon/8615>, <https://addons.mozilla.org/en-US/firefox/addon/4810> and <https://addons.mozilla.org/en-US/firefox/addon/5721>. This bug is about the tabs that you have currently open and doesn't replace about:blank.
It would be nice if there was an option to drop the screenshots and just show the favicons, page titles, and some indication of what the current page is.  (Basically, the "All Tabs" drop-down as it exists now, but with keyboard shortcuts and last-visited sorting.)

Most webpages look pretty similar, especially if you're browsing multiple pages from the same site, and in these cases screenshots just reduce the signal to noise ratio.  It's even worse in this case, because the favicon is blended into the screenshot.
Attached image Example of Visual Studio's ctrl-tab menu (obsolete) —
I'm not Microsoft apologist, but I really like the way the ctrl-tab menu works in Visual Studio.  In this screenshot, the left highlight is my keyboard cursor (moves up and down with ctrl-tab/ctrl-shift-tab) and the right highlight is under my mouse cursor.  As soon as I let go of ctrl or click on an item, the menu disappears.

The page preview is always determined by the keyboard focus, although with source code the previews are particularly useless.  With this design, though, the previews are easy to ignore.

Visual Studio Express is free, so if you have a Windows machine you can try it out for yourself.
This needs a small tweak now that bug 451015 landed, without level="top" fun things happen when you open the all tabs panel. (you get the alltabs panel but rest of the browser becomes invisible).
No longer depends on: 445458
(In reply to comment #33)
> This needs a small tweak now that bug 451015 landed, without level="top" fun
> things happen when you open the all tabs panel. (you get the alltabs panel but
> rest of the browser becomes invisible).

According to the docs, level="top" must not be used here. The "fun things" should be fixed in the recent nightly (bug 452385).
Attached patch patch v5.2 (obsolete) — Splinter Review
updated to trunk
Attachment #336215 - Attachment is obsolete: true
Attachment #339422 - Flags: review?(mconnor)
Attachment #336215 - Flags: review?(mconnor)
Blocks: 456088
Connor, can we get this reviewed so we can have it land in Beta 1 and get some community feedback?
Target Milestone: --- → Firefox 3.1b1
Whiteboard: [has patch][needs review mconnor]
Is this still going to land if beta 1?   If not, please update the TM field for the version it should hit.   

Requesting blocking-firefox3.1 flag.
Flags: blocking-firefox3.1?
Attached patch patch v5.2 (obsolete) — Splinter Review
updated to trunk
Attachment #339422 - Attachment is obsolete: true
Attachment #341736 - Flags: review?(mconnor)
Attachment #339422 - Flags: review?(mconnor)
Target Milestone: Firefox 3.1b1 → ---
Attached patch patch v5.3 (obsolete) — Splinter Review
I included some of the changes from bug 456088, which simplifies both patches.
Attachment #341736 - Attachment is obsolete: true
Attachment #342063 - Flags: review?(gavin.sharp)
Attachment #341736 - Flags: review?(mconnor)
Whiteboard: [has patch][needs review mconnor]
Blocks: 347930
Hey Dao - can you attach the image you're using for KUI-close.png so I can try applying this patch? Or tell me the dimensions I should be using?
A few comments applying to the latest try-server build (on WinXP):

Functionality:

* All Tabs search doesn't update on Enter. I.e. when searching a tab and quickly hitting Enter, a different tab is selected than when shortly pausing before hitting Enter.

* Clicking on the All Tabs button when the All Tabs panel is already being displayed doesn't dismiss it (opposed to e.g. the Identity Button, the address bar dropdown or bookmark folders on the toolbar).

Layout/Appearance:

* On my second monitor, the All Tabs panel opens aligned towards the primary monitor (e.g. bottom-aligned when the second monitor extends the desktop above or top-left-aligned when it extends it to the right). In case it matters: The second monitor has a higher resolution than the primary one.

* The close buttons above the individual don't disappear (consistently). IMO they should disappear when not hovering near a preview. However they currently only disappear when hovering over an empty spot on the grid (if there is any).

* The panel currently feels somehow foreign on my otherwise transparency-less XP. Hopefully this can be improved during the upcoming theme refresh, though.

Flickering:

* Tabbing out of the search box frequently causes the whole All Tabs panel to flicker (the same flicker also happens when updating/clearing a search).

* The All Tabs panel is displayed before its content is updated which causes unneeded UI movement (and poses a mostly theoretical privacy issue).

* When the panel appears, the word "Search" flickers in the search bar (appears and vanishes in quick succession).
Functionality:

* Esc during a search dismisses the whole panel, opposed to the Downloads manager or the Applications pref pane.

* Overflow isn't handled too nicely - the previews simply bleed over the panel's edge when there's too many of them. What about a scroll bar (leaving the search box in place) so that browsing remains an option?
(In reply to comment #41)
> * On my second monitor, the All Tabs panel opens aligned towards the primary
> monitor

see bug 445765

> * The All Tabs panel is displayed before its content is updated which causes
> unneeded UI movement (and poses a mostly theoretical privacy issue).

I think the panel actually displays outdated content that doesn't exist anymore.

> * When the panel appears, the word "Search" flickers in the search bar (appears
> and vanishes in quick succession).

The emptytext is there mainly for a11y. I wonder if the themes should hide it.

(In reply to comment #42)
> * Overflow isn't handled too nicely - the previews simply bleed over the
> panel's edge when there's too many of them.

This shouldn't happen. How many tabs did you have?
Depends on: 445765
(In reply to comment #43)
> The emptytext is there mainly for a11y. I wonder if the themes should hide it.

Would adding the emptytext attribute onfocus be too late for a11y (so that it's visible when you tab out)? Then again, most of the other search fields don't really advertise themselves, either.

> This shouldn't happen. How many tabs did you have?

Quite a lot. It's broken the first time between 57 and 64, then again from 73 on, from 101 on, etc. After 101, I infrequently get to see all the tabs, but the page titles of the last line are consistently cut off. And somewhere after 500 tabs there seem to be other artifacts... ;-)
(In reply to comment #40)
> Hey Dao - can you attach the image you're using for KUI-close.png so I can try
> applying this patch? Or tell me the dimensions I should be using?

Please use 'hg import --no-commit FILE' instead of 'patch'.
There are also new tryserver builds with the patch for this bug, bug 456088 and an almost-complete patch for bug 347930:
https://build.mozilla.org/tryserver-builds/2008-10-08_08:15-dgottwald@mozilla.com-try-d5c69b6eb1f/

(In reply to comment #44)
> (In reply to comment #43)
> > The emptytext is there mainly for a11y. I wonder if the themes should hide it.
> 
> Would adding the emptytext attribute onfocus be too late for a11y (so that it's
> visible when you tab out)?

Not sure. A tooltip would probably be better anyway.
Running the tryserver build from Comment 45 on Mac, I opened up the BBC news feed in tabs. I did a little testing, left my machine idle, and then came back.  Suddenly I hit an issue where clicking on the various tabs (while not in preview) did not move them - menus were frozen, etc but the application was not hung to the point that it was indicating I needed to force quit.  I was using a new profile. Activity monitor showed CPU at 100%.
Attachment #342063 - Flags: review?(gavin.sharp) → review?(mconnor)
(In reply to comment #46)
> Running the tryserver build from Comment 45 on Mac, I opened up the BBC news
> feed in tabs. I did a little testing, left my machine idle, and then came back.
>  Suddenly I hit an issue where clicking on the various tabs (while not in
> preview) did not move them - menus were frozen, etc but the application was not
> hung to the point that it was indicating I needed to force quit.  I was using a
> new profile. Activity monitor showed CPU at 100%.

The mentioned patch for bug 347930 could have caused this, as it wasn't finished at that time. I haven't seen that myself, though.
I've tested the try server build on OS X and also cannot see the hang while the all tab panel is open.

Further I noticed following issues:

* If you are using the mouse, hovered tabs are getting a blue outline. If you leave the mouse over a tab and use the tab key to navigate through the list of tabs, an additionally blue outline is shown.

* Navigating by using the keyboard (tab, up, down, left, right keys) shows a really small but noticeable lag before the focus is switched to the next tab.  

* Pressing up or down while having the focus inside the search textbox we leave the box. Is that really volitional?

* Hitting Esc while having the focus inside the search textbox closes the "All tabs" preview. To be consistent with other dialogs shouldn't we only blur the textbox?

* Adding about 30 new tabs and reopening the panel shows a big white tab at the bottom before re-arranging all the tabs. 

* Why we have such a big close button?
(In reply to comment #48)
> * Pressing up or down while having the focus inside the search textbox we leave
> the box. Is that really volitional?

I'd prefer we won't leave the box at all when arrow keys are being used.

> * Hitting Esc while having the focus inside the search textbox closes the "All
> tabs" preview. To be consistent with other dialogs shouldn't we only blur the
> textbox?

Do you still have text inside the textbox when you hit Escape? If so, hitting Escape should take away the text but leave the panel open. A second press of Escape, or hitting Escape when there's no text inside, should close the panel.
(In reply to comment #49)
> Do you still have text inside the textbox when you hit Escape? If so, hitting
> Escape should take away the text but leave the panel open. A second press of
> Escape, or hitting Escape when there's no text inside, should close the panel.

Yes, there was some text inside the textbox when I've pressed Esc. Sorry, but forgot to mention that and we don't blur but clear the text in all the other search textboxes.
It's great that you all provide feedback. However, the reason why I don't respond to all points or update the patch accordingly is that these are mostly minor issues that can be ironed out once this patch has finally landed, which I hope to be soon. Trying to make a decision on every single aspect of the UI and to fix all tiny glitches now would only slow us down, I believe. So once landed, please make sure to file separate bugs on these things.
Attached patch patch v5.3 (obsolete) — Splinter Review
updated to trunk
Attachment #342063 - Attachment is obsolete: true
Attachment #342805 - Flags: review?(mconnor)
Attachment #342063 - Flags: review?(mconnor)
Attachment #342805 - Flags: review?(mconnor)
Comment on attachment 342805 [details] [diff] [review]
patch v5.3

so, I didn't see the new version of the patch, this is based on the pre-update review.  Would like to see another rev with comments addressed.

also, in the latest tryserver build this doesn't have the same aero glass styling as ctrl+tab, please fix that as well.

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

>   capture: function (aTab, aStore) {
>+    if (!this.width)
>+      this.init();
>+

is this change related?  It doesn't seem so at first glance.

>+  get firstVisibleBox () {
>+    if (!this.visible)
>+      return null;
>+    var boxes = this.container.childNodes;
>+    for (let i = 0; i < boxes.length; i++) {
>+      if (!boxes[i].collapsed)
>+        return boxes[i];
>+    }
>+    return null;
>+  },
>+  get lastVisibleBox () {
>+    var boxes = this.container.childNodes;
>+    for (let i = boxes.length - 1; i >= 0; i--) {
>+      if (!boxes[i].collapsed)
>+        return boxes[i];
>+    }
>+    return null;
>+  },

nit: prefix these with _ since they're clearly just there for internal impl, not really for external use.  the ones below probably as well.

really, anything you don't want to guarantee will be available to external callers should be prefixed thusly, IMO.

>+  get maxWidth () screen.availWidth * .9,
>+  get maxHeight () screen.availHeight * .7,

I kinda want this whole set of convenience methods/pseudo-consts at the end of the object, but that's probably inconsistent with prevailing style...


>+  watchTabEvents: function (win, start) {
>+    var tabContainer = win.gBrowser.tabContainer;
>+    if (start) {
>+      tabContainer.addEventListener("TabOpen", this, false);
>+      tabContainer.addEventListener("TabMove", this, false);
>+      tabContainer.addEventListener("TabClose", this, false);
>+    } else {
>+      tabContainer.removeEventListener("TabOpen", this, false);
>+      tabContainer.removeEventListener("TabMove", this, false);
>+      tabContainer.removeEventListener("TabClose", this, false);
>+    }
>+  },
>+  pick: function (aBox) {
>+    if (!aBox)
>+      aBox = this.firstVisibleBox;

nit: newline here

>+    var win;
>+    if (aBox) {
>+      win = aBox._tab.ownerDocument.defaultView;
>+      win.gBrowser.selectedTab = aBox._tab;
>+    }
>+    this._activeWin = win;

This means _activeWin is set to undefined if !aBox.  Do all of the callers cope well with this case?

>+  filter: function () {

>+      } else {

style nit:

}
else {


>+  reflow: function () {
>+    this.updateTabCloseButton();
>+
>+    // the size of the box relative to the preview image
>+    const REL_BOX_THUMBNAIL = 1.2;
>+
>+    var maxHeight = this.maxHeight;
>+    var maxWidth = this.maxWidth;
>+    var rel = tabPreviews.height / tabPreviews.width;
>+    var rows, boxHeight, boxWidth;
>+    var boxMaxWidth = tabPreviews.width * REL_BOX_THUMBNAIL;
>+    this.columns = Math.floor(maxWidth / boxMaxWidth);
>+    do {
>+      rows = Math.ceil(this.visible / this.columns);
>+      boxWidth = maxWidth / this.columns;
>+      boxHeight = boxWidth * rel;
>+      this.columns++;
>+    } while (rows * Math.round(boxHeight) + this.boxLabelHeight > maxHeight);

If we're doing this rounding here, and I assume the layout engine does the same, why not just set boxHeight to the rounded value instead of the float value?

>+    this.columns--;

this whole thing seems like there should be a better way of figuring out the number of rows/columns that will fit.  This works, and seems acceptably fast, if a little hard to parse at first.

>+    if (boxWidth > boxMaxWidth) {
>+      boxWidth = boxMaxWidth;
>+      boxHeight = rel * boxWidth;
>+    }

to save my brain, how does this case get hit?

>+    var outerWidth = boxWidth;
>+    var outerHeight = boxHeight + this.boxLabelHeight;
>+    var innerWidth = boxWidth / REL_BOX_THUMBNAIL;
>+    var innerHeight = boxHeight / REL_BOX_THUMBNAIL;
>+    {
>+      let verticalPadding = boxHeight - innerHeight;
>+      let paddingTop = verticalPadding * .7;
>+      var boxPadding = paddingTop + "px " + (boxWidth - innerWidth) / 2 + "px 0";
>+      var boxPaddingBottom = (verticalPadding - paddingTop) + "px";
>+    }

what's the point of this block being in this style?  seems a little like "because I can"


>+  addBox: function (aTab) {


if we need to nullcheck aBox in removeBox, why not aTab in addBox?

>+  removeBox: function (aBox) {
>+    if (aBox) {

>+    }
>+  },

should dump something to console if !aBox here, because a caller's screwing up if so.

>+  getBox: function (aTab) {
>+    var boxes = this.container.childNodes;
>+    for (let i = 0; i < boxes.length; i++)
>+      if (boxes[i]._tab == aTab)
>+        return boxes[i];
>+    return null;
>+  },
>+  getLabel: function (aBox) {
>+    return aBox.lastChild;
>+  },
>+  getThumbnail: function (aBox) {
>+    return aBox.firstChild;
>+  },

I kinda want these to be more explicitly named (getBoxForTab, etc) but I'm not sure if it matters.  If they're meant for consumers outside of the object, I'd tend to think "more obvious" otherwise mark them as internal and it doesn't matter

>+  updatePreview: function (aBox) {
>+    var label = this.getLabel(aBox);
>+    label.setAttribute("flex", 1);
>+    label.setAttribute("crop", aBox._tab.crop);
>+    label.setAttribute("value", aBox._tab.label);
>+    aBox.setAttribute("tooltiptext", aBox._tab.label);
>+
>+    this.getThumbnail(aBox).setAttribute("src", tabPreviews.get(aBox._tab));
>+  },

this seems to overlap a lot with tabAttrChanged...

>+  open: function (aToggle) {
>+    if (this.panel.state == "open" || this.panel.state == "showing") {
>+      if (aToggle)
>+        this.close();
>+      return;
>+    }

aToggle is creating a bug.  if I click on the all tabs button again, the preview pane goes away and comes back.  It should work like the location bar or larry and dismiss on a second click.

>+    if (gPrefService.getBoolPref("browser.allTabs.allWindows")) {
>+      let enumerator = Cc["@mozilla.org/appshell/window-mediator;1"]
>+                         .getService(Ci.nsIWindowMediator)
>+                         .getEnumerator("navigator:browser");

if you're using the Cc prefix, periods are trailing and the new lines align with Cc.

>+  updateTabCloseButton: function (event) {
>+    if (event && event.target.parentNode == this.container) {
>+      let anchor = this.getThumbnail(event.target).boxObject;
>+      this.tabCloseButton.left =
>+        anchor.screenX - this.container.boxObject.screenX +
>+        anchor.width + +this.container.left;
>+      this.tabCloseButton.top =
>+        anchor.screenY - this.container.boxObject.screenY -
>+        this.tabCloseButton.boxObject.height;

these are kinda hard to read.  you also have an extra + that snuck in.

I'd rather do one-per-line when we're doing stuff like this, like so:

      this.tabCloseButton.left = anchor.screenX - 
                                 this.container.boxObject.screenX +
                                 anchor.width +
                                 this.container.left;

>+      this.tabCloseButton._tab = event.target._tab;
>+      this.tabCloseButton.style.visibility = "visible";
>+    } else {
>+      this.tabCloseButton.style.visibility = "hidden";
>+      this.tabCloseButton.left = this.tabCloseButton.top = 0;
>+      this.tabCloseButton._tab = null;
>+    }
>+  },

actually, where is this close button?  I don't see it in the tryserver builds... maybe this is dead code now?



>+  handleEvent: function (event) {
>+    switch (event.type) {
>+      case "DOMAttrModified":
>+        this.tabAttrModified(event.target, event.attrName);
>+        break;
>+      case "TabOpen":
>+        if (this.panel.state == "open" || this.panel.state == "showing")
>+          this.close();
>+        this.addBox(event.target);
>+        break;
>+      case "TabSelect":
>+      case "TabMove":
>+        if (event.target.nextSibling)
>+          this.container.insertBefore(this.getBox(event.target),
>+                                      this.getBox(event.target.nextSibling));
>+        else
>+          this.container.appendChild(this.getBox(event.target));
>+        break;
>+      case "TabClose":
>+        this.removeBox(this.getBox(event.target));
>+        break;
>+      case "keypress":
>+        this.onKeyPress(event);
>+        break;
>+      case "popupshown":
>+        this.filterField.focus();
>+        break;
>+      case "popuphidden":
>+        this.onPopupHidden();
>+        break;
>+    }
>+  }
>+};

can we actually get tabmove events with the panel up?



>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
>@@ -1120,16 +1120,17 @@ function delayedStartup(isLoadingBlank, 
> 
>   // Attach a listener to watch for "command" events bubbling up from error
>   // pages.  This lets us fix bugs like 401575 which require error page UI to
>   // do privileged things, without letting error pages have any privilege
>   // themselves.
>   gBrowser.addEventListener("command", BrowserOnCommand, false);
> 
>   tabPreviews.init();
>+  allTabs.init();

do we need to do this during startup?  Can we just have open() call init() conditionally?  Seems like until the user actually invokes this we can save a bunch of overhead observing stuff.

>+        <toolbarbutton id="allTabs-tab-close-button"
>+                       class="tab-close-button"
>+                       oncommand="allTabs.closeTab(event);"
>+                       tooltiptext="&closeTab.label;"
>+                       style="visibility:hidden"/>

so I asked this already, where does this show up?


>+            <xul:toolbarbutton class="tabs-alltabs-button" anonid="alltabs-button"
>+                               tooltiptext="&listAllTabs.label;"
>+                               oncommand="allTabs.open();"/>

thinking we should change the class here so themers do something more appropriate, open to arguments for keeping it the same.


>diff --git a/browser/locales/en-US/chrome/browser/tabbrowser.dtd b/browser/locales/en-US/chrome/browser/tabbrowser.dtd

>+<!ENTITY  listAllTabs.commandkey "a">
>+<!ENTITY  listAllTabsSearch.emptytext "Search">

This alignment makes me sad.  As much as I generally hate whitespace changes, can you just clean this up?  Its getting out of hand.
Attached patch patch v6 (obsolete) — Splinter Review
(In reply to comment #53)
> also, in the latest tryserver build this doesn't have the same aero glass
> styling as ctrl+tab, please fix that as well.

This would require bug 457997 to be fixed.

> >+    var win;
> >+    if (aBox) {
> >+      win = aBox._tab.ownerDocument.defaultView;
> >+      win.gBrowser.selectedTab = aBox._tab;
> >+    }
> >+    this._activeWin = win;
> 
> This means _activeWin is set to undefined if !aBox.  Do all of the callers cope
> well with this case?

yes

> >+    if (boxWidth > boxMaxWidth) {
> >+      boxWidth = boxMaxWidth;
> >+      boxHeight = rel * boxWidth;
> >+    }
> 
> to save my brain, how does this case get hit?

boxWidth is calculated this way:

columns = Math.floor(maxWidth / boxMaxWidth);
boxWidth = Math.round(maxWidth / columns);

If you try this with maxWidth = 1000 and boxMaxWidth = 190, you get a boxWidth of 200. And if this doesn't cause vertical overflow, that's the boxWidth you'd have after the loop.

This can also be handled within the loop:

Math.min(boxMaxWidth, Math.round(maxWidth / this.columns));

> >+    var outerWidth = boxWidth;
> >+    var outerHeight = boxHeight + this.boxLabelHeight;
> >+    var innerWidth = boxWidth / REL_BOX_THUMBNAIL;
> >+    var innerHeight = boxHeight / REL_BOX_THUMBNAIL;
> >+    {
> >+      let verticalPadding = boxHeight - innerHeight;
> >+      let paddingTop = verticalPadding * .7;
> >+      var boxPadding = paddingTop + "px " + (boxWidth - innerWidth) / 2 + "px 0";
> >+      var boxPaddingBottom = (verticalPadding - paddingTop) + "px";
> >+    }
> 
> what's the point of this block being in this style?  seems a little like
> "because I can"

The vars are used below, the lets aren't. This information helps e.g. if you refactor this code as I've done a few times.

> actually, where is this close button?  I don't see it in the tryserver
> builds... maybe this is dead code now?

fallout from bug 347930

> can we actually get tabmove events with the panel up?

Probably hard to achieve (would likely be a bug), unless drag & drop gets implemented in the panel.

> >+            <xul:toolbarbutton class="tabs-alltabs-button" anonid="alltabs-button"
> >+                               tooltiptext="&listAllTabs.label;"
> >+                               oncommand="allTabs.open();"/>
> 
> thinking we should change the class here so themers do something more
> appropriate, open to arguments for keeping it the same.

This would change with bug 347930 anyway.
Attachment #342805 - Attachment is obsolete: true
Attachment #342878 - Flags: review?(mconnor)
Attached patch patch v6 (obsolete) — Splinter Review
updated to trunk
Attachment #342878 - Attachment is obsolete: true
Attachment #343231 - Flags: review?(mconnor)
Attachment #342878 - Flags: review?(mconnor)
Depends on: 460400
can we get this patch reviewed?  We'd love to have this into the nightlies as soon as possible so we can start testing.   Thanks.
Attachment #343231 - Flags: review?(gavin.sharp)
with the latest tryserver build (cd476cc0dd1) i get some graphical glitches after closing the all tab panel
(a) via ESC when using search box
(b) via the the top right "x"
(see screenshot, Windows XP (classic theme))
=> new bug?
Attached patch v6 updated to trunk (obsolete) — Splinter Review
Attachment #343231 - Attachment is obsolete: true
Attachment #344618 - Flags: review?(mconnor)
Attachment #343231 - Flags: review?(mconnor)
Attachment #343231 - Flags: review?(gavin.sharp)
Attachment #344618 - Flags: review?(gavin.sharp)
(In reply to comment #57)
> Created an attachment (id=344609) [details]
> graphical glitches after closing the panel
> 
> with the latest tryserver build (cd476cc0dd1) i get some graphical glitches
> after closing the all tab panel
> (a) via ESC when using search box
> (b) via the the top right "x"
> (see screenshot, Windows XP (classic theme))
> => new bug?

Yes, please file a new bug... not sure about the component, I'd start with "GFX: Win32", but Core in any case.
Attachment #344618 - Flags: review?(mconnor)
Attachment #344618 - Flags: review?(gavin.sharp)
No longer blocks: 456088
Depends on: 456088
Attached patch patch (obsolete) — Splinter Review
Attachment #334677 - Attachment is obsolete: true
Attachment #337581 - Attachment is obsolete: true
Attachment #344609 - Attachment is obsolete: true
Attachment #344618 - Attachment is obsolete: true
Attachment #345203 - Flags: review?(mconnor)
Attached patch patch -- search added (obsolete) — Splinter Review
Attachment #345203 - Attachment is obsolete: true
Attachment #345733 - Flags: review?(mconnor)
Attachment #345203 - Flags: review?(mconnor)
Attachment #345733 - Attachment is obsolete: true
Attachment #345769 - Flags: review?(mconnor)
Attachment #345733 - Flags: review?(mconnor)
Mconnor, could you please give a status update for this bug? The freeze is coming soon and QA needs some time to run verification tests on this feature. Even none of the patches were reviewed yet. Doing so will probably cause remaining work which has to be done asap.

Thanks!
Whiteboard: [needs status update]
Attachment #345769 - Attachment is obsolete: true
Attachment #345848 - Flags: review?(mconnor)
Attachment #345769 - Flags: review?(mconnor)
No longer depends on: 460400
Comment on attachment 345848 [details] [diff] [review]
patch -- made Ctrl+F in Ctrl+Tab switch to All Tabs and focus the search field

In general, wouldn't you want to use toLocaleLowerCase instead of toLowerCase on strings?

Also, please use named functions.  Anonymous functions are painful to debug, use dtrace, etc.

>+          else if (gBrowser.mTabs.length == 2)
>+            gBrowser.selectedTab = gBrowser.selectedTab.nextSibling ||
>+                                   gBrowser.selectedTab.previousSibling;
>+          else if (gBrowser.mTabs.length > 2 && this._handleCtrlTab)
nit: can you please add braces here since you have more than one line of code.  That means all the other else if's need braces too.

>       case "keydown":
>       case "keyup":
>+        if (event.target == this.searchField) {
>+          if (event.keyCode == event.DOM_VK_RETURN || event.keyCode == event.DOM_VK_ESCAPE)
>+            this.panel.focus();
>+        } else {
>+          // the panel is open; don't propagate any key events
>+          event.stopPropagation();
>+          event.preventDefault();
nit: comment is inaccurate.  We don't propagate any keyup or keydown events.

r=sdwilsh with the above fixed or addressed.
Attachment #345848 - Flags: review+
(In reply to comment #65)
> (From update of attachment 345848 [details] [diff] [review])
> In general, wouldn't you want to use toLocaleLowerCase instead of toLowerCase
> on strings?

I'm not sure in which cases this makes a difference, but I guess it can't hurt.

> >+          // the panel is open; don't propagate any key events
> >+          event.stopPropagation();
> >+          event.preventDefault();
> nit: comment is inaccurate.  We don't propagate any keyup or keydown events.

Actually the given patch did, but explicitly focusing the panel fixes that.
Attachment #345848 - Attachment is obsolete: true
Attachment #345903 - Flags: review?(mconnor)
Attachment #345848 - Flags: review?(mconnor)
(In reply to comment #66)
> (In reply to comment #65)
[...]
> > In general, wouldn't you want to use toLocaleLowerCase instead of toLowerCase
> > on strings?
> 
> I'm not sure in which cases this makes a difference, but I guess it can't hurt.
[...]

What about Turkish, where dotted İi is one letter and undotted Iı is another, so that i and I are not each other's case complements in that locale?
Attachment #345903 - Flags: review?(mconnor) → review+
Comment on attachment 345903 [details] [diff] [review]
patch -- review comments addressed

code-wise, this looks fine.  really needs chrome tests once we're settled on impl details.
Attachment #345903 - Attachment is obsolete: true
Whiteboard: [needs status update] → [needs status update][icon-3.1]
So, I made a build of this with bug 456088 applied (I think it has to be?).  The feature seems to be broken though - it doesn't always seem to work, and sometimes I could get more than one tab to be highlighted.
(In reply to comment #71)
> So, I made a build of this with bug 456088 applied (I think it has to be?). 
> The feature seems to be broken though - it doesn't always seem to work, and
> sometimes I could get more than one tab to be highlighted.

Shawn, see my comment 48 on this bug which lists issues which aren't working quiet well. It's time to file them as new bugs now or do you want to have them fixed with this bug?
(In reply to comment #71)
> The feature seems to be broken though - it doesn't always seem to work

I think we're facing some other breakage on trunk. Sometimes I can't use any keyboard shortcut, independently from these patches.

> sometimes I could get more than one tab to be highlighted.

Maybe you're seeing the mouse hover feedback?
per boriss
Attachment #346083 - Attachment is obsolete: true
(In reply to comment #73)
> (In reply to comment #71)
> > The feature seems to be broken though - it doesn't always seem to work
> 
> I think we're facing some other breakage on trunk. Sometimes I can't use any
> keyboard shortcut, independently from these patches.

And further I think this has something to do with session restore, as restoring a session seems to be the situation where I see the failure...
I've backed this out due to the test failures on linux.
I emailed a few notes, I'll put them here too:

I'm running the latest build from Bug 436304 and Bug 456088 and see a few
problems.  Here's the three biggest ones:

1. Control Tab and All Tabs are still separate items.  I believe mconnor made
the call that we were creating only one preview mode.  Since both now have 12
tabs, the I think the distinction between them makes even less sense now.  The
design that is posed to land is one Control+Tab with a search bar and
pagination and 12 previews.

2. When the preview panes appear and disappear is broken.  Right now
Control+Tab sometimes disappears when you let go of the keys, and sometimes
not.  All tabs stays until a selection is made.  Here is my recommendation,
assuming only one Control+Tab panel:

- If Control+Tab is called via the chrome button, it is permanent until a
selection is made or the user clicks away from the screen.  

- If the user calls the window with Control+Tab, it is temporary and is
dismissed if the keys are released.  This is true unless the user begins typing
or moves the cursor over the window, in which case the window becomes permanent
unless a selection is made or the user clicks off the screen.

3. If the user presses Control+Tab and then Tabs to a preview and releases,
Control+Tab should go to that window and be dismissed.  Right now, often
nothing happens if the user tabs over.
Attached patch mac keyup issue fixed (obsolete) — Splinter Review
This should fix the "panel persists after releasing Ctrl" issue that several people have reported. Thanks to mstange for helping me narrow this down.
Attachment #346225 - Attachment is obsolete: true
Attached patch for checkinSplinter Review
not sure if I'll be around when this can land...
Attachment #346309 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs status update][icon-3.1] → [icon-3.1]
to clarify: comment 78 refers to the problem that I think for example Shawn (comment 71) and Jenny (comment 77) were seeing. It's unrelated to what I mentioned in comment 75.
http://hg.mozilla.org/mozilla-central/rev/bf2f8ad8f1ce
Keywords: checkin-needed
Target Milestone: --- → Firefox 3.1b2
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 463213
Is there a keyboard shortcut to open the All Tabs panel? I didn't find one skimming through the patch.

I noticed in a previous version of the patch that is was possible to search for tabs from all windows (browser.allTabs.allWindows). Is it planned to add such a feature?
(In reply to comment #82)
> Is there a keyboard shortcut to open the All Tabs panel? I didn't find one
> skimming through the patch.

Ctrl+Tab basically. I think we should also to add Ctrl+Q on Windows.

> I noticed in a previous version of the patch that is was possible to search for
> tabs from all windows (browser.allTabs.allWindows). Is it planned to add such a
> feature?

As far as I know, that's not planned.
(In reply to comment #83)
> Ctrl+Tab basically. I think we should also to add Ctrl+Q on Windows.

yes thanks. However my use case is to find a given tab by typing part of its name without using the mouse. So I was looking for a shortcut to open the All Tabs panel in sticky mode (or being able to focus the search field using keyboard after typing Ctrl+Tab).


(In reply to comment #49)
> (In reply to comment #48)
> > * Pressing up or down while having the focus inside the search textbox we leave
> > the box. Is that really volitional?
> 
> I'd prefer we won't leave the box at all when arrow keys are being used.

ok, but then it is not possible to select one of the thumbnail using keyboard once the search textbox is focused. Or am I missing something? Maybe the TAB key should still let the focus get out of the search textbox in order to select one of the thumbnail with keyboard.
(In reply to comment #84)
> (or being able to focus the search field using
> keyboard after typing Ctrl+Tab).

Ctrl+F currently does that.

> ok, but then it is not possible to select one of the thumbnail using keyboard
> once the search textbox is focused. Or am I missing something? Maybe the TAB
> key should still let the focus get out of the search textbox in order to select
> one of the thumbnail with keyboard.

Yes, the TAB key should definitely do that (but doesn't currently).
(In reply to comment #85)
> Ctrl+F currently does that.

Great, exactly what I need :-)

> Yes, the TAB key should definitely do that (but doesn't currently).

Do you want me to file this?
There's no bug on that yet, so feel free to file.
Depends on: 463220
No longer depends on: 463220
Depends on: 463227
I'm not going to block a FIXED bug, but a menuitem is needed for this capability. See why under bug 463329.
Depends on: 463363
Flags: in-litmus?
Several new test cases added to Litmus - see https://wiki.mozilla.org/QA/Firefox3.1/Ctrl-Tab_Test_Plan#Testcases_added_to_Litmus.
Flags: in-litmus? → in-litmus+
Depends on: 463635
No longer depends on: 463693
No longer depends on: 461519
Depends on: 463574
No longer depends on: 463569
Depends on: 463969
Flags: blocking-firefox3.1? → blocking-firefox3.1-
No longer blocks: 463201
Depends on: 463201
Depends on: 464045
Depends on: 464205
Depends on: 463822
Depends on: 464437
Depends on: 464439
Depends on: 464721
No longer depends on: 463574
Depends on: 463574
No longer depends on: 445765
No longer depends on: 463318
Depends on: 463318
You need to log in before you can comment on or make changes to this bug.