non-zero GtkRange::trough-border value produces strange boxes in Firefox

If I run the reftests under Xvfb (like so:

$ xvfb-run -a -s "-screen 0 1024x1024x24" make reftest > reftest.log 2>&1

) I get twenty-one failures that don't happen if I run the reftests on the primary X server. All of them involve scroll bars in some way -- the most common syndrome is that one member of a reftest pair draws a horizontal scrollbar under some element and the other member doesn't.

There are two prominent differences between the enviroment under Xvfb and the normal environment: Xvfb doesn't implement the RANDR extension, and (as run above) there's no window manager available.

I'm attaching the logfile from a reftest run under Xvfb. The failures that don't appear in a regular build are:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/234964-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/308406-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/308406-2.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/360757-1b.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/362901-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/367247-s-hidden.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/372037-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/374719-1.xul |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/386470-1a.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/386470-1c.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/402567-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/402567-2.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/410621-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/430412-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/bugs/456484-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/percent-overflow-sizing/simpleAbsHeightD.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/percent-overflow-sizing/simpleAbsHeight.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/percent-overflow-sizing/simpleAbsMinHeightD.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/percent-overflow-sizing/simpleHeight100D.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/percent-overflow-sizing/simpleHeight100.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///home/zack/src/mozilla/moz-central/layout/reftests/percent-overflow-sizing/simpleMinHeight100D.html |

Created attachment 370142
reftest log showing failures

Here's the log I said I was attaching; it is nearly nine megabytes uncompressed, which I guess is too big for bugzilla.

I tracked this down to bug 485030. Reverting the change in bug 485030 fixes the issue for me.

Created attachment 371116
testcase

Created attachment 371117
what the testcase looks like with this bug

Weird. I hope Zack can track this down ... gDumpPaintList=1 would be a good start

(In reply to comment #5)
> Weird. I hope Zack can track this down ... gDumpPaintList=1 would be a good
> start

The display lists don't seem to provide any clues. When the phantom scrollbar is drawn there are a few extra entries for the scrollbar and scrollbarbutton etc, but that is the only difference.

Here is the relevant display list output from a plain trunk build (where the bug occurs).

Painting --- before optimization (dirty 0,0,61440,34800):
SolidColor 0xb0e925b0(Viewport(-1)) (0,0,61440,34800)
Clip (nil)() (0,0,61440,34800)
    CanvasBackground 0xb0e92690(Canvas(html)(-1)) (0,0,61440,34800) uniform
Clip (nil)() (0,0,61440,34800)
    Background 0xb0eb7d10(HTMLScroll(div)(1)) (480,480,6000,6000) opaque uniform
    Background 0xb0eb7dfc(ScrollbarFrame(scrollbar)(-1)) (480,6480,6000,120)
    Border 0xb0eb7dfc(ScrollbarFrame(scrollbar)(-1)) (480,6480,6000,120)
    Background 0xb0ebb040(ButtonBoxFrame(scrollbarbutton)(-1)) (540,6540,840,840)
    Background 0xb0ebb1fc(ButtonBoxFrame(scrollbarbutton)(-1)) (5580,6540,840,840)
    Clip (nil)() (480,480,6000,6000)
        Text 0xb0ebba0c(Text(0)) (480,480,540,1140)
        nsDisplayReflowCount 0xb0ebba0c(Text(0)) (480,480,540,1140)
Painting --- after optimization:
SolidColor 0xb0e925b0(Viewport(-1)) (0,0,61440,34800)
Clip (nil)() (0,0,61440,34800)
    CanvasBackground 0xb0e92690(Canvas(html)(-1)) (0,0,61440,34800) uniform
    Background 0xb0eb7dfc(ScrollbarFrame(scrollbar)(-1)) (480,6480,6000,120)
    Border 0xb0eb7dfc(ScrollbarFrame(scrollbar)(-1)) (480,6480,6000,120)
    Background 0xb0ebb040(ButtonBoxFrame(scrollbarbutton)(-1)) (540,6540,840,840)
    Background 0xb0ebb1fc(ButtonBoxFrame(scrollbarbutton)(-1)) (5580,6540,840,840)
Painting --- before optimization (dirty 480,480,6000,6000):
SolidColor 0xb0e925b0(Viewport(-1)) (0,0,61440,34800)
Clip (nil)() (0,0,61440,34800)
    CanvasBackground 0xb0e92690(Canvas(html)(-1)) (0,0,61440,34800) uniform
Clip (nil)() (0,0,61440,34800)
    Background 0xb0eb7d10(HTMLScroll(div)(1)) (480,480,6000,6000) opaque uniform
    Clip (nil)() (480,480,6000,6000)
        Text 0xb0ebba0c(Text(0)) (480,480,540,1140)
        nsDisplayReflowCount 0xb0ebba0c(Text(0)) (480,480,540,1140)
Painting --- after optimization:
Clip (nil)() (0,0,61440,34800)
    Background 0xb0eb7d10(HTMLScroll(div)(1)) (480,480,6000,6000) opaque uniform
    Clip (nil)() (480,480,6000,6000)
        Text 0xb0ebba0c(Text(0)) (480,480,540,1140)
        nsDisplayReflowCount 0xb0ebba0c(Text(0)) (480,480,540,1140)

And here is the display list from a build with the change in bug 485030 reverted (the bug does not occur).

Painting --- before optimization (dirty 0,0,61440,34800):
SolidColor 0xaff995b0(Viewport(-1)) (0,0,61440,34800)
Clip (nil)() (0,0,61440,34800)
    CanvasBackground 0xaff99690(Canvas(html)(-1)) (0,0,61440,34800) uniform
Clip (nil)() (0,0,61440,34800)
    Background 0xaffcbd10(HTMLScroll(div)(1)) (480,480,6000,6000) opaque uniform
    Clip (nil)() (480,480,6000,6000)
        Text 0xaffd3a0c(Text(0)) (480,480,540,1140)
        nsDisplayReflowCount 0xaffd3a0c(Text(0)) (480,480,540,1140)
Painting --- after optimization:
SolidColor 0xaff995b0(Viewport(-1)) (0,0,61440,34800)
Clip (nil)() (0...

Created attachment 371605
quick fix?

When drawing the possible scrollbar stuff for a nsGfxScrollFrame in nsGfxScrollFrameInner::BuildDisplayList I restricted the dirty rect to the overflow rect for the nsGfxScrollFrame and that fixes the testcase for me. This feels like it is fixing the symptom and not the root cause.

> This feels like it is fixing the symptom and not the root cause.

Yeah, it shouldn't be trying to draw the scroll bar in the first place, 'cos there's no overflow to be scrolled! I think this is a deeper bug that we were just masking in the past.

I think what's happening is that to fix bug 485030 we stopped clipping the scrollframe dirty rect the scrollframe overflow area. Hidden scrollbars are collapsed so that their size is (0,0) but we don't actually get around to collapsing the scroll buttons and slider children of the scrollbar, so now those children are intersecting the dirty area and being displayed. I'm not sure why this isn't affecting other configurations.

The attached patch is pretty close and would be OK, but it's probably a slightly better fix for nsGfxScrollFrameInner::BuildDisplayList to just check kid->GetStyleVisibility()->mVisible == NS_STYLE_VISIBILITY_VISIBLE.

Created attachment 371712
check visibility version of above patch

(In reply to comment #9)
<snip>
> The attached patch is pretty close and would be OK, but it's probably a
> slightly better fix for nsGfxScrollFrameInner::BuildDisplayList to just check
> kid->GetStyleVisibility()->mVisible == NS_STYLE_VISIBILITY_VISIBLE.

Thanks, this is what I wanted to do but didn't have time to find the right thing to check. Except it doesn't fix the phantom scrollbar showing up, so the scroll bar is getting set to visible when it shouldn't.

Yeah, I realized after I went to sleep that was wrong, because the way scrollframe hides the scrollbars is just setting their size to zero. So just use kid->GetRect()->IsEmpty().

(In reply to comment #11)
> Yeah, I realized after I went to sleep that was wrong, because the way
> scrollframe hides the scrollbars is just setting their size to zero. So just
> use kid->GetRect()->IsEmpty().

I realized after I tried this and saw that it was not working that it wasn't going to work :). Since it is drawing the scroll buttons, their GetRect() will not be empty.

I've been poking at this with debugging printfs, and indeed, despite the GfxScrollFrameInner object *thinking* it has hidden the horizontal scrollbar - that is, mHasHorizontalScrollbar is false and GetScrollPortSize() returns a rectangle with no space reserved for the horizontal scrollbar - the ScrollbarFrame for the horizontal scrollbar is visible and has a nonempty bounding rectangle. I'm able to reproduce this under xvfb *and* in a regular, but barebones, X session - just one xterm, a window manager, and ./dist/bin/firefox <tn's testcase>. It does *not* happen under a regular GNOME session, which continues to baffle me.

Anyway, this has moved my attention from the display list phase to the reflow phase, but I'm still trying to work out how that all works.

If it helps I'm seeing this bug when VNCing into a ubuntu box (I assume that the vnc server must be using xvfb). The vnc session has all the trimmings of a regular ubuntu desktop, GNOME, GDM etc, and looks the same as on a real monitor in every way (including the greeting screen where you log in).

I'm also seeing another scrollbar bug that only happens under VNC that may or may not be related. When scrolling a scrollbar with the mouse (either dragging the slider or clicking and holding on the scroll buttons) the slider will disappear when it hits the end of the scroll bar, and then reappear when you release the mouse. It is only a regression in mozilla-central and it occurs in the 2009-03-21 nightly, but not in the 2009-03-20 nightly. It may have the same root cause as this bug. One thing I would like to try when I can get around to it is to apply the patch in bug 485030 to a source tree from before/after 2009-03-21 (but before 485030 got checked in), and see if it still causes the same issue.

nsGfxScrollFrameInner::LayoutScrollbars should be setting the vertical scrollbar width to zero or the horizontal height to zero when we don't want that scrollbar to be visible. Stepping through that function when we're reflowing the scroll area of interest would be interesting.

Making some more progress debugging here: as far as I can tell, nsGfxScrollFrameInner is fine. It's correctly determining that the box does not need scroll bars, and correctly setting the height of the horizontal scroll bar to zero. But in between when it does that and when we actually draw anything, the pres shell finds the horizontal scrollbar frame on its mDirtyRoots list and calls its Reflow method, which begins by delegating to nsBoxFrame::Reflow, which resets the height of the scroll bar to its intrinsic height. That's not fatal by itself; nsScrollBarFrame::Reflow is aware that nsGfxScrollFrame may have set its width or height to zero to hide it, so it checks whether the availableWidth/availableHeight in its aReflowState argument are zero, and if so, overrides the corresponding 'desired size' values back to zero, which in turn causes nsPresShell to zap the scrollbar height back to zero.

Now here's the kicker: somewhere in between the call to SetBounds(state, r) on line 754 of nsBoxFrame.cpp (this is nsBoxFrame::Reflow), and the special check in nsScrollBarFrame::Reflow, *something* is stomping on aReflowState.availableHeight: it was 0 and it becomes 0x40000000 (NS_INTRINSICSIZE/NS_UNCONSTRAINEDSIZE). However, infuriatingly, no GDB watchpoint fires when this happens, so I am failing to find the event.

Hum, actually, I take that back. aReflowState.availableHeight is NS_UNCONSTRAINEDSIZE from the get-go; nsPresShell::DoReflow sets it up that way:

  // Don't pass size directly to the reflow state, since a
  // constrained height implies page/column breaking.
  nsSize reflowSize(size.width, NS_UNCONSTRAINEDSIZE);
  nsHTMLReflowState reflowState(mPresContext, target, rcx, reflowSize);

I wonder if it should only do that when target == rootFrame. Thoughts?

Note that this is also tripping the assertions at the end of nsPresShell::DoReflow about size changing during incremental reflow...

And I still have *no idea* why this is environment-dependent. Based on the above analysis it seems like it ought to be wrong all the time.

Changing line 6855 of nsPresShell.cpp to

  nsHTMLReflowState reflowState(mPresContext, target, rcx,
                                target == rootFrame ? reflowSize : size);

makes tn's test case pass in xvfb, and makes all the reftest failures go away in xvfb as well, but I'm still getting floods of these assertions:

###!!! ASSERTION: reflow roots must not have visible overflow: 'desiredSize.mOverflowArea == nsRect(nsPoint(0, 0), nsSize(desiredSize.width, desiredSize.height))', file /home/zack/src/mozilla/moz-central/layout/base/nsPresShell.cpp, line 6888
###!!! ASSERTION: reflow roots should never split: 'status == NS_FRAME_COMPLETE', file /home/zack/src/mozilla/moz-central/layout/base/nsPresShell.cpp, line 6890
###!!! ASSERTION: reflow state computed incorrect width: 'reflowState.ComputedWidth() == size.width - reflowState.mComputedBorderPadding.LeftRight()', file /home/zack/src/mozilla/moz-central/layout/base/nsPresShell.cpp, line 6870

none of which appear if reftests are run on the normal x server.

I'm tempted to take the patch in comment #7 except that it won't work when the vertical scrollbar is on the left-hand side.

Setting the availableHeight is a bad idea since that can trigger page breaking.

How about just changing nsScrollbarFrame::Reflow so that if it is a reflow root (i.e. its aReflowState has no parentReflowState), then we just save its current size on entry and set aDesiredSize to that size on exit, thus ensuring that the presshell assertions are not fired and presumably fixing this bug?

Doesn't work; still got the assertions. I tried several variations on your suggestion, some of which actually make *vertical* scrollbars start showing up when they shouldn't.

My best guess for why it doesn't work is that some of the other things nsBoxFrame::Reflow does, when called under these conditions, are inconsistent with having aDesiredSize forced to the original size.

On a more positive note, I've figured out why it was only showing up in xvfb: the phenomenon is gtk-theme dependent. For instance, "Clearlooks" doesn't do it, but "Glider" does.

Created attachment 372172
WIP patch part 1: make nsIFrame::IsCollapsed callable without an nsBoxLayoutState

I know what's going on now and I need a bit of help to resolve the bug. Here's part one of my work-in-progress patch, which just removes the (totally unused) nsBoxLayoutState& parameter to nsIFrame::IsCollapsed(). I need this so I can call it from nsCSSReflowState::InitOffsets(). I think this can go in now, by itself. Explanation along with next patch.

Created attachment 372174
WIP patch part 2: set visibility:collapse on scrollbars and check for that before letting the theme specify borders for 'em

Ok, here's the patch that attempts to fix this bug. It seems to work, but it's doing enough dodgy stuff that I'm not sure it's the *right* fix. It has a bunch of debugging code in it whose output is meant to be read along with the reflow trace report (GECKO_DISPLAY_REFLOW_RULES_FILE).

The ultimate cause of the problem is that some, but not all, Gtk themes specify a one-pixel widget border for their scrollbars. This is recorded in the reflow state by nsCSSOffsetState::InitOffsets, which is totally ignorant of whether the scrollbar in question has been allotted any space! This causes an assertion in nsPresShell::DoReflow to fire before we even get to calling the frame reflow method ("reflow state computed incorrect width"). It then goes on to derange the desiredSize that comes back from the frame reflow method (even with roc's original suggestion), and boom.

My fix for this is to set visibility:collapse on the anonymous content element associated with the scrollbar, and check that in nsCSSOffsetState::InitOffsets, but as I say, this feels kinda dodgy. But it works on tn's test case. (Reftest run still in progress.) Abusing r/sr to ask both roc and dbaron to look at it ;-) (Obviously any checked in version would remove all the debugging printfs.)

This bug bites me hard in KDE so, I decided to try the above patches(10+ extra scroll bars per page).

Using a tip build plus the patches; the extra half height scrollbars are gone, but sometimes the normal scroll bars are gone too. For example: in this bug there is no scrollbar on the right side of the page. The Slashdot front page has scrollbars, but the comment pages have none, even thou the page is really long.

example url http://tech.slashdot.org/article.pl?sid=09/04/13/0120226

Comment on attachment 372172
WIP patch part 1: make nsIFrame::IsCollapsed callable without an nsBoxLayoutState

I'll take this on general cleanup

I don't really want to go down the road of the second patch, though.

Here's another plan: instead of iterating through the kids of the scrollframe in BuildDisplayList, just explicitly do
  if (mHScrollbarBox && mHasHorizontalScrollbar) {
    rv = mOuter->BuildDisplayListForChild(aBuilder, mHScrollbarBox, aDirtyRect, aLists);
    NS_ENSURE_SUCCESS(rv, rv);
  }
etc

I guess we don't need to check mHScrollbarBox there because mHasHorizontalScrollbar can't be true if there is no scrollbar (I hope!)

(In reply to comment #26)
> Here's another plan: instead of iterating through the kids of the scrollframe
> in BuildDisplayList, just explicitly do
> if (mHScrollbarBox && mHasHorizontalScrollbar) {
> rv = mOuter->BuildDisplayListForChild(aBuilder, mHScrollbarBox, aDirtyRect,
> aLists);
> NS_ENSURE_SUCCESS(rv, rv);
> }
> etc

That will fix the bad painting but not the assertions; as I say in comment 23, we've broken frame tree invariants as early as nsCSSOffsetState::InitOffsets() [when called on a collapsed scrollbar which happens to be a dirty reflow root, with a theme that specifies a border for it]. To fix that, either we have to stop these collapsed scrollbars from being treated as dirty reflow roots in the first place, or we have to figure out some way of communicating to InitOffsets that it should ignore the border specified by the theme.

It occurred to me yesterday that we could just clamp the various margins computed by InitOffsets to the stated size of the border-box (zero in this case). That might be enough to make PresShell::DoReflow happy, and might even eliminate the need for your trick in BuildDisplayList. Can you think of any reason that won't work?

The problem with your second patch is that setting attributes during reflow (or changing styles in other ways) is strictly forbidden. You could get away with it in a post-reflow callback but it's still a heavyweight solution. Modifying InitOffsets sounds like it could work.

Created attachment 373011
patch part 2 (works now): just clamp computed border/padding to containing box

Here is a fully working patch - no inappropriate scrollbars, no pres shell assertions - that doesn't need to touch styles at all and (I think) gets to the real root of the problem. Basically, when we set up a reflow-state object, we were already clamping the computed height and width to the containing block's height and width, but we also need to do the same to the 'mComputedBorderPadding' and 'mComputedPadding' margin objects, when even they do not fit. I have not been able to write a HTML-only test case for this -- a HTML box's size seems to be bounded below by the size of its border+padding, even if there are explicit width:/height: settings in CSS. In the scenario that triggers the bug, the bad borders are coming from the native theme and do not go through whatever calculation forces a larger size. (Which is as it should be, since nsGfxScrollFrame needs to be able to force a scrollbar's dimensions to zero no matter what the theme wants.)

We really do need all of the logic I added to nsHTMLReflowState; under some (unclear to me) conditions, the box is smaller than its border+padding but not of zero size.

Comment on attachment 373011
patch part 2 (works now): just clamp computed border/padding to containing box

I think dbaron is the better reviewer here

What native theme objects are setting padding that's larger than what they report in GetMinimumWidgetSize? That sounds like a bug. (Or maybe we should make it so it's not a bug?)

I don't know if any native theme objects are doing that. Scrollbars are a little weird - nsGfxScrollFrame hides scrollbars (for overflow:auto) by forcing their width or height to zero, which we then *have to* honor at all times throughout reflow and paint or we get these display errors and/or assertions.

*** Bug 490276 has been marked as a duplicate of this bug. ***

Artem: to be 100% sure your problem is due to this bug try changing your Gtk+ theme and see if the problem goes away. (I see it with "Glider" but not with "Clearlooks", for instance.)

For me the bug is visible with e.g. the ThinIce theme but everything's fine with the Clearlooks theme.

ThinIce appears to be another of the themes that puts a 1px border on its scrollbars, so yes, you're hitting this bug.

Peter(6) said:

> If I understand this bug right it's an issue with non-default themes.

In Fedora 10 it happens with default GTK2 theme: at the time when I hit this bug I had no gtkrc file and only later created it for experiments.

It'll happen with any theme that puts a border on its scrollbars. I've retitled the bug to be clearer.

(In reply to comment #32)
> What native theme objects are setting padding that's larger than what they
> report in GetMinimumWidgetSize? That sounds like a bug. (Or maybe we should
> make it so it's not a bug?)

I'm not sure how all of this is *supposed* to work, but what jumps out at me now that I've finally gotten around to looking at the code, is that NS_THEME_SCROLLBAR_TRACK_* have no entries in nsNativeThemeGTK::GetMinimumWidgetSize. They do, however, appear in ::GetWidgetBorder. This may be the thing that should really change. I'm running some experiments now.

Setting a minimum widget size for NS_THEME_SCROLLBAR_TRACK_* has no effect on the bug.

*** Bug 487816 has been marked as a duplicate of this bug. ***

*** Bug 495654 has been marked as a duplicate of this bug. ***

Comment on attachment 373011
patch part 2 (works now): just clamp computed border/padding to containing box

Sorry I didn't get to this much sooner.

Reducing the border and/or padding in general is incorrect. The correct rendering of the testcase:

<div style="width: 3px">
  <div style="border: 5px;height:20px"></div>
</div>

is to have the inner div's border-box still be 10px wide (and 30px tall), overflowing its parent. (I hope you'll find that that's true across browsers, although I admit I haven't checked.)

It looks to me like this patch changes that, at least in some cases.

Well, it only changes things for the reflow root, it looks like. But still, it seems extremely complicated (and a lot of pretty untested code) for just fixing an issue with scrollbars.

(Can we just make the scrollbar frame not put the scrollbars on the display list when they're supposed to be hidden, instead of trying to reflow them at 0 size?)

I'll look at your nested-div case, but one immediate comment:

> (Can we just make the scrollbar frame not put the scrollbars on the display
> list when they're supposed to be hidden, instead of trying to reflow them at 0
> size?)

this sounds like the approach roc suggested in comment 26; it corrects the visual errors but not the "size changed during incremental reflow" assertions. The pres shell is trying to reflow the scrollbars themselves, and AFAICT there is nothing the scrollframe can do to stop it.

Created attachment 387583
alternative much smaller patch

Here's an alternative, much smaller patch, that just puts knowledge of the special rules for scrollbars into nsCSSOffsetState::InitOffsets. I get no reftest failures and no reflow assertions with this patch (alone). Maybe it's less problematic?

Comment on attachment 387583
alternative much smaller patch

>+ nsCOMPtr<nsIAtom> frameType(frame->GetType());

nsIAtom *frameType = frame->GetType();

r=dbaron with that, I suppose

(In reply to comment #46)
> this sounds like the approach roc suggested in comment 26; it corrects the
> visual errors but not the "size changed during incremental reflow" assertions.
> The pres shell is trying to reflow the scrollbars themselves, and AFAICT there
> is nothing the scrollframe can do to stop it.

... even if you do this *instead* of setting the size to 0 (rather than in addition)?

(In reply to comment #47)
> Created an attachment (id=387583) [details]
> alternative much smaller patch
>
> Here's an alternative, much smaller patch, that just puts knowledge of the
> special rules for scrollbars into nsCSSOffsetState::InitOffsets. I get no
> reftest failures and no reflow assertions with this patch (alone). Maybe it's
> less problematic?

This works like a charm. Haven't found any odd scrollbars yet. Thanks.

Created attachment 387776
third alternative; as suggested in comment 49; non-working

(In reply to comment #49)
> (In reply to comment #46)
> > this sounds like the approach roc suggested in comment 26; it corrects
> > the visual errors but not the "size changed during incremental reflow"
> > assertions. The pres shell is trying to reflow the scrollbars themselves,
> > and AFAICT there is nothing the scrollframe can do to stop it.
>
> ... even if you do this *instead* of setting the size to 0 (rather than in
> addition)?

I wanted to try this, but I'm embarrassed to report I can't find the code that actually sets the scrollbar size to 0! This is as far as I got.

On further investigation I really don't think this is going to work. As far as I can tell, what's setting the scrollbar size to 0 is the call to nsBoxFrame::LayoutChildAt from LayoutAndInvalidate (static function in nsGfxScrollFrame.cpp) from nsGfxScrollFrameInner::LayoutScrollbars ... and lying to that function about the amount of space available to the scrollbar just trips *different* pres shell assertions ("reflow roots must not have visible overflow").

Why can't we not place the scrollbar at all?

Created attachment 388372
alt patch, rev 2: unnecessary nsCOMPtr removed

Per comment #48.

Do we have a maintainer for the Gtk theme code?

Michael Ventnor

Comment on attachment 388372
alt patch, rev 2: unnecessary nsCOMPtr removed

dbaron already said r=dbaron with this change

Well, he didn't seem very happy about it. But I've spent a lot of time on alternate approaches to this bug with no luck, so *shrug*. Let's get it landed.

http://hg.mozilla.org/mozilla-central/rev/4e85941dfbb5

this fixes our themes with GtkRange::trough-border != zero

Comment on attachment 388372
alt patch, rev 2: unnecessary nsCOMPtr removed

ubuntu 9.10 plans to use theme with GtkRange::trough-border;
would be great if this would be considered for 1.9.1 branch.

Is this bug actually visible on 1.9.1 branch without the patch? We were under the impression that, as a regression from bug 485030, it did not affect 1.9.1.

For the record, this patch was modified in http://hg.mozilla.org/mozilla-central/rev/8ea02ebd43f0 and if we port this, we should port that as well.

The bug that's visible on 1.9.1 (and was apparently fixed on mozilla-central by this bug's checkin) is bug 500368, if I understand correctly from an IRC conversation with that bug's reporter yesterday.

I've generated a try-server build of mozilla1.9.1 + this bug's fix + the followup fix that dbaron mentioned in comment 62:
https://<email address hidden>/

Eric / Alexander, if you guys are hitting either this bug or bug 500368 in Firefox 3.5.x, could you test that try-server build to confirm that it's fixed there?

Yes, it is fixed.

Comment on attachment 388372
alt patch, rev 2: unnecessary nsCOMPtr removed

To take this on the 1.9.1 branch we'd need a roll-up back-port including the fixes in bug 510748 (right?) and a testcase we can check in.

Created attachment 407691
rollup patch for 1.9.1

Rolled up with bug 510748 and back ported to 1.9.1 and including a testcase.

The testcase probably won't fail on tinderbox because this bug depends on the theme the OS is using. The testcase should also be checked into 1.9.2 and m-c.

(In reply to comment #67)
> The testcase probably won't fail on tinderbox because this bug depends on the
> theme the OS is using. The testcase should also be checked into 1.9.2 and m-c.

What I meant was, the testcase will probably pass on tinderbox both before and after this patch is applied.

Comment on attachment 407691
rollup patch for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers

Pushed the test to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/c724ba56c88c

Pushed the test to 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/489e7359206a

Pushed the rollup patch to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/27d9d4107522