330673 - Horizontal "ghost" scroll bar in the customize window

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Ria Klaassen (not reading all bugmail)]

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.

Comment 1

[Ria Klaassen (not reading all bugmail)]

First try bug 192767, cc [e-mail]

Comment 2

[Ria Klaassen (not reading all bugmail)]

The white scroll bar disappears when I choose "Restore Default Set".

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Step two is unnecessary.  You just need to open the customize window twice; the second time it will have the problem.

Comment 4

[Wayne Woods]

This occurs on Mac OS X as well -> All/All.

Comment 5

[Wayne Woods]

Attachment: 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

[Wayne Woods]

(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

[Wayne Woods]

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Wayne Woods]

(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

[Wayne Woods]

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?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 12

[Wayne Woods]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Thanks again for figuring it out.

Comment 14

[-fullmetaljacket-]

VERIFIED

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