342890 - make tab minWidth 125px and heed the browser.tabs.tabMinWidth hidden pref

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[logan]

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>

Comment 1

[(not reading, please use [e-mail] instead)]

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 ***

Comment 2

[logan]

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...

Comment 3

[(not reading, please use [e-mail] instead)]

re-opening, I thanks for clarifying.

Comment 4

[logan]

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.

Comment 5

[logan]

Attachment: patch v1

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. :)

Comment 6

[(not reading, please use [e-mail] instead)]

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!

Comment 7

[(not reading, please use [e-mail] instead)]

> 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

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: patch, use 125 as the default

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.

Comment 9

[(not reading, please use [e-mail] instead)]

moving back to FF 20 b1.

Comment 10

[logan]

(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

Comment 11

[(not reading, please use [e-mail] instead)]

> 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 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 227511 [details] [diff] [review]
patch, use 125 as the default

seeking asaf's review, too.

Comment 13

[Asaf Romano (gone)]

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 14

[Mike Connor [:mconnor]]

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)

Comment 15

[Asaf Romano (gone)]

WhatMconnorSaid! :)

Comment 16

[(not reading, please use [e-mail] instead)]

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;

Comment 17

[Asaf Romano (gone)]

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...

Comment 18

[(not reading, please use [e-mail] instead)]

> 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.

Comment 19

[(not reading, please use [e-mail] instead)]

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 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 227511 [details] [diff] [review]
patch, use 125 as the default

re-seeking review from asaf, based on recent findings.

Comment 21

[Reed Loden [:reed]]

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?

Comment 22

[logan]

(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

Comment 23

[(not reading, please use [e-mail] instead)]

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

Comment 24

[Asaf Romano (gone)]

Attachment: What i see in DOMi

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

Comment 25

[Adam Guthrie]

(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 26

[Asaf Romano (gone)]

Comment on attachment 227511 [details] [diff] [review]
patch, use 125 as the default

r=mano with nits fixed (see bug 343019).

Comment 27

[Asaf Romano (gone)]

Comment on attachment 227511 [details] [diff] [review]
patch, use 125 as the default

(but obsoleting this since  actual work is on the other bug).

Comment 28

[(not reading, please use [e-mail] instead)]

> 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.

Comment 29

[(not reading, please use [e-mail] instead)]

> 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.

Comment 30

[Asaf Romano (gone)]

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

Comment 31

[pal-moz]

latest 20060707 Beta 1 and Minefield.

set to 60, nothing changes.

Comment 32

[(not reading, please use [e-mail] instead)]

> 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.

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: 120 px vs 60 px