bug 318168 adds tab overflow and bumps tab minWidth to 140, which would be about 5 tabs at 800x600. I think there's probably a good amount of users who would be willing to go with smaller tabs vs overflow. Needs some error checking, but maybe something like this: --- ./content/global/bindings/tabbrowser.xml.orig 2006-06-26 15:35:08.000000000 -0500 +++ ./content/global/bindings/tabbrowser.xml 2006-06-27 13:27:31.000000000 -0500 @@ -1095,7 +1095,7 @@ t.setAttribute("crop", "end"); t.maxWidth = 250; - t.minWidth = 140; + t.minWidth = this.mTabContainer.mTabMinWidth; t.width = 0; t.setAttribute("flex", "100"); t.setAttribute("validate", "never"); @@ -2389,12 +2389,15 @@ Components.classes['@mozilla.org/preferences-service;1']. getService(Components.interfaces.nsIPrefBranch2); try { + this.mTabMinWidth = pb2.getIntPref("browser.tabs.tabMinWidth"); this.mTabClipWidth = pb2.getIntPref("browser.tabs.tabClipWidth"); this.mCloseButtons = pb2.getIntPref("browser.tabs.closeButtons"); } catch (e) { } + this.childNodes[0].minWidth = this.mTabMinWidth; + this._updateDisableBackgroundClose(); this.adjustTabstrip(false); @@ -2464,6 +2467,7 @@ } }); </field> + <field name="mTabMinWidth">140</field> <field name="mTabClipWidth">130</field> <field name="mCloseButtons">1</field>

logan, thanks for starting the patch. I'll take it and run with it for bug #342385 *** This bug has been marked as a duplicate of 342385 ***

Is this really a dupe of 342385 or are you just going to do both in the same bug? .tabClipWidth is (or was originally) used to determine when to show/hide the close button on tabs. a bit confused...

re-opening, I thanks for clarifying.

mwheel scrolling seems to get a bit messed up with a lower tab.minWidth. I'm stopped well short of the end with a value of 100.

Can't seem to reproduce the wheeling issue I saw earlier, so here's patch v1 with some error checking. Anyone have concerns with the min/max checks? You can't see any text with a minWidth of 30 and minWidth shouldn't exceed maxWidth (250, not configurable). I know mconnor said to target this for b2, but it's quick, easy, and would be nice to get in along with 318168, should it make it into branch for b1. That being the case, it'd probably have to go in on trunk pretty quick... vlad seemed to not like the overflow bit a whole lot, so maybe I'll have some additional support on this one. :)

logan, I'll try out your patch, review it, and help drive it into the trunk and branch. thanks for jumping in here and helping!

> Anyone have concerns with the min/max checks? + if (this.mTabMinWidth < 30 || this.mTabMinWidth > 250) + this.mTabMinWidth = 140; I have concerns, as I don't we want to hard code this assumption. but, regardless, your patch wouldn't work (as is) because use "<" in the constructor you'd need this, right? <constructor><![CDATA[ ... ]]></constructor> mconnor tells me that he and beltzner thing that 125 px is a better default than 140, and that seems reasonable to me. additionally, there is a default minwidth="140" in tabbrowser.xml that we should adjust as well. let me attached my revised patch based on logan's initial patch. let me attach a

I decided not to include the min / max check, and since I'm not using the < > characters, I don't need the CDATA. as logan points out, you can shoot yourself in the foot here with a "bad" pref, but that's true of many hidden prefs, so I'd rather not make the assumption about what's the (current) min or max.

moving back to FF 20 b1.

(In reply to comment #7) > but, regardless, your patch wouldn't work (as is) because use "<" in the > constructor you'd need this, right? Sorry, I added that check w/out testing. I didn't even notice the CDATA on other fields and constructors... :S

> Sorry, I added that check w/out testing. I didn't even notice the CDATA on > other fields and constructors... no worries. Thanks again for doing the initial patch!

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default seeking asaf's review, too.

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default >Index: browser/app/profile/firefox.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v >retrieving revision 1.120 >diff -u -w -r1.120 firefox.js >--- browser/app/profile/firefox.js 20 Jun 2006 02:58:31 -0000 1.120 >+++ browser/app/profile/firefox.js 29 Jun 2006 06:36:31 -0000 >@@ -247,6 +247,7 @@ > pref("browser.tabs.opentabfor.middleclick", true); > pref("browser.tabs.loadDivertedInBackground", false); > pref("browser.tabs.loadBookmarksInBackground", false); >+pref("browser.tabs.tabMinWidth", 125); > pref("browser.tabs.tabClipWidth", 140); > pref("browser.tabs.disableBackgroundClose", false); > >Index: toolkit/content/widgets/tabbrowser.xml >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v >retrieving revision 1.158 >diff -u -w -r1.158 tabbrowser.xml >--- toolkit/content/widgets/tabbrowser.xml 29 Jun 2006 01:26:28 -0000 1.158 >+++ toolkit/content/widgets/tabbrowser.xml 29 Jun 2006 06:36:31 -0000 >@@ -106,7 +106,7 @@ > <xul:tab selected="true" validate="never" > onerror="this.parentNode.parentNode.parentNode.parentNode.addToMissedIconCache(this.getAttribute('image')); > this.removeAttribute('image');" >- maxwidth="250" width="0" minwidth="140" flex="100" >+ maxwidth="250" width="0" minwidth="125" flex="100" > class="tabbrowser-tab" label="&untitledTab;" crop="end"/> > </xul:tabs> > </xul:hbox> >@@ -1099,7 +1099,7 @@ > > t.setAttribute("crop", "end"); > t.maxWidth = 250; >- t.minWidth = 140; >+ t.minWidth = this.mTabContainer.mTabMinWidth; > t.width = 0; > t.setAttribute("flex", "100"); > t.setAttribute("validate", "never"); >@@ -2397,12 +2397,15 @@ > Components.classes['@mozilla.org/preferences-service;1']. > getService(Components.interfaces.nsIPrefBranch2); > try { >+ this.mTabMinWidth = pb2.getIntPref("browser.tabs.tabMinWidth"); > this.mTabClipWidth = pb2.getIntPref("browser.tabs.tabClipWidth"); > this.mCloseButtons = pb2.getIntPref("browser.tabs.closeButtons"); > } > catch (e) { > } > >+ this.childNodes[0].minWidth = this.mTabMinWidth; >+ hrm, I read this as you try to set the minWidth of the first tab, but you're actually changing the minWidth of the entire scrollbox http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#2385 I suppose you want something like this.mTabstrip.childNodes[0].minWidth

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default or better yet, this.mTabstrip.firstChild diff -up8 please (w optional for large patches)

WhatMconnorSaid! :)

this is code is in the constructor, and when it gets called, we don't have any tabs yet, so this.mTabstrip.childNodes[0] is null (and this.mTabstrip.firstChild is undefined.) I'm going to double check, but I don't think we need this line at all. + this.childNodes[0].minWidth = this.mTabMinWidth;

is mTabstrip undefined or mTabstrip.firstChild (note the first tab is a static element). You do need to set the minWidth of the first tab somewhere...

> is mTabstrip undefined or mTabstrip.firstChild (note the first tab is a static > element). You do need to set the minWidth of the first tab somewhere... in the constructor, this.mTabstrip is not undefined (it is a XULElement), but this.mTabstrip.firstChild is null. this is the "tabs" element and this.childNodes[0] is the static "tab" element > You do need to set the minWidth of the first tab somewhere... Oops, I see that now.

mano / mconnor, here's some additional info. in the constructor for the tabbrowser-tabs binding, this.mTabstrip is the arrowscrollbox, see http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/tabbrowser.xml#2435 the tabbrowser-tabs ("this") has the tabs as the childNodes, which is why this.childNodes[0] does the right thing. the arrowscrollbox does not have any childNodes, which is why this.mTabstrip.childNodes[0] is null (and this.mTabstrip.firstChild is undefined.) from what I see in the dom inspector (looking at things as JS Objects) the current patch (based on logan's original patch) is correct, but I may be missing something.

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default re-seeking review from asaf, based on recent findings.

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default > pref("browser.tabs.tabClipWidth", 140); ... > <field name="mTabClipWidth">130</field> Why are those different values?

(In reply to comment #21) > > pref("browser.tabs.tabClipWidth", 140); > > <field name="mTabClipWidth">130</field> > Why are those different values? I noted the different values in bug 342385, but I'm not sure what's wrong with tabClipWidth to begin with. Perhaps if there is no issue, that could be updated when this is checked in. (In reply to comment #20) > re-seeking review from asaf, based on recent findings. tested on branch, works perfectly

the 130 vs 140 issue that reed and logan point out is valid, and I'll attach a new patch to address it.

not a very recent tree, but it does include the main tabbedbrowsing fix.

(In reply to comment #8) > I decided not to include the min / max check, and since I'm not using the < > > characters, I don't need the CDATA. Just a suggestion, but why not add these so when someone else comes along and does add <s or >s they don't run into trouble?

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default r=mano with nits fixed (see bug 343019).

Comment on attachment 227511 [details] [diff] [review] patch, use 125 as the default (but obsoleting this since actual work is on the other bug).

> Just a suggestion, but why not add these so when someone else comes along and > does add <s or >s they don't run into trouble? good idea. I'll add the CDATA to my patch in bug #343019 when I work on all the corrections and suggestions from asaf's review.

> I'll add the CDATA to my patch in bug #343019 when I work on all > the corrections and suggestions from asaf's review. done and checked into the trunk. the fix for this bug was checked in along with the fix for bug #343019 . now seeking a= for the branch.

Fixed on 1.8 branch for beta 1 (branch patch is on bug 318168).

latest 20060707 Beta 1 and Minefield. set to 60, nothing changes.

> set to 60, nothing changes. palmoz, when you changed this pref, you would need to open a new browser window (or exit and restart firefox) to see it take effect. there is no observer in the tab browser binding (tabbrowser.xml) to catch when this pref changes. I'll attach a screen shot that shows the default (120 px) vs 60 px.