Closed Bug 465076 Opened 16 years ago Closed 15 years ago

Yet another Ctrl+Tab / All Tabs design revision (pref'd off)

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: dao, Assigned: dao)

References

()

Details

(Keywords: access, ue)

Attachments

(1 file, 37 obsolete files)

92.69 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch patch v1 (obsolete) — Splinter Review
Keywords: late-l10n
Copying over beltzner's original pastebin mockup as that won't last forever.

quickswitch UI (invoked by ctrl-tab)

 .--------------------------------------------.
 |  ####  ####  ####  ####  ####  ####  ####  |
 |  ####  ####  ####  ####  ####  ####  ####  |  <-- 7 MRU tabs
 |  bar   foo   baz   baf   bop   bim   pow   |  <-- long, narrow strip
 |                                            |
 |             ( Show all N tabs )            |  <-- button is in tab order
 '--------------------------------------------'

 while holding ctrl,
    tab     key moves through previews
    <- / -> keys move through previews
    w       key closes highlighted tab
    a, f    key transforms UI to tabfind
    
 mouse hover highlights target
 selecting a tab preview switches tabs
 selecting the "Show All Tabs" button transforms UI to tabfind

tabfind UI (invoked by ctrl-tab,a / ctrl-tab,f / ctrl-tab,show all tabs / all-tabs button / view -> all tabs) 

 .--------------------------------------------.
 |    Filter [                    ]   (X)     |
 |                                            |
 |  /---  ####  ####  ####  ####  ####  ####  |
 |  \---  ####  ####  ####  ####  ####  ####  |  <-- all tabs in tabstrip order
 |  prev   bar   baz   bar   bop   bim   pow  |  <-- page entries if needed
 |                                            |  <-- overlays browser window
 |  ####  ####  ####  ####  ####  ####  ####  |
 |  ####  ####  ####  ####  ####  ####  ####  |
 |  foo   bar   baz   bar   bop   bim   pow   |
 |                                            |
 |  ####  ####  ####  ####  ####  ####  ___\  |
 |  ####  ####  ####  ####  ####  ####  ---/  |
 |  foo   bar   baz   bar   bop   bim   next  |
 |                                            |
 '--------------------------------------------'

 don't need to hold down control
 focus starts in the "Filter" field
 down key moves into the preview field, at which point arrow keys move around
 mouse hover highlights preview
 when hovering over a preview, we should overlay a (x) closebox
Attached image screenshot of v1 all tabs (obsolete) —
Boriss: Is the tab title text supposed to only show up on hover? Where should the close button be placed?
Hold on... I'm still working on the styling.
Blocks: 447580
Attached patch experimental patch v2 (obsolete) — Splinter Review
Ed, can you please give this a try? Could be that it totally breaks Ctrl+Tab on OS X.
Attachment #348340 - Attachment is obsolete: true
Attachment #348359 - Attachment is obsolete: true
Blocks: 463799
Attached patch patch v2 (obsolete) — Splinter Review
still need to figure out if the CTRLTAB_BROKEN_FOCUS stuff is needed
Attachment #348370 - Attachment is obsolete: true
Keywords: access, ue
Attached patch patch v2.1 (obsolete) — Splinter Review
more cleanup / polish

tryserver builds:

https://build.mozilla.org/tryserver-builds/2008-11-16_02:24-dgottwald@mozilla.com-try-85be9f8c30c/
Attachment #348390 - Attachment is obsolete: true
Ok, I gave the latest tryserver build a shot. Here my comments for the OS X version:

* The preview titles are presented differently for ctrl-tab and all-tabs. For the latter one there is an additional line between the preview and the description. Personally I like the all-tabs version which doesn't glue the text at the bottom of the preview.

* IMO it would look better if the selection focus ring wouldn't include the preview description. Instead only the preview should be surrounded which will make the text better readable. The applies more for all-tabs because the current version doesn't have a good contrast difference between the text color and the background color. Probably you forgot to set a black text color like for ctrl-tab.

* The close button for previews only appear for selections you make with the mouse. There is no visual clue that tabs can be closed when using the keyboard to navigate through the list of previews.

* The close buttons are misplaced in the all-tabs panel when the previews are scaled down so all will fit on one page. 

* Having two tabs open and opening all-tabs lets you switch between the tabs in the background by hitting the ctrl+tab keyboard shortcut. I don't think that we want to have this behavior as other existing bugs already report.

* Having the Ctrl-Tab and All-Tabs panel open you can open the find bar on OS X by hitting Cmd+F which switches the focus back to the browser window.

* Having the All-Tabs panel open you cannot use Ctrl+F to focus the search field. The shortcut doesn't seems to be assigned anymore.

* When comparing the sizes of the all-tabs elements the close button in the top right corner is too much in my opinion. There should be a smaller one. Similar to the close button for the tab previews.

* I'm still able to focus/select more then one tab when using the keyboard and mouse together. IMO this behavior is still irritating and is better solved for the native application switcher where only one application is focused/selected. IMO we should follow this way. This is covered by bug 463318.

* Having around 50 tabs open will still give you a noticeable lag (~0.75s) when navigating through the list. This is really annoying and makes it impossible to navigate through the list. 

* The initial painting shows a white background. This is still covered by bug 463303 

* Only showing at maximum 7 tabs for ctrl-tab makes it hard to get to a special tab. You have to open the all-tabs panel to select it. But while having your fingers still on ctrl+tab let you immediately return to the ctrl-tab panel. I believe that will frustrate users.

* Page Up/Down and Home/End would be some nice shortcuts to jump directly to the other side of the row/column.

* If you have open a huge amount of tabs (e.g. 75) and entering a search term which lets the panel recalculate the preview sizes the search bar moves

* Pressing Ctrl+Tab while the All-Tabs panel is open for a huge amount of tabs the Ctrl-Tab panel isn't shown. Instead we can see the repainting bug again. The whole underlying UI isn't refreshed. This only happens when you click onto the empty background before hitting Ctrl+Tab.

* Having around 75 tabs open and selecting one of the previews in the last column lets the panel resize while hovering the preview which also let the search textfield move to left/right.

* Entering some letters inside the All-Tabs panel should automatically put this letter into the search textbox. It will enhance the usability a lot IMO.

* Going out of the search textbox moving around with the arrow keys and pressing some letters let the panel not react on ESC. You have to close it with the mouse or go back to the search textbox to enable it again.
Blocks: 463822
Attached patch patch v3 (obsolete) — Splinter Review
- View > Show All Tabs menu item added
- not using box-shadow in the All Tabs panel anymore (perf)
- fixed the close button position on Mac
- closing the All Tabs panel when there's a browser command executed (such as Accel+L)
Attachment #348466 - Attachment is obsolete: true
Here's a tryserver build with the CTRLTAB_BROKEN_FOCUS stuff (see patch) removed:

https://build.mozilla.org/tryserver-builds/2008-11-17_10:55-dgottwald@mozilla.com-try-1f0036d0003/

Would be nice if somebody could tell me if this breaks Ctrl+Tab on OS X.
Attached image screenshot of 1f0036d0003 (obsolete) —
Here's ctrl-tab (mouse over 4th tab) and all-tab views.

Ctrl-tab doesn't always work. Sometimes it doesn't select the tab until I press and release ctrl a second time.
(In reply to comment #13)
> Created an attachment (id=348616) [details]
> screenshot of 1f0036d0003
> 
> Here's ctrl-tab (mouse over 4th tab) and all-tab views.
> 
> Ctrl-tab doesn't always work. Sometimes it doesn't select the tab until I press
> and release ctrl a second time.

Ok, Thanks. So that's still bug 463802, and the CTRLTAB_BROKEN_FOCUS stuff needs to stay.
(In reply to comment #13)
> Created an attachment (id=348616) [details]
> screenshot of 1f0036d0003

Is it only me or does font-weight:bold make the Ctrl+Tab labels too heavy?
(In reply to comment #12)
> https://build.mozilla.org/tryserver-builds/2008-11-17_10:55-dgottwald@mozilla.com-try-1f0036d0003/
> 
> Would be nice if somebody could tell me if this breaks Ctrl+Tab on OS X.

That breaks Ctrl-Tab on Vista for me. Most of the time the ctrl-tab panel closes immediately after it was opened. I have to focus a textbox so it stays open. 

(In reply to comment #15)
> Is it only me or does font-weight:bold make the Ctrl+Tab labels too heavy?

As what I said on bug 458866, fonts are looking bad for me at all on Vista.

(In reply to comment #10)
> - View > Show All Tabs menu item added

Bah. Please don't use Shift-Ctrl-Tab as shortcut. Users wont be able to navigate backwards trough the list of tabs as initial action anymore. But beside, will we also have a shortcut on OS X?

> - not using box-shadow in the All Tabs panel anymore (perf)

That's much better now. There is no lag noticeable anymore. Only the white background during initialization is visible. Otherwise it looks good. 

> - fixed the close button position on Mac

Not really. Now it overlays the top right corner of the tabs preview which makes it some kind cut-off at the half of the x.

> - closing the All Tabs panel when there's a browser command executed (such as
> Accel+L)

Ctrl+Tab still switches between two opened tabs in the background.

More comments soonish.
(In reply to comment #16)
> That breaks Ctrl-Tab on Vista for me. Most of the time the ctrl-tab panel
> closes immediately after it was opened. I have to focus a textbox so it stays
> open.

This sounds like bug 457997 comment 0. If you can reproduce this with the build in comment 11, that bug should probably be reopened.

> Bah. Please don't use Shift-Ctrl-Tab as shortcut. Users wont be able to
> navigate backwards trough the list of tabs as initial action anymore. But
> beside, will we also have a shortcut on OS X?

Control+Shift+Tab as well?

> That's much better now. There is no lag noticeable anymore. Only the white
> background during initialization is visible.

I don't see that white background.

> > - fixed the close button position on Mac
> 
> Not really. Now it overlays the top right corner of the tabs preview which
> makes it some kind cut-off at the half of the x.

It's aligned with the clickable area that's also highlighted.
Further tries on OS X and Vista follows now:

OS X
====
* If you remove an external monitor to only use the internal display the previews are clued together on my OS X box. There is no margin in-between them. It looks like a big white stripe for me for only blank tabs.

* When will you style up the Ctrl-Tab panel so it has the same design as All-Tabs?

* There is no padding anymore between the panels border and the selection box.

* The close button is only positioned correctly for non-resized previews. As soon as they have to be shrinked the close button is misplaced.

Vista
=====

* The selection box is nearly unvisible. Looks like you are using too much opacity.

* It's hard to focus the close button for each tab preview. Most of the times I'm hovering over them they are hidden and I'm unable to close the tab. Instead the All-Tabs panel gets closed.

* The right side of the All-Tabs panel seems somehow cut-off. Means the big X is half-visible and all previews on that side have a smaller selection box on the right side.
(In reply to comment #17)
> > That breaks Ctrl-Tab on Vista for me. Most of the time the ctrl-tab panel
> > closes immediately after it was opened. I have to focus a textbox so it stays
> > open.
> 
> This sounds like bug 457997 comment 0. If you can reproduce this with the build
> in comment 11, that bug should probably be reopened.

Probably. I'm not sure if it's the identical problem. Now its even not possible over textboxes. Please decide yourself.

> > Bah. Please don't use Shift-Ctrl-Tab as shortcut. Users wont be able to
> > navigate backwards trough the list of tabs as initial action anymore. But
> > beside, will we also have a shortcut on OS X?
> 
> Control+Shift+Tab as well?

As I said, this shortcut goes against an already known shortcut to backward navigate through the tab list. It shouldn't be used at all IMO.

> > That's much better now. There is no lag noticeable anymore. Only the white
> > background during initialization is visible.
> 
> I don't see that white background.

Please open about 100 tabs and click on the All-Tabs button. But anyway we should move this out to the other bug I've filed.

> > Not really. Now it overlays the top right corner of the tabs preview which
> > makes it some kind cut-off at the half of the x.
> 
> It's aligned with the clickable area that's also highlighted.

Please open enough tabs so the previews gets shrunken. You will notice that the close button moves over the tabs preview.
Using the build in Comment 11 on OSX, I noticed that Ctrl-Tab preview extends slightly outside the border of the browser windows when the window is normal size, but if you resize the window things can look a little funky. Not saying we have to do anything about it, but just pointing it out.
(In reply to comment #18)
> * If you remove an external monitor to only use the internal display the
> previews are clued together on my OS X box. There is no margin in-between them.
> It looks like a big white stripe for me for only blank tabs.

Screen resolution changes aren't tracked, regardless of the given patch.

> * When will you style up the Ctrl-Tab panel so it has the same design as
> All-Tabs?

I don't intend to use exactly the same styling. The interaction is different, there's no tab close button in the Ctrl+Tab previews and the panel is less vulnerable to performance issues (i.e. box-shadow can still be used) due to showing less previews.

> * The selection box is nearly unvisible. Looks like you are using too much
> opacity.

Could you please attach a screenshot?

> * It's hard to focus the close button for each tab preview. Most of the times
> I'm hovering over them they are hidden and I'm unable to close the tab. Instead
> the All-Tabs panel gets closed.

Strange, I don't see that on XP.

> * The right side of the All-Tabs panel seems somehow cut-off. Means the big X
> is half-visible and all previews on that side have a smaller selection box on
> the right side.

Sounds like another aero glass bug.
(In reply to comment #19)
> Probably. I'm not sure if it's the identical problem. Now its even not possible
> over textboxes. Please decide yourself.

Please comment in the bug. I don't have Vista.

> As I said, this shortcut goes against an already known shortcut to backward
> navigate through the tab list. It shouldn't be used at all IMO.

It's a useless shortcut for an MRU ordered list.

> > It's aligned with the clickable area that's also highlighted.
> 
> Please open enough tabs so the previews gets shrunken. You will notice that the
> close button moves over the tabs preview.

Right. The clickable area gets smaller, but the close button stays the same size. I don't see a problem with that.
Depends on: 457997
Depends on: 463802
No longer depends on: 463802
Attached patch patch v3.1 (obsolete) — Splinter Review
this disables aero glass because
Attachment #348548 - Attachment is obsolete: true
(In reply to comment #23)
> Created an attachment (id=348725) [details]
> patch v3.1
> 
> this disables aero glass because

because of bug 457997
(In reply to comment #23)
> Created an attachment (id=348725) [details]
> patch v3.1

https://build.mozilla.org/tryserver-builds/2008-11-18_00:39-dgottwald@mozilla.com-try-1d69199c3af/
Blocks: 463329
Attached patch patch v3.2 (obsolete) — Splinter Review
changed the previews' hover feedback to stay when moving the mouse over the close button

https://build.mozilla.org/tryserver-builds/2008-11-18_05:29-dgottwald@mozilla.com-try-e817685c67f/
Attachment #348725 - Attachment is obsolete: true
Blocks: 463318
(In reply to comment #26)
> Created an attachment (id=348891) [details]
> patch v3.2
> 
> changed the previews' hover feedback to stay when moving the mouse over the
> close button
> 
> https://build.mozilla.org/tryserver-builds/2008-11-18_05:29-dgottwald@mozilla.com-try-e817685c67f/
Hmm.. On Vista the all-tabs button on the tab-strip isn't working, In the error console I see:
Error: this._getBox(this.tabCloseButton._tab) is null
Source File: chrome://browser/content/browser.js
Line: 7745

STR:
1) Open a few tabs.
2) Ctrl-Tab through them.
3) Hit "Show All Tabs" button on the bottom
4) Close a background tab, then close the all-tabs with the X
5) Hit the all-tabs button on the tab-strip. Walla
Attached patch patch v3.3 (obsolete) — Splinter Review
mouseover and focus highlight unified in the ctrl+tab panel
Attachment #348891 - Attachment is obsolete: true
(In reply to comment #28)
> Created an attachment (id=348996) [details]
> patch v3.3
> 
> mouseover and focus highlight unified in the ctrl+tab panel

https://build.mozilla.org/tryserver-builds/2008-11-19_07:32-dgottwald@mozilla.com-try-18b3115cec8/
(In reply to comment #27)
> Hmm.. On Vista the all-tabs button on the tab-strip isn't working, In the error
> console I see:
> Error: this._getBox(this.tabCloseButton._tab) is null
> Source File: chrome://browser/content/browser.js
> Line: 7745
> 
> STR:
> 1) Open a few tabs.
> 2) Ctrl-Tab through them.
> 3) Hit "Show All Tabs" button on the bottom
> 4) Close a background tab, then close the all-tabs with the X
> 5) Hit the all-tabs button on the tab-strip. Walla

Thanks for reporting. Can you try this again with the latest tryserver build?
(In reply to comment #30)
No, it doesn't happen anymore. However, the Ctrl-tab background is very light and easy to miss (I couldn't find it at first). Also the "Show All Tabs" button is part of the tab-order, i.e. tabbing focuses the button as well, is this intentional? if so maybe you should rethink that as it's not part of regular Ctrl-tabbing under any environment (ie windows Alt-tab). I would expect Ctrl-tab to just loop through the tabs themselves.
On a side note... I really hope this feature sticks in 3.1, it's really awesome, thanks!
(In reply to comment #31)
> Also the "Show All Tabs" button
> is part of the tab-order, i.e. tabbing focuses the button as well, is this
> intentional?

Yes, that's by design -- see comment 2.
at least with windows XP (classic) there is a misalignment with an uneven amount of tabs in the all-tabs panel. intended?
(In reply to comment #33)
> Created an attachment (id=349033) [details]
> misalignment of tab preview (all-tabs panel)
> 
> at least with windows XP (classic) there is a misalignment with an uneven
> amount of tabs in the all-tabs panel. intended?

Not directly intended -- it's the close button on the right that pushes the search field to the left.
Attached patch patch 3.4 (obsolete) — Splinter Review
comment 33 addressed
Attachment #348996 - Attachment is obsolete: true
Attachment #349033 - Attachment is obsolete: true
Attached image Selection box for all-tabs on Vista (obsolete) —
The selection box is nearly unvisible on Vista.
Comment on attachment 349166 [details]
Selection box for all-tabs on Vista

Sorry, that wasn't with the latest tryserver build. Now with the darker background there is a better distinction.
Attachment #349166 - Attachment is obsolete: true
(In reply to comment #21)
> > * When will you style up the Ctrl-Tab panel so it has the same design as
> > All-Tabs?
> 
> I don't intend to use exactly the same styling. The interaction is different,
> there's no tab close button in the Ctrl+Tab previews and the panel is less
> vulnerable to performance issues (i.e. box-shadow can still be used) due to
> showing less previews.

I only meant the selection box effect. It looks much better for the all-tabs panel.

> > * It's hard to focus the close button for each tab preview. Most of the times
> > I'm hovering over them they are hidden and I'm unable to close the tab. Instead
> > the All-Tabs panel gets closed.
> 
> Strange, I don't see that on XP.

Ok, that has been solved. But if you click to fast on these close buttons the all-tabs panel is closed and leaves ugly artifacts behind. Open around 70 tabs and position the mouse on top of a close button. Now just click with a fast rate. I'll attach a screenshot.

> > * The right side of the All-Tabs panel seems somehow cut-off. Means the big X
> > is half-visible and all previews on that side have a smaller selection box on
> > the right side.
> 
> Sounds like another aero glass bug.

Has been solved.
Attached image Artifacts on Vista (obsolete) —
Closing tabs too fast by clicking the small close icon closes the all-panels tab and leaves behind ugly artifacts.
(In reply to comment #22)
> > As I said, this shortcut goes against an already known shortcut to backward
> > navigate through the tab list. It shouldn't be used at all IMO.
> 
> It's a useless shortcut for an MRU ordered list.

Not everyone will have a MRU list. If you turn it off users don't expect to see the all-tabs panel but switching backwards through the list. These shortcut has been in use forever. So please don't change it now. I believe a lot of ppl will agree.

> > > It's aligned with the clickable area that's also highlighted.
> > 
> > Please open enough tabs so the previews gets shrunken. You will notice that the
> > close button moves over the tabs preview.
> 
> Right. The clickable area gets smaller, but the close button stays the same
> size. I don't see a problem with that.

Yeah, it stays at the same size but it moves over the tabs preview. Shouldn't it always be displayed on the right side of the preview without this overlap?
Some version I tried, the all-tabs view only showed the tab's title on hover. Is that intentional/required?

Also, couldn't the close tab button be placed in the top left corner like the mockup:

https://wiki.mozilla.org/images/9/97/Alltabsview1.png
Dao, did you mean to also make all-tabs handle keyboard/mouse focus on hover?

Is all-tabs supposed to move the selection when you press ctrl-(shift)-tab? (Right now, pressing tab/shift tab will move focus)

For ctrl-shift-tab by itself without anything open, we could open up the all-tabs view with the current tab selected.
(In reply to comment #42)
> Dao, did you mean to also make all-tabs handle keyboard/mouse focus on hover?
> 
> Is all-tabs supposed to move the selection when you press ctrl-(shift)-tab?
> (Right now, pressing tab/shift tab will move focus)
> 
> For ctrl-shift-tab by itself without anything open, we could open up the
> all-tabs view with the current tab selected.

All-tabs is persistant, meaning it isn't dismissed by releasing keys.  So, pressing tab or the arrow keys or mousing over should shift the focus.
Well, on os x, hovering an item will give it a hover effect and the close button, but the "really focused" tab still has its focused styling.

Additionally, pressing ctrl-tab inside all-tab causes ctrl-tab to show up and close all-tab. Pressing ctrl-shift-tab would probably just reopen the all-tabs view instead of shifting focus to the previous tab preview.

And "accel" on os x is cmd and not control.
(In reply to comment #39)
> Created an attachment (id=349168) [details]
> Artifacts on Vista
> 
> Closing tabs too fast by clicking the small close icon closes the all-panels
> tab and leaves behind ugly artifacts.

See bug 463574.

> Not everyone will have a MRU list. If you turn it off users don't expect to see
> the all-tabs panel but switching backwards through the list.

The right way to disable this is browser.ctrlTab.previews

(In reply to comment #41)
> Some version I tried, the all-tabs view only showed the tab's title on hover.
> Is that intentional/required?

That was the very first patch in this bug, right? That was a theming issue on Mac and unintentional.

> Also, couldn't the close tab button be placed in the top left corner like the
> mockup:
> 
> https://wiki.mozilla.org/images/9/97/Alltabsview1.png

Sure it could. On Windows and Linux though, the correct side for the close button is the logical end.
Attachment #349148 - Attachment description: patch → patch 3.4
Here's a quick patch against v3.4 to make ctrl-shift-tab actually open all-tabs on OS X as well as allow the user to keep holding ctrl while pressing tab/shift-tab to move the selection in the all-tabs view. Right now pressing ctrl-shift-tab will focus the current tab and pressing tab will move it to the very next (in tab order) or shift-tab to previous (in tab order) unless it runs off the list and goes to the search box.
Attachment #349291 - Attachment is patch: true
Attachment #349291 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #46)
> allow the user to keep holding ctrl while pressing
> tab/shift-tab to move the selection in the all-tabs view.

According to beltzner, ctrl+shift+tab should focus the search field just like pressing the button or using the menu item.
(In reply to comment #44)
> And "accel" on os x is cmd and not control.

Oops.
Attached patch patch v3.5 (obsolete) — Splinter Review
s/accel/control/

close button position inverted for Mac and/or rtl
Attachment #348616 - Attachment is obsolete: true
Attachment #349148 - Attachment is obsolete: true
Attachment #349168 - Attachment is obsolete: true
Attached patch patch v3.5 (obsolete) — Splinter Review
Attachment #349326 - Attachment is obsolete: true
(In reply to comment #42)
> Dao, did you mean to also make all-tabs handle keyboard/mouse focus on hover?

No... I'm pretty sure we don't want to steal the the search field's focus when moving the mouse.
(In reply to comment #49)
> Created an attachment (id=349326) [details]
> patch v3.5
> 
> s/accel/control/
> 
> close button position inverted for Mac and/or rtl

https://build.mozilla.org/tryserver-builds/2008-11-20_17:46-dgottwald@mozilla.com-try-d2241a45492/
(In reply to comment #45)
> > Also, couldn't the close tab button be placed in the top left corner like the
> > mockup:
> > 
> > https://wiki.mozilla.org/images/9/97/Alltabsview1.png
> 
> Sure it could. On Windows and Linux though, the correct side for the close
> button is the logical end.

Just to be curious, but do we have mockups also available for other OS as OS X? Will they all look the same way? Lately there were a lot of changes to colors, backgrounds and so on. Wouldn't it make more sense to have requirements set before doing all the design revision work? It's even hard to track things like the "selection box is nearly invisible" when the design changes even between the tryserver builds which were given on this bug.

Alex and Jennifer, do I miss the link or is there even nothing right now?
(In reply to comment #53)
> Wouldn't it make more sense to have requirements set
> before doing all the design revision work?

This bug is first and foremost about the interaction, not the styling.
Mmh, so what shall the summary tell me in detail? Looks like a bad wording.
No longer depends on: 463574
>Just to be curious, but do we have mockups also available for other OS as OS X?
>Will they all look the same way?

>Alex and Jennifer, do I miss the link or is there even nothing right now?

The design should be platform specific, Boriss is handling the visual design on this one so she can link to where those mockups or tracking bugs are (I really haven't been following any of this too closely).
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre
Mac OS X 10.4.11
clean profile

show all tabs has stopped working in the 11/20 nightly. Was working int the 11/19 nightly
(In reply to comment #57)
> show all tabs has stopped working in the 11/20 nightly. Was working int the
> 11/19 nightly

Bug 465843, backed out for beta release.
Attached patch patch v3.6 (obsolete) — Splinter Review
some minor visual adjustments

https://build.mozilla.org/tryserver-builds/2008-11-24_05:41-dgottwald@mozilla.com-try-37951882e7b/
Attachment #349327 - Attachment is obsolete: true
The [x] in the All-Tab panel with only one tab closes the whole window (and possibly Firefox, if that's the only one opened), is this intentional? IMHO, if the [x] on the tab-strip itself is removed in a one tab situation, the same should apply to All-Tabs. Also, but this is more a UI decision, what does ctrl-shift-tab have to do with All-Tabs, everywhere else shift is used to reverse the effect of the command (even now ctrl-shift-tab just reverses the order of tab switching), at least on Windows. Sounds a little unintuitive, why can't another shortcut be used?
Attached patch patch v3.7 (obsolete) — Splinter Review
(In reply to comment #60)
> IMHO, if
> the [x] on the tab-strip itself is removed in a one tab situation, the same
> should apply to All-Tabs.

Thanks, fixed.
Attachment #349799 - Attachment is obsolete: true
Attached patch patch v3.8 (obsolete) — Splinter Review
https://build.mozilla.org/tryserver-builds/2008-12-02_01:31-dgottwald@mozilla.com-try-bc44bda9428/
Attachment #349812 - Attachment is obsolete: true
Attachment #350952 - Flags: ui-review?(beltzner)
Attachment #350952 - Flags: review?(mconnor)
I don't think an OS skinned command button on a translucent tab strip mixes well
Dao, it would be great if you could always give a comment about things which have changed in the new version. That saves us from download and test if only minor issues were fixed.
(In reply to comment #64)
> That saves us from download and test if only minor issues were fixed.

You can assume that it's a minor change if the version bump is minor and no comment was made.

Those who are curious should try bugzilla's interdiff, although it doesn't always work and distinguishing actual changes from interdiff noise can be tricky.
Attached patch patch v3.9 (obsolete) — Splinter Review
updated for bug 457997 and bug 450800

https://build.mozilla.org/tryserver-builds/2008-12-06_04:31-dgottwald@mozilla.com-try-d2343a15dc3/
Attachment #350952 - Attachment is obsolete: true
Attachment #351705 - Flags: ui-review?(beltzner)
Attachment #351705 - Flags: review?(mconnor)
Attachment #350952 - Flags: ui-review?(beltzner)
Attachment #350952 - Flags: review?(mconnor)
Blocks: 468600
Here's a tryserver build with the patch in bug 463802:

https://build.mozilla.org/tryserver-builds/2008-12-12_01:34-dgottwald@mozilla.com-try-b3126c3c775/dgottwald@mozilla.com-try-b3126c3c775-firefox-try-mac.dmg

Can somebody please test the Ctrl+Tab shortcut with this on OS X?
(Note that the build contains the fix for bug 430332, which caused crashes and got backed out since then, but for a brief test run this should be ok.)
Ctrl-Tab seems to be working when switching forwards/backwards on OS X. I'm assuming you took out the BROKENFOCUS stuff?

A separate issue is when you go into all tabs mode, how are you supposed to select the tab? Pressing enter doesn't work, but space seems to work.

But some reason it was natural for me to press ctrl to try selecting the tab. Probably because I let go of ctrl to switch into all-tabs mode, so pressing+letting go of ctrl a second time should have made it select the currently focused tab.

Additionally, when I was looking at the bug for showing the target of a submit button, I noticed the old tab listing would show the url of the tab with setOverLink:
http://hg.mozilla.org/mozilla-central/rev/c8f2a2be4d61#l3.245
(In reply to comment #69)
> Ctrl-Tab seems to be working when switching forwards/backwards on OS X. I'm
> assuming you took out the BROKENFOCUS stuff?

exactly

> A separate issue is when you go into all tabs mode, how are you supposed to
> select the tab? Pressing enter doesn't work, but space seems to work.

Enter works just like Space for me on Windows (and Linux, pretty sure). I thought button.xml#button-base handles this, but I'm not sure right now...

> Additionally, when I was looking at the bug for showing the target of a submit
> button, I noticed the old tab listing would show the url of the tab with
> setOverLink:
> http://hg.mozilla.org/mozilla-central/rev/c8f2a2be4d61#l3.245

An early version (of this UI, actually) did that in bug 436304, but the panel can overlap the statusbar and it was more noisy than useful anyway.
Blocks: 469768
Attached patch patch v3.10 (obsolete) — Splinter Review
workaround for bug 463802 removed
Attachment #351705 - Attachment is obsolete: true
Attachment #353415 - Flags: ui-review?(beltzner)
Attachment #353415 - Flags: review?(mconnor)
Attachment #351705 - Flags: ui-review?(beltzner)
Attachment #351705 - Flags: review?(mconnor)
Blocks: 470970
I was pointed here from a bug that I filed:
Bug 470970 -  Control-Tab and Control-Shift-Tab don't cycle through the Tab History List
https://bugzilla.mozilla.org/show_bug.cgi?id=470970

As a long-term user that has grown up with (in a computing sense) with Control-Tab and Control-Shift-Tab in an MDI cycling through the document history list, aka MRU list or sometimes Z-order, I'm really shuddering at what I'm reading here.  Please see my bug for a description of how Control-Tab and Control-Shift-Tab are *supposed* to work, at least in all Windows environments since 3.1.  This behaviour is already broken in FF, but instead of just fixing it, you seem hell bent on changing it permanently to something different so that it can never be fixed.

Control-Tab, along with its window counterpart, Alt-Tab, are the most useful shortcuts in a Windows environment.  If you must do this:
1)	Fix Control-Tab and Control-Shift-Tab to behave as they *should*;
2)	Use a shortcut other than Control-Tab or Control-Shift-Tab for this;
3)	Do NOT include it in the standard build, make it a downloadable add-on for those that want it;
Depends on: 473152
Attached patch updated to trunk (obsolete) — Splinter Review
https://build.mozilla.org/tryserver-builds/2009-01-18_03:51-dgottwald@mozilla.com-try-a5e62063f5b/
Attachment #353415 - Attachment is obsolete: true
Attachment #357545 - Flags: ui-review?(beltzner)
Attachment #357545 - Flags: review?(mconnor)
Attachment #353415 - Flags: ui-review?(beltzner)
Attachment #353415 - Flags: review?(mconnor)
Keywords: late-l10n
More on control-tab (not sure if I'm reading comment 73 a a duplicate of this comment or not).  Control tab used to cycle through tabs starting at the *current* tab.  This makes sense, because you generally want to move from where you currently are to something nearby.  But the new behavior is to always start at the beginning.  This requires:

1) Search through the thumbnails for the page you are looking for - and possibly the one where you already are (if  you know what it looks like)
2) Move through the entire list - note that the leftmost tab is most likely the oldest, since tabs open from left to right.
3) Get to the tab you want.

So if you were on tab 8 and middle clicked on two links to open two new tabs, control tab now takes 9 and 10 keystrokes respectively to reach your new tabs, whereas it used to take 1 or 2.  This is extremely inefficient.

Worse, since only 7 tabs are displayed, you may have no idea what you are cycling TO.
Have you considered including the favicon (perhaps as an overlay on the scaled page image) in the tab preview? For me the preview image is pretty much useless: 90% of my tabs are black-on-white and not differentiable.

Also, the close-tab button makes me nervous. I can't just slew my mouse across and point at the thing I want. I have to be really careful not to click the close-tab button by mistake.
The tab panel is a jarring overlaying of the UI. The MRU order is unintuitive because it conflicts with the visible tab bar order and is an abstract and ever changing mental model.

I would suggest (plead!) that CTRL+Tab either stay as it was (not have any overlay) or that the overlay be in harmony with the tab bar (see below).

  ______  ________  ______  ______  ______
 /  T1  \// *T2* \\/  T3  \/  T4  \/  T5  \  <-- tabs
|       |         |       |       |        |
|       |         |       |       |        | <-- overlays
|       |         |       |       |        |
+-------+---------+-------+-------+--------+

- The active tab (T2) should be highlighted.
- The selected and/or hovered tab should show a (different!) highlight effect.

Yes, the preview panels would get pretty small when there are many tabs (>5). A solution could be that the selected tab is made larger and the neighboring tabs dynamically taper off in size. We should never show less than 5 tabs (that's only 2 in the direction of travel) (better would be 7 tabs). There should always be some "connecting lines" between the preview panels and their tabs.

Drop MRU (or give it a different key-combo than CTRL+Tab). It cannot be implemented without sacrificing the much more important UI metaphors.
(In reply to comment #77)
> Have you considered including the favicon (perhaps as an overlay on the scaled
> page image) in the tab preview? For me the preview image is pretty much
> useless: 90% of my tabs are black-on-white and not differentiable.

Here's a tryserver build with favicons in the all-tabs panel:
https://build.mozilla.org/tryserver-builds/2009-01-28_13:44-dgottwald@mozilla.com-try-e42d5c6e1b3/

My impression is that they clutter up the panel and make it harder to focus on the snapshots.
I'm quite liking the favicons, I'm finding I can either concentrate on them or the thumbnail preview. Though they're not implemented for ctrl+tab, just tab. Anyway, it's certainly feels a lot better than the old overflow menu which felt very limited (couldn't even middle click to close tabs on it). 

Is it intentional that when you ctrl+tab and mouse over "show all tabs" and let go of ctrl it opens all tabs? It seems odd but also reasonable at the same time.
Yeah, the favicons really help me, too. They're pretty subtle right now... I wonder whether making them bigger would help at all or just be distracting.
Added the favicons to ctrl+tab, made them bigger and slightly transparent:

https://build.mozilla.org/tryserver-builds/2009-01-29_12:21-dgottwald@mozilla.com-try-08d6fd6258e/
4 tweaked versions of the favicon

1) white background with 1px outline rgba(0,0,0,.5)
2) white background + black .1
3) white background + no outline
4) no background + no outline
Attached patch patch (with favicons) (obsolete) — Splinter Review
Attachment #359670 - Flags: ui-review?(beltzner)
Attached patch patch v4 (with favicons) (obsolete) — Splinter Review
Added a white background (thanks Ed) and a 2px padding to the favicons.
Attachment #357545 - Attachment is obsolete: true
Attachment #359675 - Attachment is obsolete: true
Attachment #359754 - Flags: ui-review?(beltzner)
Attachment #359754 - Flags: review?(mconnor)
Attachment #357545 - Flags: ui-review?(beltzner)
Attachment #357545 - Flags: review?(mconnor)
Attachment #359670 - Flags: ui-review?(beltzner)
(In reply to comment #85)
> Created an attachment (id=359754) [details]
> patch v4 (with favicons)
> 
> Added a white background (thanks Ed) and a 2px padding to the favicons.

https://build.mozilla.org/tryserver-builds/2009-01-30_05:25-dgottwald@mozilla.com-try-6d8860a0716/
i was wondering if there is a way to give the thumbnails the same centering as the web page. this is mostly relevant for wide screens.
for example in the this screen grab:
http://i41.tinypic.com/wat7w6.jpg
you can see that the left and right thumbnails are not centered correctly, so that a lot of the thumbnail's area is wasted.
Dao, do you maintain a public repo I would like to pull and integrate these patches in my build.
(In reply to comment #80)
> Is it intentional that when you ctrl+tab and mouse over "show all tabs" and let
> go of ctrl it opens all tabs? It seems odd but also reasonable at the same
> time.

Yes, that's intentional. Just like tabbing, the mouse changes focus within the available elements, and the "show all tabs" button is one of them.

(In reply to comment #87)
> i was wondering if there is a way to give the thumbnails the same centering as
> the web page. this is mostly relevant for wide screens.

The problem is that centering the snippet isn't right for left-aligned pages, and capturing the whole page means less detail. I tried making the viewport smaller using fullZoom, but that's rather expensive and messes up the scroll position.

(In reply to comment #88)
> Dao, do you maintain a public repo I would like to pull and integrate these
> patches in my build.

There's no public repository currently. I figured it wasn't worth the overhead since I work on the code alone and make the latest patch available continuously.
(In reply to comment #89)
> (In reply to comment #87)
> > i was wondering if there is a way to give the thumbnails the same centering as
> > the web page. this is mostly relevant for wide screens.
> 
> The problem is that centering the snippet isn't right for left-aligned pages,
> and capturing the whole page means less detail. I tried making the viewport
> smaller using fullZoom, but that's rather expensive and messes up the scroll
> position.
> 
can't you check the alignment of the page and let the snippet have the same alignment?
No, there are too many ways to align a page layout. An interesting idea would be to do some smart analysis of the snaptshot's image data and throw away the side with the least diversity -- might work but would likely be slow. At that point we could just use fullZoom.
Attached patch patch v4.1 (obsolete) — Splinter Review
updated to latest trunk
Attachment #359754 - Attachment is obsolete: true
Attachment #367485 - Flags: ui-review?(beltzner)
Attachment #367485 - Flags: review?(mconnor)
Attachment #359754 - Flags: ui-review?(beltzner)
Attachment #359754 - Flags: review?(mconnor)
Attachment #367485 - Attachment description: patch v4 → patch v4.1
Blocks: 487830
Blocks: 487826
(In reply to comment #93)
> https://build.mozilla.org/tryserver-builds/2009-03-15_06:44-dgottwald@mozilla.com-try-c6892973413/

Dao, this build isn't available anymore. Could you kick-off a new one?
No longer blocks: 488043
Depends on: 488043
Blocks: 308492
Attached patch patch v4.1 (obsolete) — Splinter Review
updated to latest trunk

https://build.mozilla.org/tryserver-builds/2009-04-19_03:35-dgottwald@mozilla.com-try-5b6ca6be093/
Attachment #367485 - Attachment is obsolete: true
Attachment #373575 - Flags: ui-review?(beltzner)
Attachment #373575 - Flags: review?(mconnor)
Attachment #367485 - Flags: ui-review?(beltzner)
Attachment #367485 - Flags: review?(mconnor)
how about implementing here the idea of bug 404416: Show keyboard shortcuts (⌘1 through ⌘9) in the "All Tabs" menu ?
(In reply to comment #98)
> how about implementing here the idea of bug 404416: Show keyboard shortcuts (⌘1
> through ⌘9) in the "All Tabs" menu ?

That doesn't seem to fit with the UI here.
https://build.mozilla.org/tryserver-builds/dgottwald@mozilla.com-try-569ad6b9fbc/

Note that the Ctrl-Tab extension now basically has the same UI if you use it with Firefox 3.5:
https://addons.mozilla.org/firefox/addon/5244
Blocks: 496060
Blocks: 498272
Attached patch patch v4.2 (obsolete) — Splinter Review
fixed a focus issue

http://build.mozilla.org/tryserver-builds/dgottwald@mozilla.com-try-3e432e227dcb
Attachment #349291 - Attachment is obsolete: true
Attachment #359670 - Attachment is obsolete: true
Attachment #373575 - Attachment is obsolete: true
Attachment #373575 - Flags: ui-review?(beltzner)
Attachment #373575 - Flags: review?(mconnor)
Blocks: 503532
Blocks: 482065
Attached patch patch v5 (obsolete) — Splinter Review
This adds a pref for enabling/disabling the all-tabs panel, similar to the existing one for Ctrl+Tab.

Prefs are:
browser.allTabs.previews
browser.ctrlTab.previews
Attachment #387741 - Attachment is obsolete: true
Blocks: 503382
(In reply to comment #102)
> browser.ctrlTab.previews

Are there plans to make the CTRL+Tab panel OFF and drop the MRU by default? Both are real usability killers. (I *was* a heavy CTRL+Tab user until the panel & MRU ruined it for me)
(In reply to comment #102)
> Created an attachment (id=387918) [details]
> patch v5
> 
> This adds a pref for enabling/disabling the all-tabs panel, similar to the
> existing one for Ctrl+Tab.
> 
> Prefs are:
> browser.allTabs.previews
> browser.ctrlTab.previews

https://build.mozilla.org/tryserver-builds/dgottwald@mozilla.com-try-e1f7c48d628c/
Comment on attachment 387918 [details] [diff] [review]
patch v5

>+# Ctrl-Tab
>+ctrlTab.showAll.label=Show all %S tabs

This is a good candidate for PluralForm ;)
(In reply to comment #102)
> This adds a pref for enabling/disabling the all-tabs panel, similar to the
> existing one for Ctrl+Tab.
> 
> Prefs are:
> browser.allTabs.previews
> browser.ctrlTab.previews

If you require two designs of .tabs-alltabs-button,
I hope this for the theme developer. 
Now, the design switch of .tabs-alltabs-box-animate is not easy. 

Plan 1:
<stack class="tabs-alltabs-stack" previews="true/false">
  <hbox class="tabs-alltabs-box-animate"/>
  <toolbarbutton class="tabs-alltabs-button">
   <menupopup class="tabs-alltabs-popup"/>
  </toolbarbutton>
</stack>

browser.allTabs.previews;true -> <stack class="tabs-alltabs-stack" previews="true">
browser.allTabs.previews;false -> <stack class="tabs-alltabs-stack" previews="false">

Plan 2:
<stack>
  <hbox class="tabs-alltabs-box-animate" previews="true/false"/>
  <toolbarbutton class="tabs-alltabs-button">
   <menupopup class="tabs-alltabs-popup"/>
  </toolbarbutton>
</stack>

browser.allTabs.previews;true -> <hbox class="tabs-alltabs-box-animate previews="true">
browser.allTabs.previews;false -> <hbox class="tabs-alltabs-box-animate" previews="false">
(In reply to comment #105)
> >+# Ctrl-Tab
> >+ctrlTab.showAll.label=Show all %S tabs
> 
> This is a good candidate for PluralForm ;)

Indeed, thanks.

(In reply to comment #106)
> If you require two designs of .tabs-alltabs-button,
> I hope this for the theme developer.

A theme may provide a different design depending on the all-tabs pref. The tryserver build from above does that on Windows and Linux, but not on OS X.

> Now, the design switch of .tabs-alltabs-box-animate is not easy.

I don't think it makes a lot of sense to style tabs-alltabs-box-animate differently depending on that pref.
Attached patch patch v5.1 (obsolete) — Splinter Review
using PluralForm
Attachment #387918 - Attachment is obsolete: true
(In reply to comment #107)
> A theme may provide a different design depending on the all-tabs pref. The
> tryserver build from above does that on Windows and Linux, but not on OS X.

You say that this is different by the design of theme. 

> I don't think it makes a lot of sense to style tabs-alltabs-box-animate
> differently depending on that pref.

New tab button, All tabs button, and Tab scrollbox arrow of Windows default theme are special designs that look like the tab. 
These are different from the design of global toolbarbutton. 

A special design might not need the change in .tabs-alltabs-box-animate. 
However, there might be a design for which the change is necessary.
This depends on the design of theme, too. 

There is no method of change now.
(In reply to comment #109)
> A special design might not need the change in .tabs-alltabs-box-animate. 
> However, there might be a design for which the change is necessary.

It might be desirable in some cases, but certainly not necessary. .tabs-alltabs-box-animate is expected to contain a texture that overlays the all-tabs button momentarily when the tab bar overflows. That works pretty much regardless of what the all-tabs does and whether it uses a different icon or even a different button appearance depending on [menu="true"].

By the way, I'm using your Past Modern theme right now and the button, including the overflow animation, seems to look right for both pref states.

Also note that browser.allTabs.previews is currently thought to be an interim solution. When we reach a point where we're confident that the new UI is a win for the majority of our users, it's likely that we would drop support for the old UI.
(In reply to comment #110)
> depending on [menu="true"].

err, [type="menu"]
No longer blocks: 308492
(In reply to comment #110)
> By the way, I'm using your Past Modern theme right now and the button,
> including the overflow animation, seems to look right for both pref states.

Well... It is not a topic of piece person of me. 

If two designs are necessary, the theme developer will design them. 
However, please enable two designs. 

> Also note that browser.allTabs.previews is currently thought to be an interim
> solution. When we reach a point where we're confident that the new UI is a win
> for the majority of our users, it's likely that we would drop support for the
> old UI.

All right.
I also hope for it. 
If the user wants old UI, it might be work of Extension.
Attached patch patch v5.2 (obsolete) — Splinter Review
Previous patches had the constraint that the tab preview image size generally depended on what would fit the Ctrl+Tab panel. The Ctrl+Tab panel resizes the images dynamically now, allowing them to be bigger in the all-tabs panel.

http://build.mozilla.org/tryserver-builds/dgottwald@mozilla.com-try-5130fa015e78
Attachment #387961 - Attachment is obsolete: true
Attached patch patch v5.2 (obsolete) — Splinter Review
The previous patch skipped the Ctrl+Tab browser chrome test if the pref is set to false, which is bad. It just sets the pref to true now.
Attachment #388054 - Attachment is obsolete: true
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
So, a few comments:

1) The panels are disabled now by default, will that be the default once this lands?
2) When using the ctrl-tab panel, the tabs are being sorted in MRU order, which brings up some usability issues:
  a) If you ctrl-tab quickly (so as not to bring up the panel), then ctrl-tab again, you land in an endless cycle of tab a (currently selected)->tab b (next in MRU order)->tab a, etc.
  b) and this is much the same as the point a, but manifests a bit differently, if you tab switch (say twice) then tab switch again, you end up in a very confusing cycle.
3) If the panels are disabled by default (1), can we get a checkbox under Options->Tabs to enable them?

I'm not sure how to go about satisfying the MRU requirement and the usability issue, but if the tabs could move around a bit less, that would certainly help. I don't think switching between the same two tabs is what the ctrl-tab panel is for.
All tabs panel:

1) keyboard bindings are a bit weird:
  a) Pressing up/down takes you up and down through the tabs from left to right, but if there is a column that doesn't have a tab on the bottom-most row, that column will be skipped.
  b) going through the tabs with left/right arrow keys goes through the tabs left to right, up and down, and then will land on the search textbox. You can't get out of the textbox with the left/right arrowkeys after that.
  c) In the textbox, hitting up/down behaves differently than other textboxes (move to begining/end of text) and moves out of the textbox.
2) On my computer, the hovering effect seems a bit slowish (I get a bit of lag between hovers), my computer is somewhat slow, so that may be expected.

IMO, the left/right arrowkeys should not move to the searchbox, it should be in the tab order only when using the up/down keys. Even then, I'm not sure how to handle (c).
(In reply to comment #116)
> 1) The panels are disabled now by default, will that be the default once this
> lands?

That's the plan, as this might need more work and polish, and 3.6 is expected to be a very quick release.

> 2) When using the ctrl-tab panel, the tabs are being sorted in MRU order, which
> brings up some usability issues:

I'm not sure you're really describing issues; it's what I'd expect from an MRU interface. See also Alt+Tab. But if you see possibilities for improving this, please spin them off to new bugs, newsgroup threads, etc.

> I don't think switching between the same two tabs is what the ctrl-tab panel is
> for.

It's actually the most important use case, as the other tab will be on the top of the user's mental memory, the one that's most likely to be used next, etc.

> 3) If the panels are disabled by default (1), can we get a checkbox under
> Options->Tabs to enable them?

We should probably consider this for the Ctrl+Tab panel, when it's enabled by default.
(In reply to comment #117)
> All tabs panel:
> 
> 1) keyboard bindings are a bit weird:
>   a) Pressing up/down takes you up and down through the tabs from left to
> right, but if there is a column that doesn't have a tab on the bottom-most row,
> that column will be skipped.

I hate that code, but I'll have a look at it once more.

>   b) going through the tabs with left/right arrow keys goes through the tabs
> left to right, up and down, and then will land on the search textbox. You can't
> get out of the textbox with the left/right arrowkeys after that.

By design, left/right keys shouldn't leave the textbox, even if it's empty. That's an a11y requirement. If left/right keys didn't move /to/ the search field, then it would be strange that up/down do. And if up/down didn't move to the search field, it would seem strange or at least inconsistent that they can leave it...

>   c) In the textbox, hitting up/down behaves differently than other textboxes
> (move to begining/end of text) and moves out of the textbox.

That's also by design, see comment 2. I think I could live without it, as keyboard users are more likely to try Tab.
Summary: Yet another Ctrl+Tab / All Tabs design revision → Yet another Ctrl+Tab / All Tabs design revision (pref'd off)
Attached patch patch v5.3 (obsolete) — Splinter Review
fixed this:

>   a) Pressing up/down takes you up and down through the tabs from left to
> right, but if there is a column that doesn't have a tab on the bottom-most row,
> that column will be skipped.

Neil kindly agreed to review this.
Attachment #388157 - Attachment is obsolete: true
Attachment #388875 - Flags: review?(enndeakin)
(In reply to comment #120)
> Created an attachment (id=388875) [details]
> patch v5.3
> 
> fixed this:
> 
> >   a) Pressing up/down takes you up and down through the tabs from left to
> > right, but if there is a column that doesn't have a tab on the bottom-most row,
> > that column will be skipped.
> 
> Neil kindly agreed to review this.

I cannot build with Debian GNU/Linux unstable.

Build log:
/usr/bin/python2.5 /home/hideo/develop/mozilla/config/JarMaker.py \
	   -j ../../../../dist/bin/chrome \
	  -t /home/hideo/develop/mozilla -f jar  -DNDEBUG -DTRIMMED -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DMOZILLA_VERSION=\"1.9.2a1pre\" -DMOZILLA_VERSION_U=1.9.2a1pre -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UNAME_DOMAINNAME_FIELD=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_ATTRIBUTE=1 -DHAVE_DIRENT_H=1 -DHAVE_GETOPT_H=1 -DHAVE_SYS_BITYPES_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_GNU_LIBC_VERSION_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_SYS_CDEFS_H=1 -DHAVE_LIBM=1 -DHAVE_LIBDL=1 -DHAVE_DLADDR=1 -DFUNCPROTO=15 -DHAVE_XSHM=1 -DHAVE_FT_BITMAP_SIZE_Y_PPEM=1 -DHAVE_FT_GLYPHSLOT_EMBOLDEN=1 -DHAVE_FT_LOAD_SFNT_TABLE=1 -DHAVE_FT_SELECT_SIZE=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_STAT64=1 -DHAVE_LSTAT64=1 -DHAVE_TRUNCATE64=1 -DHAVE_SETBUF=1 -DHAVE_ISATTY=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_RES_NINIT=1 -DHAVE_GNU_GET_LIBC_VERSION=1 -DHAVE_LANGINFO_CODESET=1 -DVA_COPY=va_copy -DHAVE_VA_COPY=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_EMBEDDING_LEVEL_DEFAULT=1 -DMOZ_EMBEDDING_LEVEL_BASIC=1 -DMOZ_EMBEDDING_LEVEL_MINIMAL=1 -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_DEFAULT_TOOLKIT=\"cairo-gtk2\" -DMOZ_X11=1 -DMOZ_WIDGET_GTK2=1 -DMOZ_ENABLE_XREMOTE=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DMOZ_PANGO=1 -DMOZ_ENABLE_GCONF=1 -DMOZ_ENABLE_GNOMEUI=1 -DMOZ_ENABLE_DBUS=1 -DMOZ_ENABLE_LIBNOTIFY=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM_OBSOLETE=1 -DMOZ_OGG=1 -DATTRIBUTE_ALIGNED_MAX=64 -DMOZ_WAVE=1 -DMOZ_SYDNEYAUDIO=1 -DMOZ_MEDIA=1 -DHAVE_LIBASOUND=1 -DMOZ_XTF=1 -DMOZ_CRASHREPORTER=1 -DHAVE_CURL_CURL_H=1 -DMOZ_CRASHREPORTER_ENABLE_PERCENT=100 -DMOZ_MATHML=1 -DMOZ_ENABLE_CANVAS=1 -DMOZ_SVG=1 -DMOZ_SMIL=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_PLACES=1 -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DSIZEOF_INT_P=4 -DMOZ_MEMORY_SIZEOF_PTR_2POW=2 -DMOZ_MEMORY=1 -DMOZ_MEMORY_LINUX=1 -DMOZ_ENABLE_OLD_ABI_COMPAT_WRAPPERS=1 -DHAVE___CXA_DEMANGLE=1 -DHAVE__UNWIND_BACKTRACE=1 -DMOZ_USER_DIR=\".mozilla\" -DMOZ_ENABLE_LIBXUL=1 -DHAVE_STDINT_H=1 -DHAVE_INTTYPES_H=1 -DMOZ_TREE_CAIRO=1 -DHAVE_UINT64_T=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_RDF=1 -DMOZ_MORKREADER=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -DMOZ_ACCESSIBILITY_ATK=1 -DATK_MAJOR_VERSION=1 -DATK_MINOR_VERSION=26 -DATK_REV_VERSION=0  \
	  /home/hideo/develop/mozilla/browser/themes/gnomestripe/browser/jar.mn
processing /home/hideo/develop/mozilla/browser/themes/gnomestripe/browser/jar.mn
Traceback (most recent call last):
  File "/home/hideo/develop/mozilla/config/JarMaker.py", line 463, in <module>
    main()
  File "/home/hideo/develop/mozilla/config/JarMaker.py", line 460, in main
    jardir=options.j)
  File "/home/hideo/develop/mozilla/config/JarMaker.py", line 231, in makeJar
    localedirs)
  File "/home/hideo/develop/mozilla/config/JarMaker.py", line 300, in processJarSection
    outHelper, jf)
  File "/home/hideo/develop/mozilla/config/JarMaker.py", line 332, in _processEntryLine
    raise RuntimeError("file not found: " + src)
RuntimeError: file not found: KUI-close.png
make[7]: *** [libs] エラー 1
(In reply to comment #122)
Chances are that you didn't apply the patch correctly. You need to use hg import or git-apply.
Comment on attachment 388875 [details] [diff] [review]
patch v5.3

>+var tabPreviewPanelHelper = {
>+  opening: function (host) {
>+    host.panel.hidden = false;
>+
>+    var handler = this._generateHandler(host);
>+    host.panel.addEventListener("popuphiding", handler, false);
>+    host.panel.addEventListener("popupshown", handler, false);
>+

These listeners don't seem to get removed anywhere.


>+    host.panel.setAttribute("norestorefocus", "true");

Can this just be set in the xul directly?


>+  _popupshown: function (host) {
>+    if ("setupGUI" in host)
>+      host.setupGUI();
>+  },
>+  _popuphiding: function (host) {

Do these two event handlers deal with the case where a child popup is closed? For instance, the filter field's context menu?


>+  get selected () this._selectedIndex < 0 ?
>+                    document.activeElement :
>+                    this.thumbnails.item(this._selectedIndex),

Why document.activeElement?


>   updatePreview: function ctrlTab__updatePreview(aThumbnail, aTab) {
>+    if (aThumbnail == this.showAllButton)
>+      return;
>+
>     do {
>       if (aThumbnail._tab) {
>         if (aThumbnail._tab == aTab)
>           break;
>         aThumbnail._tab.removeEventListener("DOMAttrModified", this, false);
>       }
>       aThumbnail._tab = aTab;
>       if (aTab)
>         aTab.addEventListener("DOMAttrModified", this, false);
>     } while (false);

Why is a loop used here?


>+  advanceFocus: function ctrlTab_advanceFocus(aForward) {
...
>+      } while (this.selected.hidden);

When would a tab button in the ctrlTab view be hidden?


>+  showAllTabs: function ctrlTab_pick(aThumbnail) {

ctrlTab_pick should ctrlTab_showAllTabs

Also, the functions are inconsistent whether they use one or two underscores.


>+  open: function ctrlTab_open() {
>+    if (this.isOpen)
>+      return;
>+
>+    allTabs.close();
>+
>+    document.addEventListener("keyup", this, true);

To ensure it gets removed, is 'close' always guaranteed to be called when the key listener is no longer needed?


>+  _openPanel: function ctrlTab_openPanel() {
>+    tabPreviewPanelHelper.opening(this);
>+
>+    this.panel.width = Math.min(screen.availWidth * .99,
>+                                this.canvasWidth * 1.25 * this.tabThumbnailCount);

Why .99 ? Are we just wanting to account for a border of some kind, or do you specifically mean to make the size just a bit smaller than the screen? The height, or the other hand, doesn't have this check.


>+  setupGUI: function ctrlTab_setupGUI() {
>+    this.selected.focus();
>+    this._selectedIndex = -1;

I don't understand what -1 means here.


>+    this._trackMouseOver = false;
>+    setTimeout(function (self) {
>+      if (self.isOpen)
>+        self._trackMouseOver = true;
>+    }, 0, this);
>+  },

What is this timer for? Add a comment here if it is needed to indicate what this is waiting for.


>         if (isOpen && event.ctrlKey) {
>           switch (event.charCode) {
>-            case this.closeCharCode:
>-              gBrowser.removeTab(this.selected._tab);
>+            case this.keys.close:
>+              this.remove(this.selected);
>               break;

Should the delete key remove the tab as well? 


>+  readPref: function allTabs_readPref() {
>+    var allTabsButton = document.getAnonymousElementByAttribute(
>+                          gBrowser.tabContainer, "anonid", "alltabs-button");
>+    if (gPrefService.getBoolPref(this.prefName)) {
>+      allTabsButton.removeAttribute("type");
>+      allTabsButton.setAttribute("command", "Browser:ShowAllTabs");
>+    } else {
>+      allTabsButton.setAttribute("type", "menu");
>+      allTabsButton.removeAttribute("command");
>+      allTabsButton.removeAttribute("oncommand");

This removes the oncommand, but never sets it again. Is that intended?


>+  closeTab: function allTabs_closeTab(event) {
>+    if (event.type != "click" || event.button == 1) {

I only see closeTab being called from a command event listener.


>+  open: function allTabs_open(aToggle) {

open doesn't seem to be called with an argument anywhere.


>+    this.panel.addEventListener("keypress", this, true);
>+    this._browserCommandSet.addEventListener("command", this, false);

Is these listener supposed to be removed when the panel is closed?


>+          case event.DOM_VK_RETURN:
>+            if (event.target == this.filterField) {
>+              // If there's a pending search, kick it off now.
>+              if (this.filterField._timer)

This shouldn't be relying on a field of the textbox that supposed to be private.


>+  _addBox: function allTabs_addBox(aTab) {
>+    var box = document.createElement("button");
>+    box.setAttribute("class", "allTabs-thumbnail");

box.className = "allTabs-thumbnail";


>+  _getBox: function allTabs_getBox(aTab) {
>+    var boxes = this.container.childNodes;
>+    for (let i = 0; i < boxes.length; i++)
>+      if (boxes[i]._tab == aTab)
>+        return boxes[i];
>+    return null;
>+  },

Callers of _getBox don't null-check the result. Is the return null just to avoid a warning?


>+      this.tabCloseButton.left = thumbnail.left -
>+                                 container.left +
>+                                 parseInt(this.container.left) +
>+                                 (alignLeft ? 0 :
>+                                  thumbnail.width - this.tabCloseButton.boxObject.width);

I'd be consistent and use getBoundingClientRect instead of the box object here as well.


>+  _updatePreview: function allTabs_updatePreview(aBox) {
>+    aBox.setAttribute("label", aBox._tab.label);
>+    aBox.setAttribute("tooltiptext", aBox._tab.label);
>+    aBox.setAttribute("crop", aBox._tab.crop);
>+    if (aBox._tab.image)
>+      aBox.setAttribute("icon", aBox._tab.image);
>+    else
>+      aBox.removeAttribute("icon");

Many of there can be set as properties instead of attributes.


>+
>+  _onKeyPress: function allTabs_onKeyPress(event) {

Why does allTabs have two different key listeners? If they do different things, this should be clearer or commented.


>+      switch (event.keyCode) {
>+        case event.DOM_VK_UP:
>+          if (this._visible) {
>+            let boxes = this._visibleBoxes;
>+            boxes[Math.floor(boxes.length / this._columns) * this._columns - 1].focus();

Can't Math.floor(boxes.length / this._columns) end up being 0?


>+  _advanceFocusVertically: function allTabs_advanceFocusVertically(event) {
>+    var box = document.commandDispatcher.focusedElement;

Active element should be sufficient here.

You use the word box a lot here. Maybe it would clearer to use a word that describes them better?


>diff --git a/browser/base/content/browser-tabPreviews.xml b/browser/base/content/browser-tabPreviews.xml
>+<!DOCTYPE bindings [
>+<!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">
>+%browserDTD;
>+]>

I don't any of the entities being used anywhere.


>+        <xul:hbox class="ctrlTab-favicon-container" xbl:inherits="hidden=noicon">
>+          <xul:image class="ctrlTab-favicon" xbl:inherits="src=icon"/>
>+        </xul:hbox>

Why not just use 'image' instead of 'icon', then you can set it with .image


>+/* ::::: Keyboard UI Panel ::::: */
>+.KUI-panel-closebutton {
>+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image");

There should be a means of referring to this binding via a type attribute or other tag. If not, a bug should be created. A general imagebutton type element is certainly needed.


>-#ctrlTab-pages {
>+#allTabs-tab-close-button {
>+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image");
>+}
...
>+#allTabs-tab-close-button {
>+  margin: 0;

You could merge these two rules.


>   tabPreviews.init();
>-  ctrlTab.init();
>+  ctrlTab.readPref();
>+  gPrefService.addObserver(ctrlTab.prefName, ctrlTab, false);
>+  gPrefService.addObserver(allTabs.prefName, allTabs, false);

I wonder if it would be better to just have one observer listening to a pref branch, and name the preferences to fit in one hierarchy.


>+/* All Tabs */
>+
>+#allTabs-panel {
>+  /* compensate the widget border and padding to center the panel correctly */
>+  margin-left: -13px;
>+}

'compenstate *for* the widget border...'

Which border is this anyway?


>+.allTabs-thumbnail-label {
>+  -moz-transform: translate(0, -1px);
>+}

This rule would be better grouped with the other thumbnail rules.
Responding to the comments that need a response, the others will be addressed directly:

> >+var tabPreviewPanelHelper = {
> >+  opening: function (host) {
> >+    host.panel.hidden = false;
> >+
> >+    var handler = this._generateHandler(host);
> >+    host.panel.addEventListener("popuphiding", handler, false);
> >+    host.panel.addEventListener("popupshown", handler, false);
> >+
> 
> These listeners don't seem to get removed anywhere.

They're removed in the handler that _generateHandler returns.

> >+    host.panel.setAttribute("norestorefocus", "true");
> 
> Can this just be set in the xul directly?

Sure, it can. I figured it would make more sense here, since tabPreviewPanelHelper takes over the focus restoring.

> >+  _popupshown: function (host) {
> >+    if ("setupGUI" in host)
> >+      host.setupGUI();
> >+  },
> >+  _popuphiding: function (host) {
> 
> Do these two event handlers deal with the case where a child popup is closed?
> For instance, the filter field's context menu?

Yep, _generateHandler takes care of that.

> >+  get selected () this._selectedIndex < 0 ?
> >+                    document.activeElement :
> >+                    this.thumbnails.item(this._selectedIndex),
> 
> Why document.activeElement?

Because keeping track of _selectedIndex seems like an avoidable complication. When the panel is open, document.activeElement should always point to the selected item.

> >     do {
> >       if (aThumbnail._tab) {
> >         if (aThumbnail._tab == aTab)
> >           break;
> >         aThumbnail._tab.removeEventListener("DOMAttrModified", this, false);
> >       }
> >       aThumbnail._tab = aTab;
> >       if (aTab)
> >         aTab.addEventListener("DOMAttrModified", this, false);
> >     } while (false);
> 
> Why is a loop used here?

So that 'break' can be used ;)

> >+  advanceFocus: function ctrlTab_advanceFocus(aForward) {
> ...
> >+      } while (this.selected.hidden);
> 
> When would a tab button in the ctrlTab view be hidden?

When there are more provided buttons than actual tabs. For instance when you press Ctrl-Tab with three tabs in that browser window.

> >+  open: function ctrlTab_open() {
> >+    if (this.isOpen)
> >+      return;
> >+
> >+    allTabs.close();
> >+
> >+    document.addEventListener("keyup", this, true);
> 
> To ensure it gets removed, is 'close' always guaranteed to be called when the
> key listener is no longer needed?

The listener gets removed in the admittedly weirdly-named suspendGUI method, which is guaranteed to be called.

> >+    this.panel.width = Math.min(screen.availWidth * .99,
> >+                                this.canvasWidth * 1.25 * this.tabThumbnailCount);
> 
> Why .99 ? Are we just wanting to account for a border of some kind, or do you
> specifically mean to make the size just a bit smaller than the screen?

Yeah, just to make it a bit smaller than the screen.

> The height, on the other hand, doesn't have this check.

The height is just estimated for the vertical alignment, not actually set. The vertical screen bounds shouldn't be reached, but if they were, I don't think limiting the estimated height would help.

> >+  setupGUI: function ctrlTab_setupGUI() {
> >+    this.selected.focus();
> >+    this._selectedIndex = -1;
> 
> I don't understand what -1 means here.

_selectedIndex is used to track the selected item as long as the panel isn't visible (for bug 498272). When the panel is open, it's not used anymore.

> >         if (isOpen && event.ctrlKey) {
> >           switch (event.charCode) {
> >-            case this.closeCharCode:
> >-              gBrowser.removeTab(this.selected._tab);
> >+            case this.keys.close:
> >+              this.remove(this.selected);
> >               break;
> 
> Should the delete key remove the tab as well?

I guess it would make sense. That idea didn't come up before.

> >+    if (gPrefService.getBoolPref(this.prefName)) {
> >+      allTabsButton.removeAttribute("type");
> >+      allTabsButton.setAttribute("command", "Browser:ShowAllTabs");
> >+    } else {
> >+      allTabsButton.setAttribute("type", "menu");
> >+      allTabsButton.removeAttribute("command");
> >+      allTabsButton.removeAttribute("oncommand");
> 
> This removes the oncommand, but never sets it again. Is that intended?

The oncommand gets added automatically based on the command attribute. I don't know why.

> >+  closeTab: function allTabs_closeTab(event) {
> >+    if (event.type != "click" || event.button == 1) {
> 
> I only see closeTab being called from a command event listener.

Seems like some legacy code slipped in. Will check/fix.

> >+  open: function allTabs_open(aToggle) {
> 
> open doesn't seem to be called with an argument anywhere.

ditto

> >+    this.panel.addEventListener("keypress", this, true);
> >+    this._browserCommandSet.addEventListener("command", this, false);
> 
> Is these listener supposed to be removed when the panel is closed?

Although the listener just calls this.close, which is a no-op when the panel is closed, it would make a lot of sense to remove it before that...

> >+          case event.DOM_VK_RETURN:
> >+            if (event.target == this.filterField) {
> >+              // If there's a pending search, kick it off now.
> >+              if (this.filterField._timer)
> 
> This shouldn't be relying on a field of the textbox that supposed to be
> private.

This is for bug 463635, but I think I can solve it differently.

> Callers of _getBox don't null-check the result. Is the return null just to
> avoid a warning?

Yes.

> >+  _onKeyPress: function allTabs_onKeyPress(event) {
> 
> Why does allTabs have two different key listeners? If they do different things,
> this should be clearer or commented.

One uses capture and the other doesn't. I'll see if the handlers can be merged, maybe checking event.eventPhase.

> >+/* ::::: Keyboard UI Panel ::::: */
> >+.KUI-panel-closebutton {
> >+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image");
> 
> There should be a means of referring to this binding via a type attribute or
> other tag. If not, a bug should be created. A general imagebutton type element
> is certainly needed.

Filed bug 504945.

> >   tabPreviews.init();
> >-  ctrlTab.init();
> >+  ctrlTab.readPref();
> >+  gPrefService.addObserver(ctrlTab.prefName, ctrlTab, false);
> >+  gPrefService.addObserver(allTabs.prefName, allTabs, false);
> 
> I wonder if it would be better to just have one observer listening to a pref
> branch, and name the preferences to fit in one hierarchy.

I'm open to that if it ends up being more convenient, but I can't even think of appropriate names.

> >+/* All Tabs */
> >+
> >+#allTabs-panel {
> >+  /* compensate the widget border and padding to center the panel correctly */
> >+  margin-left: -13px;
> >+}
> 
> 'compenstate *for* the widget border...'
> 
> Which border is this anyway?

Not a CSS border but something that -moz-appearance:menupopup enforces.
> >+  _updatePreview: function allTabs_updatePreview(aBox) {

> >+    aBox.setAttribute("label", aBox._tab.label);

> >+    aBox.setAttribute("tooltiptext", aBox._tab.label);

> >+    aBox.setAttribute("crop", aBox._tab.crop);

> >+    if (aBox._tab.image)

> >+      aBox.setAttribute("icon", aBox._tab.image);

> >+    else

> >+      aBox.removeAttribute("icon");

> 

> Many of there can be set as properties instead of attributes.

Most of these properties would only work if the binding is already constructed, which isn't necessarily the case.
Attached patch patch v5.4 (obsolete) — Splinter Review
I replaces the inconsistent uses of "thumbnail" and "box" with "preview".

Not sure what this exactly means:

> >+.allTabs-thumbnail-label {
> >+  -moz-transform: translate(0, -1px);
> >+}
> 
> This rule would be better grouped with the other thumbnail rules.
Attachment #388875 - Attachment is obsolete: true
Attachment #389235 - Flags: review?(enndeakin)
Attachment #388875 - Flags: review?(enndeakin)
Attached patch patch v5.4 (obsolete) — Splinter Review
just missed some occurrences of "thumbnail"
Attachment #389235 - Attachment is obsolete: true
Attachment #389238 - Flags: review?(enndeakin)
Attachment #389235 - Flags: review?(enndeakin)
> Not sure what this exactly means:
> 
> > >+.allTabs-thumbnail-label {
> > >+  -moz-transform: translate(0, -1px);
> > >+}
> > 
> > This rule would be better grouped with the other thumbnail rules.

I just meant that you have some thumbnail rules, followed by some rules that apply to the panel in general, then this last thumbnail rule. It would be better to move this last thumbnail-related rule up with the others.
(In reply to comment #129)
> I just meant that you have some thumbnail rules, followed by some rules that
> apply to the panel in general, then this last thumbnail rule. It would be
> better to move this last thumbnail-related rule up with the others.

The idea was that the first set of rules are shared by both panels, followed by a set that's specific to Ctrl-Tab and another one for All Tabs. Of course, that's not necessarily the best way to organize things.
Comment on attachment 389238 [details] [diff] [review]
patch v5.4

> >+    host.panel.setAttribute("norestorefocus", "true");
> 
> Can this just be set in the xul directly?

It would be simpler and less work to set it in the xul directly though, rather than attempting to set it repeatedly.


> > Why is a loop used here?
> 
> So that 'break' can be used ;)

I'd just write it as:

if (aThumbnail._tab != aTab) {
  if (aThumbnail._tab)
    aThumbnail._tab.removeEventListener("DOMAttrModified", this, false);
  aThumbnail._tab = aTab;
  if (aTab)
    aTab.addEventListener("DOMAttrModified", this, false);
}


> _selectedIndex is used to track the selected item as long as the panel isn't
> visible (for bug 498272). When the panel is open, it's not used anymore.

Does _selectedIndex get reset when the popup is closed?


> +  _onKeyPress: function allTabs_onKeyPress(event) {
> +    if (event.eventPhase == event.CAPTURING_PHASE) {

Perhaps use a sepearate function _onCapturingKeyPress ?


>+      switch (event.keyCode) {
>+        case event.DOM_VK_UP:
>+          if (this._visible) {
>+            let boxes = this._visibleBoxes;
>+            boxes[Math.floor(boxes.length / this._columns) * this._columns - 1].focus();
>
> Can't Math.floor(boxes.length / this._columns) end up being 0?

Actually,thinking about this some more, I'd expect that pressing up from the filter field would just select the last preview.


The two keypress handlers on allTabs are added every time the popup is opened, (although it will overwrite), but are only removed when closing the browser. Would it be more optimal to remove them on popup close as well?
(In reply to comment #132)
> > _selectedIndex is used to track the selected item as long as the panel isn't
> > visible (for bug 498272). When the panel is open, it's not used anymore.
> 
> Does _selectedIndex get reset when the popup is closed?

It gets reset when the popup opens, right after the selected preview is focused (see setupGUI method).

> Actually,thinking about this some more, I'd expect that pressing up from the
> filter field would just select the last preview.

The problem with that is, the last preview can be in the first column, and from there you couldn't cycle through the previews by pressing up repeatedly.
Attached patch patch v5.5Splinter Review
Moved the norestorefocus attribute, rewrote the non-looping loop, added _onCapturingKeyPress and moved the keypress listener removal to when the popup closes.
Attachment #389238 - Attachment is obsolete: true
Attachment #389522 - Flags: review?(enndeakin)
Attachment #389238 - Flags: review?(enndeakin)
>+    if ((aPreview._tab || null) != aTab) {
>+      if (aPreview._tab)

Why do you need the special check against null here?


> _selectedIndex is used to track the selected item as long as the panel isn't
> visible (for bug 498272). When the panel is open, it's not used anymore.

> Does _selectedIndex get reset when the popup is closed?

What I mean is that selectedIndex appears to only be updated when the popup is not open, and reset when it is about to be opened.
Is it used for some purpose while it is closed?


> Perhaps use a sepearate function _onCapturingKeyPress ?

Actually, I meant a separate function where _onKeyPress is called, but it isn't a big issue.


> The problem with that is, the last preview can be in the first column, and from
> there you couldn't cycle through the previews by pressing up repeatedly.

I wouldn't expect to be able to. I'd expect pressing up to go to the last item, and then pressing up again would move up a row.
(In reply to comment #135)
> >+    if ((aPreview._tab || null) != aTab) {
> >+      if (aPreview._tab)
> 
> Why do you need the special check against null here?

If _tab doesn't exist, comparing it with something other than 'null' or 'undefined' will produce a strict warning.

> What I mean is that selectedIndex appears to only be updated when the popup is
> not open, and reset when it is about to be opened.
> Is it used for some purpose while it is closed?

No, it's not used after that. ctrlTab.selected shouldn't accessed either at that point. Maybe it should throw if ctrlTab.isOpen is false...

> > The problem with that is, the last preview can be in the first column, and from
> > there you couldn't cycle through the previews by pressing up repeatedly.
> 
> I wouldn't expect to be able to. I'd expect pressing up to go to the last item,
> and then pressing up again would move up a row.

Right, but the interesting part starts when you reach the first row. Unless you want to be captured in that column, we need to move to another column. So, cycling happens anyway. But your suggested way would cycle through a random subset of the previews.
(In reply to comment #136)
> > What I mean is that selectedIndex appears to only be updated when the popup is
> > not open, and reset when it is about to be opened.
> > Is it used for some purpose while it is closed?
> 
> No, it's not used after that. ctrlTab.selected shouldn't accessed either at
> that point. Maybe it should throw if ctrlTab.isOpen is false...

OK, so when ctrl+tab is pressed but before the popup is open, selectedIndex will be 1. If advanceFocus is called at this point, selectedIndex will be updated. But while the popup is open, selectedIndex will be -1 and not needed.

So essentially, selectedIndex is only needed for the 100ms between when ctrl+tab is pressed and the popup is opened.

Is this correct?
That's correct.
Attachment #389522 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/0bcfdbd54fbc

Prefs are browser.ctrlTab.previews and browser.allTabs.previews. Nightly testers and whoever's interested should feel free to use them and report bugs.

Also, issues mentioned in this bug that haven't been resolved yet should be spun off to new bugs.

Thanks everyone.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
No longer depends on: 473152
Comment on attachment 389522 [details] [diff] [review]
patch v5.5

>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
 
>+# Ctrl-Tab
>+# LOCALIZATION NOTE (ctrlTab.showAll.label): #1 represents the number
>+# of tabs in the current browser window. It will always be 2 at least.
>+# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>+ctrlTab.showAll.label=;Show all #1 tabs

Will this button be visible when there are two tabs in a window, or is the lowest number of tabs 3? When I test this with two tabs, Ctrl+Tab simply switches to the other tab.

When localizing this to Swedish, I would have to use a different pronoun if there are two tabs ("all two" would be incorrect). Unfortunately, our current plural rule doesn't allow this.
(In reply to comment #140)
> Will this button be visible when there are two tabs in a window, or is the
> lowest number of tabs 3? When I test this with two tabs, Ctrl+Tab simply
> switches to the other tab.

The panel will only show up when there are at least three tabs. However, while it's open, closing a tab with Ctrl+W can lead to two tabs.
Blocks: 464122
Is it right that I don't get a glass dialog when I want to list all tabs on Windows 7? The background of the Ctrl+Tab dialog is glass, but when I list all tabs I just get a gray background. 
(Using English Windows 7 build 7600)
(In reply to comment #142)
> Is it right that I don't get a glass dialog when I want to list all tabs on
> Windows 7?

It's currently disabled because our aero glass support is broken. See https://bugzilla.mozilla.org/showdependencytree.cgi?id=458866&hide_resolved=1
(In reply to comment #143)
> 
> It's currently disabled because our aero glass support is broken. See
> https://bugzilla.mozilla.org/showdependencytree.cgi?id=458866&hide_resolved=1

Alright, I just wondered because it worked some hours ago.
Depends on: 505789
Blocks: 479590
This bug appears to be very similar in nature to the work being done for bug 501490 and bug 474056 to implement the Windows 7 taskbar previews per tab.  

I couldn't help but notice while this is a great feature, where does the mention of using Ctrl+Tab come into play with this feature?  

If this is to show All tabs, the tab previews are borderline on distinguishable and illegibly on a full window having 4 tabs open on a 1024x768 display.  This window might look better if it didn't look like iTunes meets Linux on Windows (ie it doesn't at least follow basic theme or platform specific rules, which if we want this to be perf'd on by default, this should be reviewed).  For example, the spacing could be adjusted down and look more like a native dialog or popup window. The large close button X is out of place on a standard windows platform.
Blocks: 502460
No longer blocks: 513982
Depends on: 513982
What improvements are we going for with switching the design from the 3.5.x branch Showcase View (list all tabs) with this?
Marking as verified fixed. Nothing major has been regressed. If regressions arise those should be handled as new bugs.
Status: RESOLVED → VERIFIED
Depends on: 469774
No longer depends on: 463574
Given my comment 9 on this bug I have filed a couple of new bugs and marked them dependent on bug 505404.
You need to log in before you can comment on or make changes to this bug.