Closed Bug 318168 (tabbedbrowsing) Opened 18 years ago Closed 17 years ago

Improve tabbed browsing (tab strip overflow, in particular)

Categories

(Firefox :: Tabbed Browser, defect, P1)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 2 beta1

People

(Reporter: mconnor, Assigned: moco)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: fixed1.8.1, meta, Whiteboard: 181b1+)

Attachments

(4 files, 14 obsolete files)

screen shot (on windows)
108.92 KB, image/jpeg
Details
screen shot of this feature on the mac
93.20 KB, image/tiff
Details
patch updated with feedback from mconnor
34.39 KB, patch
moco
: review+
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review
all-in-one branch patch of latest scrollbox and tabbrowser fixes
36.87 KB, patch
moco
: review+
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review 
Tracking bug for specific and targeted improvements for the Firefox 2 product cycle
Alias: tabbedbrowsing
Keywords: meta
Depends on: 320638
Depends on: 320639
Depends on: 320989
Depends on: 324321
Status: NEW → ASSIGNED
Flags: blocking-firefox2+
Priority: -- → P1
Target Milestone: --- → Firefox 2 beta1
Depends on: 236721, 281012, 322898, 324604
Depends on: 333000
No longer depends on: 333000
Attached patch wip patch (works on windows) (obsolete) — — Splinter Review 
known issues

needs changes to pinstripe tabbrowser-tabs binding (Mac theme) to work on Mac

fixes
bug 327562
bug 322898
bug 221684
the patch also includes support for different tab closebutton locations (all/active/none/on right).  The cost is minimal, and was done as part of a reworking of the css bits to handle the tab clipping and one tab cases in a cleaner way.  I don't expect to expose UI for this at present.
load-balancing tabbrowser stuff to sspitzer
Assignee: mconnor → sspitzer
Status: ASSIGNED → NEW
gah.  s/.org/.com/!
Assignee: sspitzer → sspitzer
>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

>-            t.minWidth = 30;
>+            t.minWidth = 140;

140px minwidth for tabs? That's only 5 on 800x600 and 6/7 on 1024x768... I'd have thought something like 70px would be more suitable, even though we're fixing tab overflow.

>+            // We're committed to closing the tab now.  Dispatch a notification.
>+            // We dispatch it before any teardown so that event listeners can
>+            // inspect the tab that's about to close.
>+            var evt = document.createEvent("Events");
>+            evt.initEvent("TabClose", true, false);
>+            aTab.dispatchEvent(evt);

Might it make sense to have a cancelable TabClose event, followed by a non-cancelable TabUnLoad event as (OTH) happens for windows?

>-                // Only capture clicks on tabbox.xml's <spacer>
>-                aEvent.originalTarget.localName == "spacer") {
>+                aEvent.originalTarget.localName == "box") {
>+              // xxx this needs to check that we're in the empty area of the tabstrip

Doing it this way is going to be very complicated, as some skins leave gaps between tabs which you can doubleclick through.

>-      <xul:hbox flex="1" style="min-width: 1px;">
>+      <xul:arrowscrollbox anonid="arrowscrollbox" orient="horizontal" flex="1" style="min-width: 1px;">
>         <children includes="tab"/>
>-        <xul:spacer class="tabs-right" flex="1"/>
>+      </xul:arrowscrollbox>

Surely the one conclusion of all the discussion on bug 221684 and bug 155325 was that anything *but* a horizontal arrowscrollbox should be used?
For example: if tabs are 140px wide, a user has 31 tabs open, and they want to go from the first to the last, this would take a painstaking 13 seconds! For 100 tabs, 42 seconds... (measured on a fast computer fwiw). And since tabs continue to open at the far right of the tab bar, scrolling all the way to the end of your tabs (and back) is a common task.
Not to mention the lack of any way of telling how far you've scrolled, and how hard it is to stop exactly where you want.

>+      <field name="mTabClipWidth">130</field>

Surely this needs to be at least 140, if you make the minWidth 140?

>+      <method name="_handleTabSelect">
>+        <body><![CDATA[
>+          this.mTabstrip.scrollBoxObject.ensureElementIsVisible(this.selectedItem);
>+        ]]></body>
>+      </method>

It would be nicer if we tried to make the tabs to either side visible too, as switching to the next/previous tab is extremely common (and just knowing what it is can be useful), e.g.:

+          if (this.selectedItem.previousSibling)
+            this.mTabstrip.scrollBoxObject.ensureElementIsVisible(this.selectedItem.previousSibling);
+          if (this.selectedItem.nextSibling)
+            this.mTabstrip.scrollBoxObject.ensureElementIsVisible(this.selectedItem.nextSibling);
+          this.mTabstrip.scrollBoxObject.ensureElementIsVisible(this.selectedItem);
Blocks: 327562
Whiteboard: [SWAG: 1d break out events, ?d for the rest
note, bug #322898 covers breaking out the events.
Status: NEW → ASSIGNED
Whiteboard: [SWAG: 1d break out events, ?d for the rest → [SWAG: 1d break out events, see #322898, abd ?d for the rest]
fix swag
Whiteboard: [SWAG: 1d break out events, see #322898, abd ?d for the rest] → [SWAG: 1d break out events, see #322898, ?d for the rest]
note, my port of mconnor's patch needs some work, 

for example, I did not port the pref observing change correctly:

JavaScript error: chrome://global/content/bindings/tabbrowser.xml, line 142: unc
aught exception: [Exception... "Component returned failure code: 0x80070057 (NS_
ERROR_ILLEGAL_VALUE) [nsIPrefBranch2.addObserver]"  nsresult: "0x80070057 (NS_ER
ROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://global/content/bindings/tab
browser.xml ::  :: line 2301"  data: no]
(In reply to comment #10)

Is that not just bug 340547?
> Is that not just bug 340547?

thanks gavin, you are right, it is the same bug.

I did make this mistake:

pb2.addObserver("browser.tabs.closeButtons", this, false);

should be:

pb2.addObserver("browser.tabs.closeButtons", this, true);

I'll ping ispiked to see if he wants any help with #340547.
another problem with my port is I need to remove the call to  self.adjustCloseButtons(1); on resize

(see http://lxr.mozilla.org/mozilla1.8/search?string=adjustCloseButtons)

in mconnor's patch, he has removed adjustCloseButtons

right now I get:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "self.adjustCloseButtons is not a function" {
file: "chrome://global/content/bindings/tabbrowser.xml" line: 2309}]' when calli
ng method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_
XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

I'll attach a new patch
I also don't need to add back the destructor, as these the observers are weak references, or they will be once #340547 is addressed.
is it possible to put the two scrollbuttons together instead on the two sides? imho, it would save time from having to move the mouse and might be easier to search a tab.
(In reply to comment #16)
> is it possible to put the two scrollbuttons together instead on the two sides?
> imho, it would save time from having to move the mouse and might be easier to
> search a tab.
> 

I agree with this. We can also take it a bit further and in addition to moving the tabs, it can also create a tabs list when right click. This can take the hassle of actually moving the tabs and easier to find content.
update:  working on porting these changes to mozilla/toolkit/themes/pinstripe/global/globalBindings.xml so that this patch works on the mac.
two issues to look into:

1)

###!!! ASSERTION: expected a XUL document: 'xuldoc', file c:/builds/bonecho/mozi
lla/content/xul/templates/src/nsXULContentBuilder.cpp, line 1447
WARNING: NS_ENSURE_TRUE(nsDoc) failed, file c:/builds/bonecho/mozilla/content/xu
l/content/src/nsXULElement.cpp, line 2553

2)

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [n
sIDOMXULElement.boxObject]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location
: "JS frame :: chrome://global/content/bindings/browser.xml :: get_docShell :: l
ine 0"  data: no]
************************************************************
I wrote an extension that provides a solution for tab bar overflow.

Link to superT 0.7.9 RC1:
http://supert.garyr.net/superT-0.7.9-fx+fl.xpi
I wrote an extension that provides a solution for tab bar overflow.

Link to superT 0.7.9:
http://supert.garyr.net/superT-0.7.9-fx+fl.xpi

The extension used code further developed from the original Too Many Tabs extension by John Mellor (Jomel).

It adds tab bar overflow menus on each side of the tab bar to show the tabs have been collapsed. It uses algorithms for determining the number of tabs and which tabs to be shown based on the minimum tab width and available tab bar space.
oops, need to fix this on the mac (tabs will not close with my latest patch)

Error: this.mTabstripClosebutton has no properties
Source File: chrome://global/content/bindings/tabbrowser.xml
Line: 2421


additionally, mconnor and I found a problem when dragging tabas around when you are scrolling tabs.  the drop indicator can be draw "too far to the right" causing weirdness such as a "very wide url bar".

the existing onDragOver method (see http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/tabbrowser.xml#1589 needs to be adjusted to account for the new arrowscrollbox.
> additionally, mconnor and I found a problem when dragging tabas around when you
> are scrolling tabs.  the drop indicator can be draw "too far to the right"
> causing weirdness such as a "very wide url bar".

I think I've got the solution.  If we use the screenX position instead of the x position of the tab and the tabbrowser to determine the drop indicator marginLeft (or marginRight if not ltr), this is no longer a problem.

for example:

old:  ind.style.marginLeft = this.mTabs[newIndex].boxObject.x 
- this.boxObject.x - 7 + 'px';

new:  ind.style.marginLeft = this.mTabs[newIndex].boxObject.screenX 
- this.boxObject.screenX - 7 + 'px';

I'll attach a revised patch once I test a little more.
I was seeing some weirdness when testing, but I think the solution is that in addition to fixing the the onDragOver to use screenX, I need to fix the getNewIndex method to also use screenX (of both the event and the tab box object)

old:

if (aEvent.clientX < this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2)

new:
if (aEvent.screenX < this.mTabs[i].boxObject.screenX + this.mTabs[i].boxObject.width / 2)

Additionally, I need to test when the bidi.browser.ui boolean pref is true to make sure everything works correctly when things are not "ltr" (left-to-right).
> I was seeing some weirdness when testing, but I think the solution is that in
> addition to fixing the the onDragOver to use screenX, I need to fix the
> getNewIndex method to also use screenX (of both the event and the tab box
> object)

yes, that was the solution.

> Additionally, I need to test when the bidi.browser.ui boolean pref is true to
> make sure everything works correctly when things are not "ltr" 
> (left-to-right).

I misunderstood that pref.  That pref enabled UI that allows you to toggle the page direction, but not the chrome.

dbaron gave me the suggestion of adding ":root { direction : rtl }" to my userChrome.css to change the chrome direction, and that worked, but not completely (as the locale.dir is still ltr instead of rtl.)

but here's what I've fixed with this latest patch:  

1) add the chromedir attribute to the autorepeatbutton button, so I could add css to scrollbox.css (for both winstripe and pinstripe) to reverse the icons when in rtl. 

2) add code to scrollByPixel so that when doing rtl, we scroll the right direction (multiply px by -1).

I still have two open issues:

1)  add rule so if drop indicator marginLeft (or marginRight, for rtl) is off the screen, we autoscroll.  this fixes a weird issue that mconnor and I observed and this will add the functionality of scrolling as you drag a tab.

2)  the error from accessing docShell from browser.xml after closing a tab (see comment #19)

thanks to mconnor for spotting some of these issues and suggesting fixes.
I'm sort of against adding chromedir based rules to Pinstripe before we're fixing the whole theme (see Bug 221824).
> I'm sort of against adding chromedir based rules to Pinstripe before we're
> fixing the whole theme (see Bug 221824).

asaf, to avoid conflicting with your work to make pinstripe rtl compatible, I could spin off a new bug with the rtl / chromedir changes I have for pinstripe from this bug into another bug that depends on bug #221824.

are you OK with with the other rtl / ltr / chromedir / bidi changes in the last patch, including the changes to winstripe?
more info about the exception (from comment #19)

using "dump(new Error().stack)" (thanks mrbkap) here's where the problem is coming from after I close a tab:

WARNING: NS_ENSURE_TRUE(nsDoc) failed, file c:/builds/bonecho/mozilla/content/xu
l/content/src/nsXULElement.cpp, line 2579
ex = [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILUR
E) [nsIDOMXULElement.boxObject]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  loc
ation: "JS frame :: chrome://global/content/bindings/browser.xml :: get_docShell
 :: line 0"  data: no]
Error()@:0
get_docShell()@chrome://global/content/bindings/browser.xml:0
get_webNavigation()@chrome://global/content/bindings/browser.xml:0
get_currentURI()@chrome://global/content/bindings/browser.xml:0
sss_saveWindowHistory([object ChromeWindow])@file:///c:/builds/bonecho/mozilla/f
f-debug/dist/bin/components/nsSessionStore.js:770
sss_collectWindowData([object ChromeWindow])@file:///c:/builds/bonecho/mozilla/f
f-debug/dist/bin/components/nsSessionStore.js:1128
([object ChromeWindow])@file:///c:/builds/bonecho/mozilla/ff-debug/dist/bin/comp
onents/nsSessionStore.js:1076
call([object Object],[object ChromeWindow])@:0
sss_forEachBrowserWindow((function (aWindow) {if (this._dirty || this._dirtyWind
ows[aWindow.__SSi] || aWindow == activeWindow) {this._collectWindowData(aWindow)
;} else {this._updateWindowFeatures(aWindow);}}),[object Object])@file:///c:/bui
lds/bonecho/mozilla/ff-debug/dist/bin/components/nsSessionStore.js:1693
sss_getCurrentState()@file:///c:/builds/bonecho/mozilla/ff-debug/dist/bin/compon
ents/nsSessionStore.js:1081
sss_saveState()@file:///c:/builds/bonecho/mozilla/ff-debug/dist/bin/components/n
sSessionStore.js:1641
sss_saveStateDelayed(null,0)@file:///c:/builds/bonecho/mozilla/ff-debug/dist/bin
/components/nsSessionStore.js:1623
sss_observe([object XPCWrappedNative_NoHelper],"timer-callback",null)@file:///c:
/builds/bonecho/mozilla/ff-debug/dist/bin/components/nsSessionStore.js:311
@:0

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.docShell has no properties" {file: "chr
ome://global/content/bindings/browser.xml" line: 0}]' when calling method: [nsIO
bserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DE
TAILS)"  location: "<unknown>"  data: yes]
************************************************************
on the subject of this docShell exception, see also bug #317820, bug #289035, bug #291261 and bug #320752
Depends on: 333734
here's what appears to be going on with the that exception.

sss_saveWindowHistory() is called on a timer, and in that function we get all the browsers of the tabbrowser.  for each browser, we are attempting to get the currentURI property, which we get from the webNavigation property, which requires the docShell, which we get from the boxObject.  

In nsXULElement::GetBoxObject(), we first call GetCurrentDoc(), but that will return nsnull because IsInDoc() returns false.  

it looks like this is happening because the document has be taken down, because the tab has closed.

it looks like we are using a cached list of browser (from tabbrowser.browsers in nsSessionStore.js)

looking at the tabbrowser binding, we have code to flush the cache (_browsers), but it is not getting hit in this scenario.

I'll continue to investigate.
this docShell issue is a regression which is new to this patch, due to the new "TabClose" event.

here's what's going on:

when we close the tab, we call the removeTab() method of the tabbrowser binding.  

The first thing we do in removeTab() is invalidate the broswers cache by setting this._browsers to null.

but before we are done executing all the code of removeTab, which includes code to actually remove the closed browser (and destroy it), removeTab() fires the "TabClose" event.  Here's the new code:

+            // We're committed to closing the tab now.  Dispatch a notification.
+            // We dispatch it before any teardown so that event listeners can
+            // inspect the tab that's about to close.
+            var evt = document.createEvent("Events");
+            evt.initEvent("TabClose", true, false);
+            aTab.dispatchEvent(evt);

the sss_onTabClose() method in nsSessionStore.js will call sss_saveWindowHistory() which will access the browsers attribute of the tabbrowser binding.

because our cache is invalidated (_browsers is null), when we access the browsers attribute, we will re-cache all the browsers, including the one we are attempting to remove with the call to removeTab()!

then removeTab() continues on, and will remove and destroy the browser that was closed, but will not touch the _browsers cache.  This leaves our cache in a bad state.

then, the timer fires to save session state and we attempt to iterate over all the browsers.  our _browers cache is valid (it is non-null), but one of them (the one we closed) has been destroyed so we get the exception.

One possible solution would be to invalidate the cache in removeTab() after we actually remove the tab:

            // Remove the tab
            this.mTabContainer.removeChild(oldTab);
+           this._browsers = null; // invalidate cache
(In reply to comment #36)
> One possible solution would be to invalidate the cache in removeTab() after we
> actually remove the tab:
I'd not only vote for this solution, but make sure that _browsers is always invalidated right before children are added to/removed from mTabContainer (to prevent similar situations in the future).
Ah, ok, the patch predates the browsers caching optimization, which is why I didn't see this.  Nice work.  Definitely move the invalidation as you suggested.
simon, good point about the places where we modify mTabContainer.  There are currently three places where the cache invalidation needs to happen because mTabContainer is modifies:  addTab(), removeTab(), and moveTabTo().

> make sure that _browsers is always invalidated right before children

before or after?  Here's my concern with before:

with the session restore happening on a timer, isn't it possible that in between the cache invalidation and the call that modifies mTabContainer, if the session restore code accesses the browsers property, we will rebuild the _browsers cache, and we'll have the same problem.
It shouldn't really matter, since all UI code runs on the same thread anyway. Thinking about it, it might be better though to reset the cache not until just before the modification to mPanelContainer so that tabs and browsers are always in sync, but that extensions listening for DOMNodeInserted/Removed events on the panel container already get updated .browsers (although putting it after the mTabContainer modification should yield pretty much the same result).
new patch coming with three additional fixes:

1)  the _browsers cache invalidation (per the earlier conversation)

2)  if the drop indicator marginLeft (or marginRight, for rtl) is off the screen, we won't draw it.  this fixes the issue that mconnor and I observed where dragging a tab pass the last visible tab could stretch the url bar.

3) if we are dragging over the autorepeat buttons, we scroll the tab strip. rstrong suggested that we draw the drop icon in this scenario "at the edge" when scrolling, and behaves nicely.  (The alternative is to have the drop icon jump around when scrolling, and that was jarring.)
No longer blocks: 341830
Attached patch updated patch (obsolete) — — Splinter Review 

        Attachment #225143 -
        Attachment is obsolete: true

        Attachment #225645 -
        Attachment is obsolete: true

    
    

    
  
testing that patch on the mac, I see one issue:  when I drag over the scroll box arrow, it doesn't scroll (like it does on windows) unless the mouse is moving.  I'm looking into that now.

note to asaf:  I won't check in the rtl changes to pinstripe, I'll leave those for  you in bug Bug #221824.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  
code cleanup:  the default scroll increment is now defined in one place (scrollbox.xml), and can be overridden by a hidden pref.

additional functionality: on dragover of non tab items (like a link, a bookmark, etc) over the arrowscrollbox buttons we scroll the tabstrip.

still working on the timer workaround for the mac issue.

        Attachment #225944 -
        Attachment is obsolete: true
Blocks: 342155
Blocks: 342179

    
    

    
  
Error: _scrollIncrement is not defined
Source File: chrome://global/content/bindings/scrollbox.xml
Line: 46

This is all I get when I try to scroll the tab bar with the latest patch applied.

    
    

    
  
> Error: _scrollIncrement is not defined
> Source File: chrome://global/content/bindings/scrollbox.xml
> Line: 46
>
>This is all I get when I try to scroll the tab bar with the latest patch
>applied.

are you trying on trunk or branch?

    
    

    
  
Trunk, but I'll make sure the patch applied correctly.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        correct patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #223204 -
        Attachment is obsolete: true

        Attachment #226338 -
        Attachment is obsolete: true

    
    

    
  
> Trunk, but I'll make sure the patch applied correctly.

sorry about that.  can you try again with the latest patch?

    
    

    
  
> still working on the timer workaround for the mac issue.

talked it over with mark (mento) and because both windows and linux repeatedly fire onDragOver even when the mouse does not move, he's going to look into fixing the mac widget code to behave the same.

see bug #342229

I'll start pinging mconnor for reviews.
Depends on: 342229
Whiteboard: [SWAG: 1d break out events, see #322898, ?d for the rest] → [SWAG: seeking reviews, but mac will need bug #342229 before it has parity]

    
    

    
  
Yeah, the new patch is working again, I noticed one issue though:

Open enough tabs/adjust window width so that they fill the whole tabbar, but not so many that the scroll buttons appear. Dragging a tab to the right edge still makes address/menus/tabbar slide right (this doesn't happen if the scroll buttons are there).

    
    

    
  
> Open enough tabs/adjust window width so that they fill the whole tabbar, but
> not so many that the scroll buttons appear. Dragging a tab to the right edge
> still makes address/menus/tabbar slide right (this doesn't happen if the scroll
> buttons are there).

timo:  thanks for trying out the patch and for reporting this issue.

investigating this issue now.

    
    

    
  
there's another problem, which is related to the problem time points out, which is that when the arrowscrollboxes are hidden, my patch fails to draw the margin indicator to the left of the first tab when it should.

working on both issues.

    
    

    
  
updating swag to (one more) day.
Whiteboard: [SWAG: seeking reviews, but mac will need bug #342229 before it has parity] → [SWAG: 1d, but mac will need bug #342229 before it has parity]

    
    

    
  


  

        Attachment #226434 -
        Attachment is obsolete: true

        Attachment #226519 -
        Flags: review?(mconnor)

    
    

    
  
I have a couple of nit improvements that can be made, and I'm going to spin those off into other bugs which will depend on this one.

1)  fix this code and make it so the drop indicator, if drawn on the far right (or far left, if rtl) is clipped as it is on the far left (or far right, if rtl)

here's the code I don't especially like:

+                // make sure we don't place the tab drop indicator past the
+                // edge, or the containing box will flex and stretch
+                // the tab drop indicator bar, which will flex the url bar.  
+                // XXX todo
+                // just use first value if you can figure out how to get
+                // the tab drop indicator to crop instead of flex and stretch
+                // the tab drop indicator bar.
+                var maxMarginRight = Math.min(
+                  (minMarginRight + tabStripBoxObject.width), 
+                  (ib.boxObject.x + ib.boxObject.width - ind.boxObject.width));

discusssing it with michael wu, he gave me an idea of how to do what I want.

2)  the drop indicator is really 11 pixels, but we have the box width of the tab-drop-indicator box as 9 pixels.  It's barely noticeable unless you've spent way too look starring at the drop indicator.  I'll attach a zoomed in screen shot.  it should be an easy fix, now that I am using ind.boxObject.width in the code.

I'll spin up two bugs on these issues, but the patch can still be reviewed as is.


    
    

    
  
a good point from mark (who has a patch to make scrolling work for mac!):

he's wondering why we scroll on mouseover (and not requiring a click, see bug #342229).   excluding the "while dragging" scenario, should we really scroll automatically on mouseover of the scrollbox buttons like we do in menus?  or should we behave more like a scrollbar.

perhaps ben/beltzner/mconnor already discussed this at length.  I'll spin up a follow up bug about it.

    
    

    
  
I've logged the spin off bugs I mentioned

bug 342363: tab drop indicator should be a few more pixels to the right in a certain case
bug 342364: should the scrolling tab strip scroll on mouseover, or should click be necessary?
bug 342365: tab drop indicator is cropped by 2 pixels

Blocks: 342364
Whiteboard: [SWAG: 1d, but mac will need bug #342229 before it has parity] → [SWAG: awaiting reviews]

    
    

    
  


  
this new patch includes a fix for bug #342364. details coming...

        Attachment #226519 -
        Attachment is obsolete: true

        Attachment #226893 -
        Flags: review?

        Attachment #226519 -
        Flags: review?(mconnor)

    
    

    
  
with this new patch, we now don't scroll on mouseover since the clicktoscroll="true" attribute is set.

for winstripe, I've also fixed it so the css won't make the arrowbox buttons looked depressed on mouse over.  we may want to have a new style for these buttons on mouseover if clicktoscroll is true, as we do on mouseover for most clickable buttons.

I tested to make sure the menu usage of the arrow scrollbox (in menus, orient="veritcal") still works as intended.

I still need to check out pinstripe and see what css changes are needed.

seeking review from mconnor on the new patch.

    
    

    
  
(In reply to comment #61)
> seeking review from mconnor on the new patch.

You forgot to request review from him. :)

        Attachment #226893 -
        Flags: review? → review?(mconnor)

    
    

    
  
Is bug 342679 something that belongs here? Unless it's actually a dupe of some other bug that I missed...

    
    

    
  
initial review comments from mconnor:

1) mScrollIncrement in scrollbox, I _think_ I barely remember a discussion on m vs _ for the cached values

2) put _scrollBoxObject directly before the property

as in:  rename the mScrollBoxObject field to _scrollBoxObject (since it is cached) and move it right before the scrollBoxObject property (so they are close together in the code) following convention.

3)

>+          if (this.hasAttribute("clicktoscroll") &&  
>+              this.getAttribute("clicktoscroll") == "true") {

4)  browser.tabs.scrollIncrement is the wrong prefname, since its not just for tabs, and it should live in all.js

it's a scrollbox pref, so I'll do toolkit.scrollboxScrollIncrement

new patch coming shortly.

    
    

    
  
mconnor also writes:

5)  no brackets around single lines

    
    

    
  


  

        Attachment #226893 -
        Attachment is obsolete: true

        Attachment #226894 -
        Attachment is obsolete: true

        Attachment #227121 -
        Flags: review?

        Attachment #226893 -
        Flags: review?(mconnor)

        Attachment #227121 -
        Flags: review? → review?(mconnor)

    
    

    
  
this patch as a verbal r=mconnor for the trunk.

1) land on the trunk (excluding the changes asaf didn't want me to land)
2) update the appropriate wiki describing the changes in behaviour, including the test cases I was using.
3) post to the appropriate newsgroups about the change and point to the wiki
4) directly email the well known tab browser extensions authors, point them to the newsgroup post about the changes.
5) directly email marcia and adam (ispiked), in case they have cycles to test.

    
  
Depends on: 342361

    
    

    
  
patch (minus the rtl changes to scrollbox.css) are checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 227121 [details] [diff] [review]
patch updated with feedback from mconnor

this had r=mconnor over aim.

        Attachment #227121 -
        Flags: review?(mconnor) → review+

    
    

    
  
> 1) land on the trunk (excluding the changes asaf didn't want me to land)

per asaf, I landed the winstripe rtl change to scrollbox.css, but not the pinstripe rtl change to scrollbox.css

> 2) update the appropriate wiki describing the changes in behaviour, including
the test cases I was using.

not yet, soon.

> 3) post to the appropriate newsgroups about the change and point to the wiki

http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/5a15c3357b03deab/a7b5b4e2521d3030#a7b5b4e2521d3030

> 4) directly email the well known tab browser extensions authors, point them to
the newsgroup post about the changes.

done.

> 5) directly email marcia and adam (ispiked), in case they have cycles to test.

done.

    
  
Depends on: 342818

    
  
Depends on: 342841

    
  
Depends on: 342845

    
  
Depends on: 342918

    
  
Whiteboard: [SWAG: awaiting reviews] → [SWAG: awaiting reviews] 181b1+

    
    

    
  
Comment on attachment 227121 [details] [diff] [review]
patch updated with feedback from mconnor

>-            // Fire an onselect event for the tabs element.
>+            // Support both the old "select" event and the new, better-named
>+            // "TabSelect" event.
>             var event = document.createEvent('Events');
>             event.initEvent('select', true, true);
>             this.dispatchEvent(event);
>+
>+            event = document.createEvent("Events");
>+            event.initEvent("TabSelect", true, false);
>+            this.dispatchEvent(event);
So what was the justification for this event? Can we expect to have "ButtonClicked", "ColorPicked", "ListItemSelected", "SplitterChanged", "TextboxInput", "TreeSelectionChanged" events now? Or if this was a tabbed-browser-only event, why wasn't it dispatched in tabbrowser.xml?

    
    

    
  
Comment on attachment 227121 [details] [diff] [review]
patch updated with feedback from mconnor


>Index: toolkit/themes/pinstripe/global/globalBindings.xml
>===================================================================

>@@ -59,9 +59,13 @@
>             <xul:stack>
>               <xul:spacer class="tabs-left"/>
>             </xul:stack>
>-            <xul:hbox flex="1" style="min-width: 1px;">
>+            <xul:arrowscrollbox anonid="arrowscrollbox" orient="horizontal" flex="1" style="min-width: 1px;">
>               <children/>
>-              <xul:spacer class="tabs-right" flex="1"/>
>+            </xul:arrowscrollbox>
>+            <xul:hbox class="tabs-closebutton-box" align="center" pack="end" anonid="tabstrip-closebutton">
>+              <xul:toolbarbutton ondblclick="event.stopPropagation();"
>+                           class="close-button tabs-closebutton"
>+                           oncommand="this.parentNode.parentNode.parentNode.parentNode.parentNode.parentNode.removeCurrentTab()"/>
>             </xul:hbox>
>           </xul:hbox>
>           <xul:spacer class="tabs-bottom-spacer"/>

This is eye-opening, themes bindings (including the default theme) aren't supposed to be scriptable.

    
    

    
  
>+              <xul:toolbarbutton ondblclick="event.stopPropagation();"
>+                           class="close-button tabs-closebutton"
>+                           oncommand="this.parentNode.parentNode.parentNode.parentNode.parentNode.parentNode.removeCurrentTab()"/>

I have very little coding experience, especially in XUL, but isn't there a better way of closing the tab than that? It seems to depend on the close tab button being nested 6 levels deeper than the current tab. Wouldn't gBrowser.mTabContianer.removeCurrentTab() (not tested) work?

Also, how would you doubleclick on the close tab button, if the tab is closed before the second click?

Sorry if I'm being a total n00b here.

    
    

    
  
(In reply to comment #73)
> >+              <xul:toolbarbutton ondblclick="event.stopPropagation();"
> >+                           class="close-button tabs-closebutton"
> >+                           oncommand="this.parentNode.parentNode.parentNode.parentNode.parentNode.parentNode.removeCurrentTab()"/>
> 
> Also, how would you doubleclick on the close tab button, if the tab is closed
> before the second click?

notice it says "event.stopPropagation()":
that means the close button doesn't do anything on the second click of a double-click.


    
    

    
  
Comment on attachment 227121 [details] [diff] [review]
patch updated with feedback from mconnor

seeking approval to land this (as well as the fix in bug #342364) to the branch

        Attachment #227121 -
        Flags: approval1.8.1?

    
    

    
  
Could you please revert the minwidth property?  For those who are actually using trunk builds, 140 is far from practical.

    
    

    
  
Comment on attachment 227121 [details] [diff] [review]
patch updated with feedback from mconnor

Approving for branch for 1.8.1 drivers

        Attachment #227121 -
        Flags: approval1.8.1? → approval1.8.1+

    
    

    
  
(In reply to comment #76)
> Could you please revert the minwidth property?  For those who are actually
> using trunk builds, 140 is far from practical.
> 

bug 342890 has been filed for this, in the meantime there's always userChrome.js <http://forums.mozillazine.org/viewtopic.php?t=397735>:

eval('gBrowser.addTab = ' +
      gBrowser.addTab.toString()
                     .replace('.minWidth = 140;', '.minWidth = 30;'));

gBrowser.mTabContainer.childNodes[0].minWidth = 30;

    
    

    
  
fix checked into the 1.8 branch.

note, there are several follow up bugs which I'm working on for FF 2.0 b1 (and beyond).
Keywords: fixed1.8.1

    
  
Depends on: 343096

    
  
Depends on: 343097

    
  
Depends on: 343104

    
  
Depends on: 343019

    
    

    
  
Initial design docs settled on using 'chevron' for tabs in the overflow area. This doesn't seem to be implemented and I couldn't find a bug for that. Should one be filed?

    
    

    
  
The main problem I experienced is that scrolling tabs one by one is pain. Chevron Menus would help, but the fact remains that the srolling feature is kind of useless. It's way too inefficient for heavy users and novices probably won't get it at all (fluid scrolling would be more intuitive to them).

One solution I can imagine is to have multiple tab rows with only one beeing visible (and I don't see this discussed at wiki.mozilla.org). Scrolling would mean to switch rows then. This would break drag&drop, but an option to view all rows would help. In fact reordering would be much easier as with the current implementation.

    
    

    
  
(In reply to comment #81)
> The main problem I experienced is that scrolling tabs one by one is pain.
> Chevron Menus would help, but the fact remains that the srolling feature is
> kind of useless. It's way too inefficient for heavy users and novices probably
> won't get it at all (fluid scrolling would be more intuitive to them).
> 
> One solution I can imagine is to have multiple tab rows with only one beeing
> visible (and I don't see this discussed at wiki.mozilla.org). Scrolling would
> mean to switch rows then. This would break drag&drop, but an option to view all
> rows would help. In fact reordering would be much easier as with the current
> implementation.

I second this solution recommendation.  This is the technique seen in the Windows Taskbar (when grouping is disabled) when you get too many open programs.  It works fairly well, and you get the option to drag/enlarge the taskbar to make it taller so you can see more at once.  (A user could increase the height of the tab bar and then drag/drop tabs between an visible on-screen tab spaces.)  I have also mentioned this idea here:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/5a15c3357b03deab/c448d0536e147048#c448d0536e147048


    
  
Depends on: 343554

    
    

    
  


  

        Attachment #228070 -
        Attachment is obsolete: true

        Attachment #228077 -
        Flags: review?(sspitzer)

        Attachment #228070 -
        Flags: review?(sspitzer)

    
    

    
  
Comment on attachment 228077 [details] [diff] [review]
all-in-one branch patch of latest scrollbox and tabbrowser fixes

r=sspitzer.  

I've tested this patch on my win32 bonecho tree and everything looks good.

this patch contains fixes for bugs #342890 #342906 #343019 #343370 #342841 #343097 which have all landed on the trunk.

        Attachment #228077 -
        Flags: review?(sspitzer) → review+
Depends on: 343626
No longer depends on: 343626

    
    

    
  
1) open many tabs until tab-scroll button appears.
2) focus on the right-end tab. tab-scroll button is disabled.
3) open a new tab, but the button is still disabled.
*** the button should be enabled at this point. ***
4) click the other tab, the button is enabled.

    
    

    
  
> 1) open many tabs until tab-scroll button appears.
> 2) focus on the right-end tab. tab-scroll button is disabled.
> 3) open a new tab, but the button is still disabled.
> *** the button should be enabled at this point. ***
> 4) click the other tab, the button is enabled.

pal-moz, in your step 3 is "open a new tab" opening a tab in the background?  If so, then this issue is covered by bug #343585.

    
    

    
  
(In reply to comment #87)
> in your step 3 is "open a new tab" opening a tab in the background? 
> If so, then this issue is covered by bug #343585.
> 

yes.
thank you for the information.
I'll follow that bug.

    
  

        Attachment #228077 -
        Flags: approval1.8.1+

    
    

    
  
mozilla/browser/app/profile/firefox.js 1.71.2.44
mozilla/modules/libpref/src/init/all.js
mozilla/toolkit/content/xul.css 1.61.2.13
mozilla/toolkit/content/widgets/scrollbox.xml 1.3.52.3
mozilla/toolkit/content/widgets/tabbrowser.xml 1.103.2.50
mozilla/toolkit/themes/pinstripe/global/scrollbox.css 1.2.26.2
mozilla/toolkit/themes/winstripe/global/scrollbox.css 1.3.8.2

    
  
Depends on: 343688

    
    

    
  


  
updateCurrentBrowser() will always be called when switching tabs, so the TabSelect event can be dispatched from there.  This gets rid of browser-specific changes to tabbox.

        Attachment #228461 -
        Flags: review?

        Attachment #228461 -
        Flags: review? → review?(bugs.mano)

    
    

    
  
Comment on attachment 228461 [details] [diff] [review]
Don't mess with the tabbox for a tabbrowser-specific event

Moving this to bug 343096 per Mano's request.

        Attachment #228461 -
        Attachment is obsolete: true

        Attachment #228461 -
        Flags: review?(bugs.mano)
Depends on: 344155

    
  
Blocks: 344171

    
    

    
  
I find the scrolling tab bar in Ffox 2 Beta 1 to be really annoying and personally don't even know why they added this feature in the first place, the tabs were perfectly fine as they were in earlier releases. Even in 1600x1200 when I open around 10 tabs you already see those annoying scroll buttons.

My philosophy with new software features that could possibly annoy existing users that have accustomed to older version, is the ability to still turn the feature off with ease. I know this can be done by going through about:config, but it should really be in the settings dialog somewhere, so it makes it easier for me to turn it off after a new Ffox installation, I do a lot of installs and would like to quickly turn this annoying feature off ASAP on every install I do if possible.

I believe this new feature is actually a step back in Firefox usability and hope there will be some easy way for it to be turned off in the final release, because I personally don't want this feature.

    
    

    
  
Neither solution seems to be acceptable to me.  I usually use 1024x768 on my laptop, and with too many tabs open on the old (FF 1.0/1.5) behaviour the latter tabs would just not be clickable on the screen until some tabs were closed.  Even worse, you couldn't close tabs without right-clicking on them and selecting "Close" as the close-tab button was underneath the tabs, and it looked extremely unprofessional.

In the new version, I thought Firefox was broken the first few times I've opened many tabs.  The tabs didn't get really small, nor was there immediate feedback when those tiny, tiny buttons (that look like non-buttons) appeared to scroll through the tabs.  It's also not very obvious when you're at the far left or far right of the tab list, it's extremely disorienting.

    
    

    
  
Currently with the new patch, when hovering above the tab strip, you can
scroll the tab strip with the Mouse Wheel just the same way you can do
it with the new chevrons.

How about changing this to really scroll the Tabs with the mouse wheel
and not only the tab strip -so with the wheel you can go quickly through
all the Tabs (with the tab strip following scrolling when you reach the
last visible Tab on the left or right).
That way, you can still use both ways (mouse wheel or chevrons) to get
to an Tab that is currently not visible on tab strip, and you also gain
the new functionality for easy and very quick cycling between Tabs.

Currently only (overloaded) TabMixPlus Extension is able to activate
this and once you are used to it, you don't want to miss it.
It's simply perfect for quick inspection or comparison of different Tabs
without the need to move the mouse or do some clicks, simply by turning
the mouse-wheel (but naturally only if mouse hovers over tab strip).
It feels like switching between Virtual Desktops under Linux.

And if not enabled by default, perhaps this could be some new hidden
pref.?

    
    

    
  
(In reply to comment #94)
> How about changing this to really scroll the Tabs with the mouse wheel
> and not only the tab strip -so with the wheel you can go quickly through
> all the Tabs (with the tab strip following scrolling when you reach the
> last visible Tab on the left or right).
> That way, you can still use both ways (mouse wheel or chevrons) to get
> to an Tab that is currently not visible on tab strip, and you also gain
> the new functionality for easy and very quick cycling between Tabs.

I have long been a fan of switching tabs as you describe, and even built this into my extension (Too Many Tabs). However it's not quite so clear cut here: currently wheel-scrolling will scroll by 3 tabs at a time, which lets you rapidly browse through a lot of tabs. While the way you describe has the advantage of selecting tabs too, it also only scrolls by 1 tab at a time.

    
    

    
  
One extension's functionality I don't see mentioned in this bug is that of HashColouredTabs (http://hashcolouredtabs.mozdev.org/) which will color similar tabs (from same site) in a similar manner. This is quite useful for me when looking at several websites if I need to find another link from that site quickly rather than sort through 20 tabs, I can look through maybe 4 or 5.



    
    

    
  
I think it'd be best if a new bug was opened to specifically to discuss improving the handling of many tabs, and the discussion moved there (or to Mozillazine forums). This bug is actually resolved, and a solution is in, plus this bug was a tracker for many issues and not just that particular one, so it's probably spamming a lot of people who aren't interested.

    
  
Depends on: 345028
Blocks: 342900
No longer depends on: 342900

    
  
Summary: Improve tabbed browsing → Improve tabbed browsing (tab strip overflow, in particular)

    
  
Depends on: 346441
Depends on: 345280

    
  
Depends on: 347238

    
  
Whiteboard: [SWAG: awaiting reviews] 181b1+ → 181b1+

    
    

    
  
I just tried out 2.0 and it is tough to tell the difference between a front and a background tab, so therefore some wrong tabs were inadvertently closed using the center click method. There are two answers: 1) is to make the front tab brighter so it is easier to discern 2) is to add the X to close at the end again so you don't have to hunt and peck in order to close a tab what with the moving sized tabs now and the X can fly from left to right. Open 10-15 tabs and then try to close 8 of the quickly or close every other one. Not easy, and I've done it several time already since late yesterday in testing out 2.0. 

This is not ready for prime time yet, sticking with 1.5.0.6 for now.

    
    

    
  
(In reply to comment #98)
> I just tried out 2.0 and it is tough to tell the difference between a front and
> a background tab

Sounds like bug 349190 to me.

    
  
Depends on: 350371

    
  
Depends on: 1251987

    
  
No longer depends on: 1251987

          You need to log in
          before you can comment on or make changes to this bug.