All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 2 beta1

Status

()

RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: logan+mozilla-bmo, Assigned: moco)

Tracking

({fixed1.8.1})

Trunk
Firefox 2 beta1
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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 ***
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → DUPLICATE
(Reporter)

Comment 2

13 years ago
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: make tab minWidth configurable for tab overflow (post bug 318168) → make tab minWidth configurable for tab overflow (post bug 318168) using get the browser.tabs.tabMinWidth
Assignee: nobody → sspitzer
Status: REOPENED → NEW
Target Milestone: Firefox 2 beta2 → Firefox 2 beta1
(Reporter)

Comment 4

13 years ago
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.
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
(Reporter)

Comment 5

13 years ago
Created attachment 227349 [details] [diff] [review]
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. :)
Attachment #227349 - Flags: review?(mconnor)
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!
Status: NEW → ASSIGNED
Flags: blocking-firefox2?
> 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
Attachment #227349 - Attachment is obsolete: true
Attachment #227349 - Flags: review?(mconnor)
Created attachment 227511 [details] [diff] [review]
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.
Attachment #227511 - Flags: review?(mconnor)
moving back to FF 20 b1.
Summary: make tab minWidth configurable for tab overflow (post bug 318168) using get the browser.tabs.tabMinWidth → make tab minWidth 125px and heed the browser.tabs.tabMinWidth hidden pref
Target Milestone: Firefox 2 beta2 → Firefox 2 beta1
(Reporter)

Comment 10

13 years ago
(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
Flags: blocking-firefox2? → blocking-firefox2+
> 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.
Attachment #227511 - Flags: superreview?(mconnor)
Attachment #227511 - Flags: review?(mconnor)
Attachment #227511 - Flags: review?(bugs.mano)
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
Attachment #227511 - Flags: review?(bugs.mano) → review-
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)
Attachment #227511 - Flags: superreview?(mconnor)
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.
Attachment #227511 - Flags: review- → review?(bugs.mano)
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?
(Reporter)

Comment 22

13 years ago
(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.
Created attachment 227802 [details]
What i see in DOMi

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

Comment 25

13 years ago
(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).
Attachment #227511 - Flags: review?(bugs.mano) → review+
Comment on attachment 227511 [details] [diff] [review]
patch, use 125 as the default

(but obsoleting this since  actual work is on the other bug).
Attachment #227511 - Attachment is obsolete: true
> 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Depends on: 343019
Resolution: --- → FIXED
Whiteboard: [SWAG: fixed on the trunk, seeking a= for branch]
Whiteboard: [SWAG: fixed on the trunk, seeking a= for branch] → [SWAG: fixed on the trunk, part of massive branch patch in bug #318168]
Fixed on 1.8 branch for beta 1 (branch patch is on bug 318168).
Keywords: fixed1.8.1
Whiteboard: [SWAG: fixed on the trunk, part of massive branch patch in bug #318168]

Comment 31

12 years ago
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.
You need to log in before you can comment on or make changes to this bug.