Last Comment Bug 486065 - hidden scrollbars get drawn anyway when Gtk theme gives scrollbars borders
: hidden scrollbars get drawn anyway when Gtk theme gives scrollbars borders
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: x86 Linux
: -- normal with 2 votes (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
: 487816 490276 495654 (view as bug list)
Depends on: 510748
Blocks: 485030 500368
  Show dependency treegraph
 
Reported: 2009-03-30 21:27 PDT by Zack Weinberg (:zwol)
Modified: 2010-09-17 17:17 PDT (History)
23 users (show)
roc: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.6-fixed


Attachments
reftest log showing failures (232.93 KB, application/x-bzip)
2009-03-30 21:31 PDT, Zack Weinberg (:zwol)
no flags Details
testcase (122 bytes, text/html)
2009-04-05 02:15 PDT, Timothy Nikkel (:tnikkel)
no flags Details
what the testcase looks like with this bug (26.32 KB, image/png)
2009-04-05 02:15 PDT, Timothy Nikkel (:tnikkel)
no flags Details
quick fix? (1.23 KB, patch)
2009-04-07 21:19 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
check visibility version of above patch (1008 bytes, patch)
2009-04-08 12:47 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
WIP patch part 1: make nsIFrame::IsCollapsed callable without an nsBoxLayoutState (30.38 KB, patch)
2009-04-10 19:08 PDT, Zack Weinberg (:zwol)
roc: review+
roc: superreview+
Details | Diff | Splinter Review
WIP patch part 2: set visibility:collapse on scrollbars and check for that before letting the theme specify borders for 'em (8.61 KB, patch)
2009-04-10 19:18 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
patch part 2 (works now): just clamp computed border/padding to containing box (7.21 KB, patch)
2009-04-15 17:04 PDT, Zack Weinberg (:zwol)
dbaron: review-
dbaron: superreview-
Details | Diff | Splinter Review
alternative much smaller patch (2.04 KB, patch)
2009-07-08 19:14 PDT, Zack Weinberg (:zwol)
dbaron: review+
Details | Diff | Splinter Review
third alternative; as suggested in comment 49; non-working (8.02 KB, patch)
2009-07-09 17:21 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
alt patch, rev 2: unnecessary nsCOMPtr removed (1.44 KB, patch)
2009-07-13 17:01 PDT, Zack Weinberg (:zwol)
roc: review+
dveditz: approval1.9.1.6-
Details | Diff | Splinter Review
rollup patch for 1.9.1 (3.72 KB, patch)
2009-10-21 20:29 PDT, Timothy Nikkel (:tnikkel)
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Description Zack Weinberg (:zwol) 2009-03-30 21:27:44 PDT
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 |
Comment 1 Zack Weinberg (:zwol) 2009-03-30 21:31:03 PDT
Created attachment 370142 [details]
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.
Comment 2 Timothy Nikkel (:tnikkel) 2009-04-05 02:12:09 PDT
I tracked this down to bug 485030. Reverting the change in bug 485030 fixes the issue for me.
Comment 3 Timothy Nikkel (:tnikkel) 2009-04-05 02:15:02 PDT
Created attachment 371116 [details]
testcase
Comment 4 Timothy Nikkel (:tnikkel) 2009-04-05 02:15:37 PDT
Created attachment 371117 [details]
what the testcase looks like with this bug
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-05 14:59:19 PDT
Weird. I hope Zack can track this down ... gDumpPaintList=1 would be a good start
Comment 6 Timothy Nikkel (:tnikkel) 2009-04-07 02:00:09 PDT
(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,0,61440,34800)
    CanvasBackground 0xaff99690(Canvas(html)(-1)) (0,0,61440,34800) uniform
Painting --- before optimization (dirty 480,480,6000,6000):
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:
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)

I'm continuing to look into this...
Comment 7 Timothy Nikkel (:tnikkel) 2009-04-07 21:19:15 PDT
Created attachment 371605 [details] [diff] [review]
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.
Comment 8 Zack Weinberg (:zwol) 2009-04-07 23:25:46 PDT
> 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.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-08 03:36:10 PDT
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.
Comment 10 Timothy Nikkel (:tnikkel) 2009-04-08 12:47:13 PDT
Created attachment 371712 [details] [diff] [review]
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.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-08 13:07:06 PDT
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().
Comment 12 Timothy Nikkel (:tnikkel) 2009-04-08 13:53:17 PDT
(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.
Comment 13 Zack Weinberg (:zwol) 2009-04-08 14:43:06 PDT
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.
Comment 14 Timothy Nikkel (:tnikkel) 2009-04-08 15:58:37 PDT
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.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-08 16:26:53 PDT
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.
Comment 16 Zack Weinberg (:zwol) 2009-04-08 16:52:26 PDT
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.
Comment 17 Zack Weinberg (:zwol) 2009-04-08 17:00:25 PDT
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...
Comment 18 Zack Weinberg (:zwol) 2009-04-08 17:21:36 PDT
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.
Comment 19 Zack Weinberg (:zwol) 2009-04-08 18:15:00 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-08 19:22:07 PDT
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?
Comment 21 Zack Weinberg (:zwol) 2009-04-10 12:26:15 PDT
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.
Comment 22 Zack Weinberg (:zwol) 2009-04-10 19:08:25 PDT
Created attachment 372172 [details] [diff] [review]
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.
Comment 23 Zack Weinberg (:zwol) 2009-04-10 19:18:53 PDT
Created attachment 372174 [details] [diff] [review]
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.)
Comment 24 adrian 2009-04-12 22:43:51 PDT
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 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-13 02:15:14 PDT
Comment on attachment 372172 [details] [diff] [review]
WIP patch part 1: make nsIFrame::IsCollapsed callable without an nsBoxLayoutState

I'll take this on general cleanup
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-13 02:20:51 PDT
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
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-13 02:34:09 PDT
I guess we don't need to check mHScrollbarBox there because mHasHorizontalScrollbar can't be true if there is no scrollbar (I hope!)
Comment 28 Zack Weinberg (:zwol) 2009-04-13 09:50:05 PDT
(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?
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-13 13:54:55 PDT
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.
Comment 30 Zack Weinberg (:zwol) 2009-04-15 17:04:28 PDT
Created attachment 373011 [details] [diff] [review]
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 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-15 17:15:54 PDT
Comment on attachment 373011 [details] [diff] [review]
patch part 2 (works now): just clamp computed border/padding to containing box

I think dbaron is the better reviewer here
Comment 32 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2009-05-05 16:12:32 PDT
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?)
Comment 33 Zack Weinberg (:zwol) 2009-05-05 16:22:25 PDT
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.
Comment 34 Artem S. Tashkinov 2009-05-11 20:59:47 PDT
*** Bug 490276 has been marked as a duplicate of this bug. ***
Comment 35 Zack Weinberg (:zwol) 2009-05-11 21:28:30 PDT
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.)
Comment 36 Artem S. Tashkinov 2009-05-11 22:42:25 PDT
For me the bug is visible with e.g. the ThinIce theme but everything's fine with the Clearlooks theme.
Comment 37 Zack Weinberg (:zwol) 2009-05-11 22:56:35 PDT
ThinIce appears to be another of the themes that puts a 1px border on its scrollbars, so yes, you're hitting this bug.
Comment 38 Artem S. Tashkinov 2009-05-12 01:15:52 PDT
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.
Comment 39 Zack Weinberg (:zwol) 2009-05-12 08:31:59 PDT
It'll happen with any theme that puts a border on its scrollbars.  I've retitled the bug to be clearer.
Comment 40 Zack Weinberg (:zwol) 2009-05-15 14:09:37 PDT
(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.
Comment 41 Zack Weinberg (:zwol) 2009-05-15 15:06:22 PDT
Setting a minimum widget size for NS_THEME_SCROLLBAR_TRACK_* has no effect on the bug.
Comment 42 Walter Meinl 2009-05-24 08:44:18 PDT
*** Bug 487816 has been marked as a duplicate of this bug. ***
Comment 43 dolphinling 2009-05-31 03:14:34 PDT
*** Bug 495654 has been marked as a duplicate of this bug. ***
Comment 44 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2009-07-08 14:38:00 PDT
Comment on attachment 373011 [details] [diff] [review]
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.
Comment 45 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2009-07-08 14:44:08 PDT
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?)
Comment 46 Zack Weinberg (:zwol) 2009-07-08 15:28:40 PDT
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.
Comment 47 Zack Weinberg (:zwol) 2009-07-08 19:14:07 PDT
Created attachment 387583 [details] [diff] [review]
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 48 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2009-07-08 19:53:44 PDT
Comment on attachment 387583 [details] [diff] [review]
alternative much smaller patch

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

nsIAtom *frameType = frame->GetType();


r=dbaron with that, I suppose
Comment 49 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2009-07-08 19:54:50 PDT
(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)?
Comment 50 Evgeny Burmentyev [:virus_found] 2009-07-09 00:38:57 PDT
(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.
Comment 51 Zack Weinberg (:zwol) 2009-07-09 17:21:09 PDT
Created attachment 387776 [details] [diff] [review]
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.
Comment 52 Zack Weinberg (:zwol) 2009-07-09 17:51:36 PDT
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").
Comment 53 Zack Weinberg (:zwol) 2009-07-09 17:52:15 PDT
Why can't we not place the scrollbar at all?
Comment 54 Zack Weinberg (:zwol) 2009-07-13 17:01:10 PDT
Created attachment 388372 [details] [diff] [review]
alt patch, rev 2: unnecessary nsCOMPtr removed

Per comment #48.

Do we have a maintainer for the Gtk theme code?
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-07-13 17:17:19 PDT
Michael Ventnor
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-07-28 03:13:04 PDT
Comment on attachment 388372 [details] [diff] [review]
alt patch, rev 2: unnecessary nsCOMPtr removed

dbaron already said r=dbaron with this change
Comment 57 Zack Weinberg (:zwol) 2009-07-28 08:54:31 PDT
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.
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-07-30 03:09:05 PDT
http://hg.mozilla.org/mozilla-central/rev/4e85941dfbb5
Comment 59 Alexander Sack 2009-09-30 03:00:49 PDT
this fixes our themes with GtkRange::trough-border != zero
Comment 60 Alexander Sack 2009-09-30 03:04:38 PDT
Comment on attachment 388372 [details] [diff] [review]
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.
Comment 61 Zack Weinberg (:zwol) 2009-09-30 08:29:20 PDT
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.
Comment 62 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2009-09-30 10:51:31 PDT
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.
Comment 63 Daniel Holbert [:dholbert] 2009-09-30 11:00:04 PDT
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.
Comment 64 Daniel Holbert [:dholbert] 2009-09-30 13:44:52 PDT
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://build.mozilla.org/tryserver-builds/dholbert@mozilla.com-try-b28226f038b0/

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?
Comment 65 Eric Appleman 2009-09-30 20:50:08 PDT
Yes, it is fixed.
Comment 66 Daniel Veditz [:dveditz] 2009-10-21 16:38:16 PDT
Comment on attachment 388372 [details] [diff] [review]
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.
Comment 67 Timothy Nikkel (:tnikkel) 2009-10-21 20:29:09 PDT
Created attachment 407691 [details] [diff] [review]
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.
Comment 68 Timothy Nikkel (:tnikkel) 2009-10-21 20:30:33 PDT
(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 69 Daniel Veditz [:dveditz] 2009-11-04 16:23:33 PST
Comment on attachment 407691 [details] [diff] [review]
rollup patch for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 70 Timothy Nikkel (:tnikkel) 2009-11-05 18:33:33 PST
Pushed the test to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/c724ba56c88c
Comment 71 Timothy Nikkel (:tnikkel) 2009-11-08 00:16:12 PST
Pushed the test to 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/489e7359206a
Comment 72 Timothy Nikkel (:tnikkel) 2009-11-09 01:03:11 PST
Pushed the rollup patch to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/27d9d4107522

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