379635 - with view source, when I have a horizontal scrollbar, the vertical scrollbar is "too short"

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(not reading, please use [e-mail] instead)]

with view source, when I have a horizontal scrollbar, the vertical scrollbar is "too short".

here comes two screen shots to show what I mean.

I'm using my own trunk, debug Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a5pre) Gecko/20070502 Minefield/3.0a5pre

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: screen shot (no horizontal scrollbar)

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: screen shot (with horizontal scrollbar) and the problem

Comment 3

[Brian Crowder]

For me, it is the vertical scrollbar whose location is wildly off.

Comment 4

[(not reading, please use [e-mail] instead)]

crowder:  to clarify:  for mac, too?  and, for view source (or another window?)

Comment 5

[Brian Crowder]

Yes, Mac too, but not only the view source window.  This happens to me in every window with horizontal scroll.

Comment 6

[Colin Barrett [:cbarrett]]

STR or testcase? I can't reproduce this.

Comment 7

[Brian Crowder]

It happens to me with a build from an hour or so ago while looking at many bugzilla bugs, even just https://bugzilla.mozilla.org/.  The characteristics change sometimes, if I resize the window, also the dropdown for "Target Milestone" on this page is very very messed up, too.  The down-arrow "button" is hundreds of pixels wide.

Comment 8

[(not reading, please use [e-mail] instead)]

> The down-arrow "button" is hundreds of pixels wide.

I had never seen that, until just now.  screen shot coming.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 10

[Justin Dolske [:Dolske]]

I'm also seeing this, usually the vertical scroll bar. It seems to happen more frequently when the browser has been running for a while. First started for me about a week ago.

usually I get a blank window with the scrollbar on the left.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suspect a problem with scrollbar metrics. The dropdown arrow width is defined in terms of scrollbar metrics. Also, too-wide vertical scrollbar metrics would produce exactly the misplaced scrollbar that I'm seeing. The erraticness suggests problems with uninitialized variables.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is the sizing of NS_THEME_SCROLLBAR_THUMB_VERTICAL (or HORIZONTAL, I guess). Just after the call to ::HIThemeGetTrackPartBounds(&tdi, kControlIndicatorPart, &thumbRect), I have the following result:

(gdb) p thumbRect
$12 = {
  origin = {
    x = 0, 
    y = 3.91198254
  }, 
  size = {
    width = 4074.08203, 
    height = 9.80908925e-45
  }
}

Looks like uninitialized garbage to me! It may have failed because the scrollbar frame has not yet been reflowed:

(gdb) p scrollbarFrame->mRect
$14 = {
  x = 0, 
  y = 0, 
  width = 0, 
  height = 0
}

Here's the corresponding tdi:

(gdb)  p tdi
$15 = {
  version = 0, 
  kind = 0, 
  bounds = {
    origin = {
      x = 0, 
      y = 0
    }, 
    size = {
      width = 0, 
      height = 0
    }
  }, 
  min = 0, 
  max = 0, 
  value = 0, 
  reserved = 1465171967, 
  attributes = 4, 
  enableState = 0 '\000', 
  filler1 = 0 '\000', 
  trackInfo = {
    scrollbar = {
      viewsize = 0, 
      pressState = 56 '8'
    }, 
    slider = {
      thumbDir = 0 '\000', 
      pressState = 0 '\000'
    }, 
    progress = {
      phase = 0 '\000'
    }
  }
}

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it for me.

Comment 15

[Jesse Ruderman]

You could avoid doing scrollbarFrame->GetRect() twice by moving your new code down one line and using the scrollbarRect variable.

Comment 16

[Colin Barrett [:cbarrett]]

Comment on attachment 264817 [details] [diff] [review]
fix

Jesse's suggestion is correct. r=cbarrett with that change.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 264817 [details] [diff] [review]
fix

Stealing the sr so we can unbustimicate the tree.  As per IRC, no need to call SizeTo(0,0) there, as we've already done so at the beginning of the method.  sr=dmose with that change.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 19

[Mats Palmgren (inactive)]

But we don't want GetPrefSize and friends to be dependent on reflow, right?
With the current fix we get a zero min-width before reflow and something
else after.  It seems to me that this could cause incremental reflow bugs
or at least unnecessary reflows?

FWIW, the stack leading up to the problem is:

nsSprocketLayout::Layout
 nsSprocketLayout::PopulateBoxSizes
  nsSliderFrame::GetPrefSize
   nsBoxFrame::GetPrefSize
    nsSprocketLayout::GetPrefSize
     nsBoxFrame::GetPrefSize
      nsBoxFrame::GetMinSize
       nsIFrame::AddCSSMinSize
        nsNativeThemeCocoa::GetMinimumWidgetSize
         scrollbarFrame->GetRect()

Shouldn't we use scrollbarFrame->GetMinSize() there instead?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #19)
> But we don't want GetPrefSize and friends to be dependent on reflow, right?

Right. But in this case we have no choice because the platform's desired size does depend on the height of the scrollbar. We *have* to return the correct value here or we will have bugs because the platform's scrollbar thumb isn't where our thumb frame is. Put it another way, the problem here is that Aqua doesn't obey our reflow rules...

> With the current fix we get a zero min-width before reflow and something
> else after.  It seems to me that this could cause incremental reflow bugs
> or at least unnecessary reflows?

Possibly. Hopefully the minimum width of the thumb doesn't really matter because the buttons will give us a non-varying minimum width anyway.

> FWIW, the stack leading up to the problem is:
> 
> nsSprocketLayout::Layout
>  nsSprocketLayout::PopulateBoxSizes
>   nsSliderFrame::GetPrefSize
>    nsBoxFrame::GetPrefSize
>     nsSprocketLayout::GetPrefSize
>      nsBoxFrame::GetPrefSize
>       nsBoxFrame::GetMinSize
>        nsIFrame::AddCSSMinSize
>         nsNativeThemeCocoa::GetMinimumWidgetSize
>          scrollbarFrame->GetRect()
> 
> Shouldn't we use scrollbarFrame->GetMinSize() there instead?

I don't see how that would help.

Maybe we should modify scrollbar reflow so that on Mac, at least, the thumb size is not taken into account when determining the pref or min size of the slider.

Comment 21

[Colin Barrett [:cbarrett]]

(In reply to comment #20)
> Maybe we should modify scrollbar reflow so that on Mac, at least, the thumb
> size is not taken into account when determining the pref or min size of the
> slider.

While you're thinking about changing the way scrollbars work on the Mac, bug 380185 is worth a look.