registry team mailing list archive

Thread
Date
[Bug 327863] Re: non-zero GtkRange::trough-border value produces strange boxes in Firefox

To: registry@xxxxxxxxxxxxxxxxxxx
From: Bug Watch Updater <327863@xxxxxxxxxxxxxxxxxx>
Date: Sat, 18 Sep 2010 00:16:54 -0000
Reply-to: Bug 327863 <327863@xxxxxxxxxxxxxxxxxx>
Sender: bounces@xxxxxxxxxxxxx
Launchpad has imported 73 comments from the remote bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=486065.

If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.

------------------------------------------------------------------------
On 2009-03-31T04:27:44+00:00 Zack Weinberg wrote:

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 |

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/3

------------------------------------------------------------------------
On 2009-03-31T04:31:03+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/4

------------------------------------------------------------------------
On 2009-04-05T09:12:09+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/5

------------------------------------------------------------------------
On 2009-04-05T09:15:02+00:00 Tnikkel wrote:

Created attachment 371116
testcase

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/6

------------------------------------------------------------------------
On 2009-04-05T09:15:37+00:00 Tnikkel wrote:

Created attachment 371117
what the testcase looks like with this bug

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/7

------------------------------------------------------------------------
On 2009-04-05T21:59:19+00:00 Roc-ocallahan wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/8

------------------------------------------------------------------------
On 2009-04-07T09:00:09+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/9

------------------------------------------------------------------------
On 2009-04-08T04:19:15+00:00 Tnikkel wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/10

------------------------------------------------------------------------
On 2009-04-08T06:25:46+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/11

------------------------------------------------------------------------
On 2009-04-08T10:36:10+00:00 Roc-ocallahan wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/12

------------------------------------------------------------------------
On 2009-04-08T19:47:13+00:00 Tnikkel wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/13

------------------------------------------------------------------------
On 2009-04-08T20:07:06+00:00 Roc-ocallahan wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/14

------------------------------------------------------------------------
On 2009-04-08T20:53:17+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/15

------------------------------------------------------------------------
On 2009-04-08T21:43:06+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/16

------------------------------------------------------------------------
On 2009-04-08T22:58:37+00:00 Tnikkel wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/17

------------------------------------------------------------------------
On 2009-04-08T23:26:53+00:00 Roc-ocallahan wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/18

------------------------------------------------------------------------
On 2009-04-08T23:52:26+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/19

------------------------------------------------------------------------
On 2009-04-09T00:00:25+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/20

------------------------------------------------------------------------
On 2009-04-09T00:21:36+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/21

------------------------------------------------------------------------
On 2009-04-09T01:15:00+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/22

------------------------------------------------------------------------
On 2009-04-09T02:22:07+00:00 Roc-ocallahan wrote:

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?

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/23

------------------------------------------------------------------------
On 2009-04-10T19:26:15+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/24

------------------------------------------------------------------------
On 2009-04-11T02:08:25+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/25

------------------------------------------------------------------------
On 2009-04-11T02:18:53+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/26

------------------------------------------------------------------------
On 2009-04-13T05:43:51+00:00 Adrianm2 wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/27

------------------------------------------------------------------------
On 2009-04-13T09:15:14+00:00 Roc-ocallahan wrote:

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

I'll take this on general cleanup

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/28

------------------------------------------------------------------------
On 2009-04-13T09:20:51+00:00 Roc-ocallahan wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/29

------------------------------------------------------------------------
On 2009-04-13T09:34:09+00:00 Roc-ocallahan wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/30

------------------------------------------------------------------------
On 2009-04-13T16:50:05+00:00 Zack Weinberg wrote:

(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?

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/31

------------------------------------------------------------------------
On 2009-04-13T20:54:55+00:00 Roc-ocallahan wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/32

------------------------------------------------------------------------
On 2009-04-16T00:04:28+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/33

------------------------------------------------------------------------
On 2009-04-16T00:15:54+00:00 Roc-ocallahan wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/34

------------------------------------------------------------------------
On 2009-05-05T23:12:32+00:00 L. David Baron wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/35

------------------------------------------------------------------------
On 2009-05-05T23:22:25+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/36

------------------------------------------------------------------------
On 2009-05-12T03:59:47+00:00 T-artem wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/37

------------------------------------------------------------------------
On 2009-05-12T04:28:30+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/38

------------------------------------------------------------------------
On 2009-05-12T05:42:25+00:00 T-artem wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/39

------------------------------------------------------------------------
On 2009-05-12T05:56:35+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/40

------------------------------------------------------------------------
On 2009-05-12T08:15:52+00:00 T-artem wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/41

------------------------------------------------------------------------
On 2009-05-12T15:31:59+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/42

------------------------------------------------------------------------
On 2009-05-15T21:09:37+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/43

------------------------------------------------------------------------
On 2009-05-15T22:06:22+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/44

------------------------------------------------------------------------
On 2009-05-24T15:44:18+00:00 Wuno wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/45

------------------------------------------------------------------------
On 2009-05-31T10:14:34+00:00 Bugzilla-dolphinling wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/46

------------------------------------------------------------------------
On 2009-07-08T21:38:00+00:00 L. David Baron wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/49

------------------------------------------------------------------------
On 2009-07-08T21:44:08+00:00 L. David Baron wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/50

------------------------------------------------------------------------
On 2009-07-08T22:28:40+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/51

------------------------------------------------------------------------
On 2009-07-09T02:14:07+00:00 Zack Weinberg wrote:

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?

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/52

------------------------------------------------------------------------
On 2009-07-09T02:53:44+00:00 L. David Baron wrote:

Comment on attachment 387583
alternative much smaller patch

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

nsIAtom *frameType = frame->GetType();


r=dbaron with that, I suppose

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/53

------------------------------------------------------------------------
On 2009-07-09T02:54:50+00:00 L. David Baron wrote:

(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)?

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/54

------------------------------------------------------------------------
On 2009-07-09T07:38:57+00:00 Evgeny Burmentyev wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/55

------------------------------------------------------------------------
On 2009-07-10T00:21:09+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/56

------------------------------------------------------------------------
On 2009-07-10T00:51:36+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/57

------------------------------------------------------------------------
On 2009-07-10T00:52:15+00:00 Zack Weinberg wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/58

------------------------------------------------------------------------
On 2009-07-14T00:01:10+00:00 Zack Weinberg wrote:

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

Per comment #48.

Do we have a maintainer for the Gtk theme code?

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/60

------------------------------------------------------------------------
On 2009-07-14T00:17:19+00:00 Roc-ocallahan wrote:

Michael Ventnor

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/61

------------------------------------------------------------------------
On 2009-07-28T10:13:04+00:00 Roc-ocallahan wrote:

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

dbaron already said r=dbaron with this change

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/62

------------------------------------------------------------------------
On 2009-07-28T15:54:31+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/63

------------------------------------------------------------------------
On 2009-07-30T10:09:05+00:00 Roc-ocallahan wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/64

------------------------------------------------------------------------
On 2009-09-30T10:00:49+00:00 Alexander Sack wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/72

------------------------------------------------------------------------
On 2009-09-30T10:04:38+00:00 Alexander Sack wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/73

------------------------------------------------------------------------
On 2009-09-30T15:29:20+00:00 Zack Weinberg wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/74

------------------------------------------------------------------------
On 2009-09-30T17:51:31+00:00 L. David Baron wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/75

------------------------------------------------------------------------
On 2009-09-30T18:00:04+00:00 Daniel Holbert wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/76

------------------------------------------------------------------------
On 2009-09-30T20:44:52+00:00 Daniel Holbert wrote:

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@xxxxxxxxxxx-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?

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/77

------------------------------------------------------------------------
On 2009-10-01T03:50:08+00:00 Eric Appleman wrote:

Yes, it is fixed.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/78

------------------------------------------------------------------------
On 2009-10-21T23:38:16+00:00 Dveditz wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/79

------------------------------------------------------------------------
On 2009-10-22T03:29:09+00:00 Tnikkel wrote:

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.

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/80

------------------------------------------------------------------------
On 2009-10-22T03:30:33+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/81

------------------------------------------------------------------------
On 2009-11-05T00:23:33+00:00 Dveditz wrote:

Comment on attachment 407691
rollup patch for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/82

------------------------------------------------------------------------
On 2009-11-06T02:33:33+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/83

------------------------------------------------------------------------
On 2009-11-08T08:16:12+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/84

------------------------------------------------------------------------
On 2009-11-09T09:03:11+00:00 Tnikkel wrote:

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

Reply at: https://bugs.launchpad.net/firefox/+bug/327863/comments/85


** Changed in: firefox
   Importance: Unknown => Medium

-- 
non-zero GtkRange::trough-border value produces strange boxes in Firefox
https://bugs.launchpad.net/bugs/327863
You received this bug notification because you are a member of Registry
Administrators, which is the registrant for GTK-engines.