343019 - Tab scrolling does not handle window resize and underflow well

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Brian Polidoro]

It's possible to end up with tabs that are scrolled off the tabbar and not have any scroll buttons.

1. Start with a fairly narrow browser window - say half the width of the screen.
2. Open several tabs so that some scrolling is possible.
3. Scroll the tabs all the way to the right if not already there.
4. Increase the width of the browser window.  Maximizing would probably be best.


Actual Results:
Notice how there is room to right of the tabs.  But some tabs are still scrolled out of view.  And if you are able to increase the width of the browser window enough so those tabs would fit on the tabber without the need of scrolling then the scrolling buttons would be removed and you would have no way to scroll those tabs into view.

So some tab scrolling is going to be needed when the window is resized.

Comment 1

[Brian Polidoro]

Attachment: Screenshot of the problem

Comment 2

[Brian Polidoro]

Step three may be ambiguous so it should say:
3.  Press the right scroll button until no more scrolling is possible.

Comment 3

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

brian, thanks for reporting this.  I see it on windows xp as well (I have not tried linux or mac yet).

we are calling adjustTabstrip() on resize, so I'll look into why it isn't working.

Comment 4

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

here's what's going on:

in tabbrowser, the _handleUnderflow handler for the "underflow" event is not getting called because of the "underflow" handler in scrollbox which does event.stopPropagation().

The _handleUnderflow() in tabbrowser will scroll the tabs which fixes the weirdness.

I see two possible solutions:

1) don't call event.stopPropagation() in scrollbox underflow handler, so that tabbrowser handler will get called.
2) remove the handler from tabbrowser and do the scrolling in scrollbox.

I think the second option is better.  asaf / mconnor, what do you guys think?  (I'll attach a patch for consideration).

Comment 5

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

Attachment: patch

Comment 6

[Asaf Romano (gone)]

Comment on attachment 227635 [details] [diff] [review]
patch

>Index: content/widgets/scrollbox.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/scrollbox.xml,v
>retrieving revision 1.6
>diff -u -p -8 -r1.6 scrollbox.xml
>--- content/widgets/scrollbox.xml	26 Jun 2006 22:35:07 -0000	1.6
>+++ content/widgets/scrollbox.xml	30 Jun 2006 00:43:07 -0000
>@@ -123,16 +123,17 @@
> 
>     <handlers>
>       <handler event="DOMMouseScroll" action="this.scrollByIndex(event.detail); event.stopPropagation();"/>
> 
>       <handler event="underflow"><![CDATA[
>         var kids = document.getAnonymousNodes(this);
>         kids[0].collapsed = true;
>         kids[2].collapsed = true;
>+        this.scrollBoxObject.scrollBy(-2400, 0);

So, what's this "2400" value? How about doing |ensureElementIsVisible| on the first element in the scrollbox (and probably also on the last one in case someone managed to have a right-alignd but not RTL scrollbox)?

Comment 7

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

> How about doing |ensureElementIsVisible| on the
> first element in the scrollbox (and probably also on the last one in case
> someone managed to have a right-alignd but not RTL scrollbox)?

That sounds good to me.  I'll have to check that there is at least one element, too.

I'll work up a new patch, test it, and then attach for review.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 227635 [details] [diff] [review]
patch

Thanks.

Comment 9

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

Attachment: patch following asaf's suggestion

Comment 10

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

if you make your browser window smaller and it caused the overflow, your selected tab may now no longer be visible.

so in addition to the attached fix for underflow, I need to ensure the selected tab is visible on overflow.

I'll create a new patch that includes that.

Comment 11

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

I take that back.  this isn't an overflow issue, it's a resize issue.

on resize, in tabbrowser.xml, I need to ensure the selected item is visible.

the reason it's not a overflow issue is that we might be in an overflowed state and resizing the window should keep the selected tab "visible" in the scrollbox.

I've got a patch and I'll attach it.

Comment 12

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

Attachment: updated patch, include the fix for bug #342890

Comment 13

[Asaf Romano (gone)]

Comment on attachment 227784 [details] [diff] [review]
updated patch, include the fix for bug #342890

Index: browser/app/profile/firefox.js
===================================================================

 // Tab browser preferences.
 pref("browser.tabs.loadInBackground", true);
 pref("browser.tabs.loadFolderAndReplace", true);
 pref("browser.tabs.opentabfor.middleclick", true);
 pref("browser.tabs.loadDivertedInBackground", false);
 pref("browser.tabs.loadBookmarksInBackground", false);
-pref("browser.tabs.tabClipWidth", 140);
+pref("browser.tabs.tabMinWidth", 125);
+pref("browser.tabs.tabClipWidth", 115);

Is the tabClipWidth change supposed to be here (I read bug 342890 as the tabMinWidth change is already ui-r'ed)? If so get beltzner or mconnor to ui-r this.

Index: toolkit/content/widgets/scrollbox.xml
===================================================================

     <handlers>
       <handler event="DOMMouseScroll" action="this.scrollByIndex(event.detail); event.stopPropagation();"/>
 
       <handler event="underflow"><![CDATA[
         var kids = document.getAnonymousNodes(this);
         kids[0].collapsed = true;
         kids[2].collapsed = true;
+        var childNodes = this.parentNode.childNodes;
+        if (childNodes && childNodes.length) {
+          this.ensureElementIsVisible(childNodes[0]);
+          if (childNodes.length > 1)
+            this.ensureElementIsVisible(childNodes[childNodes.length-1]);
+        }

You're getting the wrong childNodes here (this.parentNode.childNodex contains this ;)), what you really need is the _anonymous_ nodes of kids[1]. In the tabbrowser case this actually gives you one node, which is the <box> element which contains the <xul:tab> elements. This is fine since the scrollbox treats it as a one of its child elements (but do keep in mind this isn't the first tab).

So this should be |var childNodes = document.getAnonymousNodes(kids[2])

(do keep the last-element part for other cases).

Index: toolkit/content/widgets/tabbrowser.xml
===================================================================


@@ -2392,31 +2392,35 @@
       </xul:hbox>
     </content>
     <implementation>
       <constructor>
         var pb2 =
             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;

nit: use this.firstChild (Sorry for my earlier false alarm here btw).

...

         var self = this;
         function onResize() {
           self.adjustTabstrip(false);
+          self.mTabstrip.scrollBoxObject
+                        .ensureElementIsVisible(self.selectedItem);

One issue here, STR:
 1) Have seven tab open
 2) resize the window and scroll the tabbar so you only see the last three tabs.
 3) enlarge the window so it /can/ show at least one more tab.

I think we should always show as many tabs as possible after the window is resized, this change only takes care of the selected tab though. Please file a bug on this issue.


r=mano with those addressed.

Comment 14

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

asaf, thanks for the review and the help on this one.

1)  address your comments and create a new patch
2)  add the CDATA to the constructor, per adam's comment in bug #342890
3)  seek ui review on the change the the default tab clip width from beltzner or mconnor
4)  log the spin off bug about the additional issue that you pointed out.

I'll attach a new patch later today.

Comment 15

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

Attachment: patch, per mano's suggestions.

Comment 16

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

fix checked in on the trunk.  I'll go seek a= for the 1.8 branch.

> 1)  address your comments and create a new patch

done, see below.

> 2)  add the CDATA to the constructor, per adam's comment in bug #342890

done, see latest patch.

> 3)  seek ui review on the change the the default tab clip width from beltzner
or mconnor

done, see https://bugzilla.mozilla.org/show_bug.cgi?id=342385#c8

> 4)  log the spin off bug about the additional issue that you pointed out.

will go do this now.

responses to mano's review:

> Is the tabClipWidth change supposed to be here (I read bug 342890 as the
> tabMinWidth change is already ui-r'ed)? If so get beltzner or mconnor to ui-r
> this.

yes, I got beltzner to ui-r that change.

> +        var childNodes = this.parentNode.childNodes;
> You're getting the wrong childNodes here (this.parentNode.childNodex contains
> this ;)), what you really need is the _anonymous_ nodes of kids[1]. In the
> tabbrowser case this actually gives you one node, which is the <box> element
> which contains the <xul:tab> elements. This is fine since the scrollbox treats
> it as a one of its child elements (but do keep in mind this isn't the first
> tab).

Thanks for the info and the suggestion.  here's what I ended up doing, based on your info and suggestion:

var scrollBoxInnerBox = document.getAnonymousNodes(kids[1])[0]
                                .boxObject;
if (scrollBoxInnerBox.firstChild)
  this.ensureElementIsVisible(scrollBoxInnerBox.firstChild);
if (scrollBoxInnerBox.lastChild)
  this.ensureElementIsVisible(scrollBoxInnerBox.lastChild);

I've tested it, and it works as expected.

Note, I did test "this.parentNode.childNodes", and at least for tabbrowser, the childNodes were the tabs (and did not contain "this").  But I see now that getting the anonymous nodes of kids[1] is the right way.  Thanks, mano.

> nit: use this.firstChild (Sorry for my earlier false alarm here btw).

done.

no worries about the false alarm!  

my apologies for all the iterations over these patches.  I really appreciate all the prompt reviews, suggestions, comments, etc.

Comment 17

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

>> 4)  log the spin off bug about the additional issue that you pointed out.
> will go do this now.

asaf (and brian), see bug #343370.

my change for this bug does fix some resize (and underflow) scenarios, but I really need to fix bug #343370 to nail them all.

Comment 18

[Asaf Romano (gone)]

Thanks Seth, one issue though:

why
var scrollBoxInnerBox = document.getAnonymousNodes(kids[1])[0]
                                .boxObject;

This will not be ok when you've a scrollbox with "simple" elements (e.g. <hbox>es) since you're looking for the first and last child of their first element (also the last-child thing has to be something in the end of the scrollbox, not in the end of the first child element).

This might look better for the tabbed browsing case, but as I said - passing the tabbrowser <box> here is good enough.

nit: no need to check if there's a lastChild when you've a firstChild.

Comment 19

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

Attachment: supplimental patch

Comment 20

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

> why
> var scrollBoxInnerBox = document.getAnonymousNodes(kids[1])[0]
>                                .boxObject;

The reason I did that was I could not figure out a way (either by using childNodes or by getting the anonymous attributes) to get at the tabs of the tabbrowser <box>, except by going through the boxObject.

I wanted the tabs so that I could ensure both first and last tab were visible.  But from your comments, it sounds like I don't need to do that, I just need to make sure the tabbrowser <box> is visible?

> This might look better for the tabbed browsing case, but as I said - passing
> the tabbrowser <box> here is good enough.

I've fixed my patch to do just this, as you originally suggested in comment #13.  I've tested, and it seems to work for the underflow test case.

> nit: no need to check if there's a lastChild when you've a firstChild.

Oops, thanks for point that out.  In my supplimental patch this isn't an issue anymore.

asaf, could you review my supplimental patch?

Comment 21

[Asaf Romano (gone)]

Comment on attachment 227925 [details] [diff] [review]
supplimental patch

Thanks, this is much better. r=mano.

And yes, that box oontains both the first and the last tab, so ensuring it's visible is enough for tabbrowser. The reason we should also ensuring the visibility of the last child too is the case in which  the [logical] scrolled elements are direct children of the scrollbox).

Comment 22

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

thanks for additional information and the prompt review.

I've checked in the supplimental patch to the trunk.

I'll seek approval for the patches in this bug on the 1.8 branch.

Comment 23

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

Comment on attachment 227846 [details] [diff] [review]
patch, per mano's suggestions.

carrying over asaf's review.  seeking a= for the branch.  (note, I would need to check in this patch and the supplimental patch)

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 227846 [details] [diff] [review]
patch, per mano's suggestions.

a=mconnor on behalf of drivers, though I think you and mano are bitrotting each other furiously here

Comment 25

[Mike Connor [:mconnor]]

Comment on attachment 227925 [details] [diff] [review]
supplimental patch

a=mconnor on behalf of drivers

Comment 26

[Asaf Romano (gone)]

Comment on attachment 227925 [details] [diff] [review]
supplimental patch

Actually, reading the scrollbox code a bit more, this box is always there, with no siblings. We should just ensure its (and only its) visibility.

Comment 27

[Asaf Romano (gone)]

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