232838 - too-short scrollbars cause scrollport to be sized incorrectly

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

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

Followup from the bug bz uncovered in bug 232469. It looks like whenever a
scrollbar is too short, the layout of the scrollport screws up so that it
appears to grow to include the scrollbar.

Comment 1

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

Attachment: testcase

This illustrates the bug. Use a frame dump to see that the scrollport frame is
incorrectly sized.

Comment 2

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

Attachment: fix

The bug was a regression from the checkin for bug 210269, which removed the
setting of the 'collapsed' attribute on scrollbars. When a vertical or
horizontal scrollbar was too small to fit, we were trying to remove it. To
fully remove it, we have to now call LayoutBox on it again with a zero width
(or height), and we weren't doing this (I think we were relying on the setting
of 'collapsed' to trigger another reflow). This patch does that.

This patch also fixes an issue where if the horizontal scrollbar is too small
and is removed, the vertical scrollbar may need to grow. Again this was
probably handled before by another induced reflow.

This scrolling code is a bit nasty. It's unfortunate that in some cases we may
reflow the scrolled area three times.

Comment 3

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

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

bz, are you reviewing nsGfxScrollFrame these days?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Probably, but I may not be able to get to this in time for 1.7a... I've got a
bunch of reviews already on my plate.  :(

Comment 5

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

Attachment: revision

Never mind, here's a better fix. This is a bit more aggressive and logical
about doing the right thing. See the big comment. By checking minsize up front,
we can avoid laying out any scrollbar more than once AND we can ensure that if
there's only room for exactly one scrollbar, we will show the vertical
scrollbar.

Comment 6

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

Comment on attachment 140487 [details] [diff] [review]
revision

OK, I'll try dbaron then :-)

Comment 7

[Robert Kaiser]

roc, to not file another bug, I'll ask here: Will that fix the inconsistent
behaviour I have on http://com.naddy.at/?d=b&m=i (sorry, no minimized case, it
disappeared when I tried to do a minimized testcase)?
What I'm seeing is a difference of layout of the main area in the middle (has
overflow:auto set, and contains a 100% width table) between first layout (call
URL via link or URL bar) and reloading (pressing reload on the browser).
At first, right and left borders are equal (table fills whole div). At reload,
right border is greater, as between the table and div border space for the
not-shown scrollbar is reserved.
Will this change to a consistent behaviour with this patch or should I file a
seperate bug?
(btw, what I'd really need for that site to behave correctly would be
overflow-x:auto;overflow-y:visible; - but that's CSS3 and not yet supported)

Comment 8

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

Comment on attachment 140487 [details] [diff] [review]
revision

>+   Our basic strategy here is to first try laying out the content with
>+   the scrollbars in their current state. We're hoping that that will

What's the current state in the first layout?  (I guess I'd hope it's that the
scrollbars will be there, since reflowing will generally be cheaper if they
need to be removed (less stuff to reflow).)

Comment 9

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

kairo: not sure

dbaron: actually I think they're not there initially. This part is the same as
our current behaviour. I'm not sure it's unreasonable.

Comment 10

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

I think this is a layout regression, and the patch is nontrivial, so I think it
needs 1.7b exposure.

Comment 11

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

Comment on attachment 140487 [details] [diff] [review]
revision

r+sr=dbaron.  I may try to comment on the patch at a later date, but for now
we're probably better off just getting it in so it gets tested.

Comment 12

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

Comment on attachment 140487 [details] [diff] [review]
revision

layout regression, needs 1.7b exposure

Comment 13

[chris hofmann]

Comment on attachment 140487 [details] [diff] [review]
revision

a=chofmann for 1.7b

Comment 14

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

checked in.

Comment 15

[tenthumbs]

In my Linux gtk1 trunk build from this morning I'm getting a slew of these

###!!! ASSERTION: horizontal scrollbar can change height: 'hMinSize.height == 
hSize.height', file nsGfxScrollFrame.cpp, line 1506
Break: at file nsGfxScrollFrame.cpp, line 1506

###!!! ASSERTION: vertical scrollbar can change width: 'vMinSize.width == 
vSize.width', file nsGfxScrollFrame.cpp, line 1505
Break: at file nsGfxScrollFrame.cpp, line 1505

on almost all pages. One bonsai page generated 14. Is this due to this 
checkin?

Comment 16

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

Yes, I think so.

Comment 17

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

Attachment: get rid of annoying assertions

We shouldn't be using hMinSize.height or vMinSize.width at all in ::Layout()
(because other functions like AddRemoveScrollbar assume we always give
horizontal scrollbars their preferred height, etc).

Arguably we should be allowing scrollbars to shrink to their minimum width in
some circumstances. That'd be future work.

Comment 18

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

Comment on attachment 143603 [details] [diff] [review]
get rid of annoying assertions

kill bogus assertions

Comment 19

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

checked in

Comment 20

[Brian Ryner (not reading)]

I think this checkin (the large patch, not the assertion removal) caused some
funny text wrapping on the bonsai query page.  Oddly, it's not consistent, I
sometimes have to reload or shift+reload to see the problem, but I can't
reproduce it at all with this backed out.  I'll attach a screen shot to
demonstrate what I'm talking about.

Comment 21

[Brian Ryner (not reading)]

Attachment: funny wrapping

Comment 22

[Brian Ryner (not reading)]

Note that the wrapping problem is unaffected by changing the width of the window.

Comment 23

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

Have you noticed this on any other pages?

Comment 24

[Brian Ryner (not reading)]

Not yet.

Comment 25

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

Brian, are you still seeing this?

Comment 26

[Andrew Schultz]

the bug bryner mentioned was simply exposed by this checkin.  see bug 238472

Comment 27

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

OK, then I'll close this bug. Thanks!!!!!

Comment 28

[Hideo Saito]

Attachment: patch for funny wrapping

> Created an attachment (id=143915)
> funny wrapping

I can see the same problem using a build of latest trunk, however the following
patch has an effect for this problem. Can it check?

Comment 29

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

Hideo, which testcase has the problem that you're seeing? I'm not seeing it...

Comment 30

[Hideo Saito]

Attachment: testcase for flushing error

Comment 31

[Hideo Saito]

Attachment: patch for flushing error

Sorry, I made a mistake. I checked on own build on PowerPC/Linux using released
mozilla 1.7b, but the patch had without effect using same build on WinXP.

I posted a new patch that has an effect using the build on WInXP.
I think that this problem is regression of attachment 131205 [details] [diff] [review] of Bug 210269.

Comment 32

[Hideo Saito]

Attachment: Screen Shot

This patch has an effect for a problem of funny wrapping.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146918 [details] [diff] [review]
patch for flushing error

This patch is wrong.  I can see possible arguments for not flushing at all, but
there is _certainly_ no need to flush at the point where you moved that code
to.

Comment 34

[Hideo Saito]

In the case of a problem of "funny wrapping", when measuring the contents of the
first cell, the contents of the first cell were short like the following texts.

Modify Query
Mail everyone on this page (12 people)

In the case of "testcase for flushing error", when measuring the contents of the
first cell, the contents of the second cell were empty.