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.

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

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 on attachment 140485 [details] [diff] [review] fix bz, are you reviewing nsGfxScrollFrame these days?

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

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 on attachment 140487 [details] [diff] [review] revision OK, I'll try dbaron then :-)

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

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.

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

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 on attachment 140487 [details] [diff] [review] revision layout regression, needs 1.7b exposure

Comment on attachment 140487 [details] [diff] [review] revision a=chofmann for 1.7b

checked in.

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?

Yes, I think so.

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 on attachment 143603 [details] [diff] [review] get rid of annoying assertions kill bogus assertions

checked in

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.

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

Have you noticed this on any other pages?

Not yet.

Brian, are you still seeing this?

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

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

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

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

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.

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

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.

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.