If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Horizontal "ghost" scroll bar in the customize window

VERIFIED FIXED

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Ria Klaassen (not reading all bugmail), Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Steps to reproduce:
1. Open Customize Window.
2. Put an icon on the toolbar and close the customize window.
3. After reopening you should see it.

Regression between 1.9a1_2006031504 and 1.9a1_2006031521.

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-03-15+03%3A00&maxdate=2006-03-15+21%3A00

No idea what fix might have caused this but Bug 192767 is about scrolling.
(Reporter)

Comment 1

12 years ago
First try bug 192767, cc dbaron@mozilla.com
(Reporter)

Comment 2

12 years ago
The white scroll bar disappears when I choose "Restore Default Set".
Step two is unnecessary.  You just need to open the customize window twice; the second time it will have the problem.

Comment 4

11 years ago
This occurs on Mac OS X as well -> All/All.
Flags: blocking-firefox3?
OS: Windows XP → All
Hardware: PC → All

Comment 5

11 years ago
Created attachment 249826 [details]
Simple xul testcase.

Demonstrates the phantom horizontal scrollbar with a xul box with overflowed (and flex=1) contents.

Regression range for Mac is between 20060315-05 and 20060316-05, which fits the check-in for bug 192767 being the cause (though this clearly isn't an rtl/bidi issue).

Comment 6

11 years ago
(In reply to comment #5)
I forgot to note that resizing the browser window so that the horizontal scrollbar is partially covered forces a redraw which removes the bad scrollbar. So the code for redrawing the box seems to know something that the initially executed code doesn't.

Comment 7

11 years ago
Created attachment 249987 [details] [diff] [review]
Not a true fix, but a temp fix that highlights the code problem

I can confirm that attachment 215135 [details] [diff] [review] of bug 192767 caused this regression.

I've narrowed it down to one specific section of code. The attached patch is actually a reversal of that section of the original patch. While I don't expect  it's a legitimate fix, I hope it'll help dbaron work out what's wrong.

Basically... when overflow is auto and the horizontal case is tested, the current method of calculating the scrolledContentSize width yields a value that is sometimes greater than the original method (which used GetScrolledSize() ). This means that |scrolledContentSize.width > scrollAreaRect.width| returns true in cases that it originally wouldn't have. Actually, as far as I can tell, it looks like the older method gave a width that was identical to the scrollAreaRect.width.

Hope this helps.
Attachment #249987 - Flags: review?(dbaron)
In the case where this fixes the problem, what are:
 * the old scrollAreaRect?
 * the new scrollAreaRect?
 * scrolledRect?
 * scrolledContentSize?

(You should be able to get all of that from a pair of printfs right before and after your inserted line.)
Blocks: 192767

Comment 9

11 years ago
(In reply to comment #8)
I hope that the width and height values were what you were after (which were always the same as XMost() and YMost() for the nsRects).

Using the xul test case (attachment 249826 [details]), the values I got were:
scrolledRect:              width   = 2250   height  = 3885
scrollAreaRect:            width   = 2250   height  = 2250
scrolledContentSize:       width   = 2250   height  = 3885
Old scrolledContentSize:   width   = 2025   height  = 4403

Where scrolledContentSize is the one using the currently used method, and the "Old" one is the original that used mInner.GetScrolledSize(). The old width is 225 less than the new one, while the height is 518 greater.

Calling up the customize toolbar window (on Mac), I got:
1st time (no difference):
scrolledRect:              width   = 6525   height  = 5280
scrollAreaRect:            width   = 6525   height  = 4078
scrolledContentSize:       width   = 6525   height  = 5280
Old scrolledContentSize:   width   = 6525   height  = 5280

2nd and subsequent times (difference):
scrolledRect:              width   = 6750   height  = 5280
scrollAreaRect:            width   = 6750   height  = 4078
scrolledContentSize:       width   = 6750   height  = 5280
Old scrolledContentSize:   width   = 6525   height  = 5280

Again, the old method gave a width that was 225 less than the new one. The heights are identical.

I loaded a local copy of the test case, and changed the width and height of the box. Regardless of the values that I used, I found that the "old" width was always exactly 225 lower than the current one, unless (a) the height was increased until there was no overflow, in which case the "old" width value increased by 225 to be the same as the current one, or (b) the width was squeezed until the horizontal overflow was legitimate. In the latter case, the old and current width values did vary a little (by 37) when the box was set to a width of 5em, but the values were identical once the box was set to 4em or less (in which case the scrollbar was too narrow for a scroll button to be drawn).

I don't know what's magic about the value of 225. It happens to be the square of 15, which is the exact height of the scrollbar in pixels, but I assume that's a coincidence (?!?).

Comment 10

11 years ago
I see it now. And it's nothing to do with the method used for getting the rectangle dimensions. It's due to the vertical scrollbar being added here:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsGfxScrollFrame.cpp#2179

Adding the vertical scrollbar reduces the scrolledContentSize width by 225. In the original code, the scrolledContentSize was queried again at the start of the horizontal scrollbar check (which is below the vertical scrollbar code), so it got the new width. But with the current code, the dimensions are only calculated before the vertical scroll is checked for possible addition or removal, so the width can be wrong. Recalculating the scrolledContentSize after the vertical scrollbar code fixes it. I presume the |if (needsLayout)| code is intended to do this already?
Created attachment 250179 [details] [diff] [review]
patch

Right.  Thanks for figuring that out.  Here's a patch to do that.

This restores the refetching code that existed before the patch to bug 192767 by scoping the variables to a scope in which they're still valid (well, not exactly, since the call to LayoutScrollArea is near the end of the first scope).
Attachment #249987 - Attachment is obsolete: true
Attachment #250179 - Flags: superreview?(roc)
Attachment #250179 - Flags: review?(roc)
Attachment #249987 - Flags: review?(dbaron)
Blocks: 365666

Comment 12

11 years ago
(In reply to comment #11)
Thanks David. The patch seems to work well for me.

Attachment #250179 - Flags: superreview?(roc)
Attachment #250179 - Flags: superreview+
Attachment #250179 - Flags: review?(roc)
Attachment #250179 - Flags: review+
Checked in to trunk.

Thanks again for figuring it out.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 14

11 years ago
VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a2pre) Gecko/20070104 Minefield/3.0a2pre ID:2007010404 [cairo]
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: blocking-firefox3?
You need to log in before you can comment on or make changes to this bug.