Last Comment Bug 749658 - Tabs with long file names triggering tab-overflow with 8 tabs
: Tabs with long file names triggering tab-overflow with 8 tabs
Status: RESOLVED FIXED
: dogfood, regression
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: mozilla15
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
: 750131 750481 (view as bug list)
Depends on:
Blocks: 746421
  Show dependency treegraph
 
Reported: 2012-04-27 09:18 PDT by Jim Jeffery not reading bug-mail 1/2/11
Modified: 2012-08-14 05:19 PDT (History)
24 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
verified


Attachments
patch (1.11 KB, patch)
2012-04-29 11:14 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch, distinguish scrollable from visual bounds for nsTextBoxFrame (2.11 KB, patch)
2012-04-29 12:20 PDT, Jonathan Kew (:jfkthame)
roc: review+
ryanvm: checkin+
Details | Diff | Review
test for unwanted tab-strip overflow (2.24 KB, patch)
2012-04-30 05:58 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Review
reftest for unwanted scroll-overflow due to cropped textbox label (1.91 KB, patch)
2012-04-30 06:59 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Review

Description Jim Jeffery not reading bug-mail 1/2/11 2012-04-27 09:18:55 PDT
STR:
1. Open the following URL's:
http://www.msnbc.msn.com/
http://www.cnn.com/?refresh=1
http://forums.mozillazine.org/viewforum.php?f=23
http://www.neowin.net/
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/
https://tbpl.mozilla.org/
about:memory

2. Open http://www.bing.com/ NOTE: that the Tab-strip overflow is not displayed
3. Close Bing 
4. Open: http://spaceweather.com/ NOTE: now the Tab-strip overflow IS displayed
5. Click the scroll-overflow arrow and notice a blank glass area is displayed where there is normally a tab if its a true tab-overflow.

Broken in cset:
https://hg.mozilla.org/mozilla-central/rev/d871849ac3a3
Good in cset:
https://hg.mozilla.org/mozilla-central/rev/0aa393eef856

Something in the m-i->m-c merge in broken cset is causing this.

Regression , regressession window wanted.
Comment 1 Jim Jeffery not reading bug-mail 1/2/11 2012-04-27 09:26:14 PDT
Tested with m-c hourly builds, on Win7 x64 with Medium 125% screen setting 
1680x1050
Comment 2 Jonathan Kew (:jfkthame) 2012-04-28 10:14:45 PDT
Possibly triggered by bug 746421 - any chance you could confirm this using mozilla-inbound or local builds?  I wonder if the tab-strip is looking at visual overflow rects somewhere (rather than frame dimensions) to determine whether things are overflowing.
Comment 3 neil@parkwaycc.co.uk 2012-04-28 12:52:50 PDT
(In reply to Jonathan Kew from comment #2)
> Possibly triggered by bug 746421
Confirmed from local backout of changes to nsTextBoxFrame.cpp
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-04-29 05:42:05 PDT
I'm assuming that this is also the cause of my tab bar wigging out where it's rapidly switching back and forth between showing arrows and not, especially in GMail.
Comment 5 Gordon P. Hemsley [:GPHemsley] 2012-04-29 07:51:18 PDT
Could this also be responsible for the behavior I'm seeing where the tab bar doesn't snap to the right when I close a tab? (Instead, it leaves a huge space until I manually scroll the remaining tabs right.)
Comment 6 Jonathan Kew (:jfkthame) 2012-04-29 09:20:04 PDT
Quite possibly.

It seems like the (mis)behavior is dependent on the length of the title of the rightmost (visible) tab... the tab bar is behaving as though it's using the full width of that tab's title to compute the amount of scroll that may be needed, even though the title is actually truncated to fit within the tab. So if I have a rightmost tab with a title that's only slightly longer than can be displayed, the tab strip scrolls to leave a small blank area at the right; but if my rightmost tab has a really long title, the amount of "overscroll" is correspondingly greater.
Comment 7 Jonathan Kew (:jfkthame) 2012-04-29 10:42:16 PDT
After a little more experimentation - the scrollable extent of the tab bar seems to be based on the "visual overflow" areas that would be computed for *all* the tabs (not just the rightmost one) if they were displaying their full titles. So a long (but truncated) tab title that's somewhere in the middle of the tab bar can still cause an excess scrollable extent, if it's long enough that its full form would project beyond the actual right edge of the last tab.

At this point, it seems to me that this indicates an error in the tab bar (it shouldn't be using untruncated title strings as a basis for computing the scrollable extent, as this doesn't at all reflect the actual size of the tabs). It may be that bug 746421 triggered this by causing more textboxes to report that they have an overflow of some kind, and this is making the tab bar try to take it into account (but it's doing so incorrectly), but I'm not sure bug 746421 by itself would cause these symptoms if the tab bar were using the right basis for its computations.
Comment 8 Dão Gottwald [:dao] 2012-04-29 11:14:13 PDT
Created attachment 619426 [details] [diff] [review]
patch

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> At this point, it seems to me that this indicates an error in the tab bar
> (it shouldn't be using untruncated title strings as a basis for computing
> the scrollable extent, as this doesn't at all reflect the actual size of the
> tabs).

The tab strip uses an overflow:hidden frame which it doesn't compute the bounds for. I don't even know what DOM API it would use to do what you describe, or why such an API should exist in the first place.

It seems to me that bug 746421 just incorrectly didn't take cropping into account.
Comment 9 Jonathan Kew (:jfkthame) 2012-04-29 11:42:44 PDT
Hmm, I was just about to post the same patch as a possible workaround. Yes, I think bug 746421 should have used the cropped title.

However, I still think there's a deeper issue here; whether the tabs are considered to be scrollable should not depend on the "visual overflow" area but on the "scrollable overflow", which shouldn't have been affected by bug 746421 in the first place.
Comment 10 Dão Gottwald [:dao] 2012-04-29 11:48:56 PDT
(In reply to Dão Gottwald [:dao] from comment #8)
> It seems to me that bug 746421 just incorrectly didn't take cropping into
> account.

Neil thought the same in bug 746421 comment 13. However, see also bug 746421 comment 14. So maybe there's more wrong here. My patch fixes the specific issue this bug was filed for, though, as far as I can tell.
Comment 11 Jonathan Kew (:jfkthame) 2012-04-29 11:53:11 PDT
Ah - on looking again, there's a different problem in bug 746421 - it actually stores the "ink bounds" as both visual *and* scrollable overflow, which doesn't seem right. And that would explain the problem here.
Comment 12 Jonathan Kew (:jfkthame) 2012-04-29 12:20:11 PDT
Created attachment 619432 [details] [diff] [review]
patch, distinguish scrollable from visual bounds for nsTextBoxFrame

I think this fixes the overflow calculations in nsTextBoxFrame more correctly. Note that with the visual/scrollable distinction made properly, even using the uncropped title for the visual overflow calculation would no longer cause the tab-strip problem.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-29 14:32:24 PDT
Comment on attachment 619432 [details] [diff] [review]
patch, distinguish scrollable from visual bounds for nsTextBoxFrame

Review of attachment 619432 [details] [diff] [review]:
-----------------------------------------------------------------

Test please!
Comment 14 Dão Gottwald [:dao] 2012-04-29 15:42:04 PDT
*** Bug 750131 has been marked as a duplicate of this bug. ***
Comment 15 Jim Jeffery not reading bug-mail 1/2/11 2012-04-29 16:41:14 PDT
Try build here: 
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-79ac58cd8ad1/ 
based on cset:
https://hg.mozilla.org/try/rev/79ac58cd8ad1

Assuming this is the final r+ patch, it fixed the problem for me.
Comment 16 Jim Jeffery not reading bug-mail 1/2/11 2012-04-29 16:43:24 PDT
Damn bugzilla changed the status of the bug because I forgot to 'refresh' the page first...
Comment 17 Bill Gianopoulos [:WG9s] 2012-04-29 17:18:40 PDT
I am tempted to raise the severity here to blocker, except that the patch here does fix it so I am creating my own builds without this issue.  Otherwise this would be enough to prevent me from using these builds myself.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-29 17:29:41 PDT
Comment on attachment 619432 [details] [diff] [review]
patch, distinguish scrollable from visual bounds for nsTextBoxFrame

Review of attachment 619432 [details] [diff] [review]:
-----------------------------------------------------------------

Someone land this :-)
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-04-29 17:35:43 PDT
https://hg.mozilla.org/mozilla-central/rev/cfaf90b22fc3

Don't forget the tests!
Comment 20 Bill Gianopoulos [:WG9s] 2012-04-29 19:11:26 PDT
BTW you don't need 8 tabs for this.  I see it with a few as 4.
Comment 21 Jonathan Kew (:jfkthame) 2012-04-30 05:58:04 PDT
Created attachment 619536 [details] [diff] [review]
test for unwanted tab-strip overflow

Confirmed on try that this fails without the patch, and passes with it.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-30 06:00:52 PDT
Comment on attachment 619536 [details] [diff] [review]
test for unwanted tab-strip overflow

Review of attachment 619536 [details] [diff] [review]:
-----------------------------------------------------------------

I was thinking of just a reftest, but OK
Comment 23 Jonathan Kew (:jfkthame) 2012-04-30 06:59:34 PDT
Created attachment 619544 [details] [diff] [review]
reftest for unwanted scroll-overflow due to cropped textbox label

OK, let's have a reftest as well. I don't really speak XUL, but after a little experimentation, this seems to work to verify that a cropped title doesn't lead to a spurious scroll-overflow area.
Comment 24 Dão Gottwald [:dao] 2012-05-01 02:30:26 PDT
*** Bug 750481 has been marked as a duplicate of this bug. ***
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 21:27:29 PDT
Tests:

https://hg.mozilla.org/mozilla-central/rev/9498c59bfeb1
https://hg.mozilla.org/mozilla-central/rev/d60f77b10824

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