Bugs like bug 235558 are nearly impossible to fix while nsGfxScrollFrame depends on XUL box layout. For auto-sized width and height scrollframes in HTML, various chunks of HTML reflow state must be propagated from the scrollframe's reflow environment to the scrolled block for everything to work correctly. Right now we have some hacks to propagate some state. But it's hacky and doesn't always work. Furthermore, reasoning about the code, especially about its performance, is very difficult. Therefore I propose splitting nsGfxScrollFrame into two classes. nsHTMLScrollFrame will be used to scroll HTML block elements. It will not be a XUL box, just a regular HTML frame doing its reflow in Reflow(). nsXULScrollFrame will be used to scroll XUL elements. It will be a XUL box, doing its reflow in Layout(). This is a fairly high priority item for me since there are bugs I don't know how to fix without doing this, and other bugs (e.g., bug 240248) that, even if fixable, would be much easier to fix if we rip out the HTML reflow->XUL reflow->HTML reflow transitions, and the sooner we fix this right the less wasted work there will be.

I have a patch that refactors nsGfxScrollFrame, doing general cleanup and moving functionality from nsGfxScrollFrame into nsGfxScrollFrameInner, so that when we split into nsHTMLScrollFrame and nsXULScrollFrame they can share nsGfxScrollFrameInner and have less code duplication. I'll attach the patch when I have network access again.

Initially I plan to use nsXULScrollFrame whenever the parent frame is a XUL box or the scrolled frame is a XUL box. This will preserve the current behaviour for those cases. The basic reflow strategy nsHTMLScrollFrame should look like this: -- Guess whether we need vertical and/or horizontal scrollbars. There are a range of guessing strategies. -- Do the following in a loop: -- Position the scrolled frame based on whether a vertical scrollbar will be present and whether it's on the left or right. (Note: a horizontal scrollbar is never at the top so can't affect positioning of the scrolled frame.) -- Reflow scrolled frame, adjusting HTMLReflowState availWidth, computedWidth, computedMaxWidth for any width taken up by any vertical scrollbar, and similarly for any height constraints. -- Compute our desired width. It's the desired width of the scrolled frame, plus the width of any horizontal scrollbar, modulated by our width constraints (if any). -- Compare the overflow-area-XMost of the scrolled frame against our desired width minus the width of any vertical scrollbar. If it's greater, we need horizontal scrolling. Determine whether a horizontal scrollbar should be present. -- Compute our desired height. It's the desired height of the scrolled frame, plus the height of any horizontal scrollbar, modulated by our height constraints (if any). -- Compare the overflow-area-YMost of the scrolled frame against our desired height minus the height of any horizontal scrollbar. If it's greater, we need vertical scrolling. Determine whether a vertical scrollbar should be present. -- If our guesses were correct, stop the loop. -- Otherwise try all the above steps again, trying a new guess combination. (If we guessed one of the dimensions right, we don't change that guess, of course). If we find ourselves getting into an infinite loop by having to try an old guess again, just do one more iteration, favouring a guess that has a scrollbar in a given dimension over one that doesn't. -- Knowing our desired width and height and which scrollbars are present, place the scrollbar and scrollcorner frames and reflow them. -- The scrolled frame is already positioned and reflowed, we don't need to touch it.

The strategy for first guess will be "guess the same scrollbars as last time we were reflowed" for non-initial reflows. For initial reflows it's tricky. We should definitely use history state for reloads. When there's no history state, I think Safari guesses the scrollbar state of the last page loaded, for viewports. We could do that too. For other elements I have no clue. I think a three-iteration guessing strategy will work fine: -- (H1, V1) = first_guess(); -- (DH, DV) = what we actually ended up needing in the first iteration -- second guess is (DH, DV) -- (DH2, DV2) = what we actually ended up needing in the second iteration -- third guess is (DH || DH2, DV || DV2), and we just don't try again. I think we will just pass our reflow reason on down to the scrolled frame for its reflow, in the first iteration. Subsequent iterations ought to use a Resize reason.

This patch prepares for the nsHTMLScrollFrame/nsXULScrollFrame split by moving some functionality out of nsGfxScrollFrame to nsGfxScrollFrameInner where it can be shared by multiple scrollframe implementations, and doing general cleanup: -- remove unncecessary parameters from nsGfxScrollFrame() and GetScrolledView() -- make nsGfxScrollFrameInner a direct member -- remove unnecessary mPresContext

Comment on attachment 146902 [details] [diff] [review] cleanup patch Oh, this patch also removes unnecessary nsGfxScrollFrame::Paint override, and removes unused methods in nsIScrollableFrame (and their implementations)

Comment on attachment 146902 [details] [diff] [review] cleanup patch I don't like the AddRef in the nsGfxScrollFrameInner constructor, since we generally follow the rule of constructing with a refcount of zero. How about doing mInner.AddRef in nsGfxScrollFrame's ctor instead? ...and commenting there that it's never released because it's a member. But you need to do something so that it doesn't confuse the refcount logging. How about removing |aDocument| from NS_NewGfxScrollFrame too? with that, sr=dbaron

Roger that. Thanks.

checked in.

This patch makes further progress by splitting nsGfxScrollFrame into nsHTMLScrollFrame and nsXULScrollFrame (which currently do the same thing). In nsCSSFrameConstructor we select nsHTMLScrollFrame if the parent is not a XUL box.

Comment on attachment 148553 [details] [diff] [review] progress suitable for checkin.

Hmmm. I'm not crazy about forking this much code. I was hoping that the reflow architecture would improve so that we wouldn't need so many, if any, differences, but I guess this is probably the best thing to do for now...

I think that ultimately there will be very little duplicated code; the reflow strategy will end up being quite different.

checked that in.

See, I'm hoping that refactoring reflow would mean it could be more similar...

As I mentioned in the wiki, I need to get rid of nsScrollBoxFrame (and its subclass nsScrollPortFrame), for two main reasons: -- having it be a XUL box makes it impossible for HTML scrolling to avoid going through box layout, which is the big goal ... and forking it into nsHTMLScrollPortFrame and nsXULScrollPortFrame would be yucky -- it will be much simpler for nsHTMLScrollFrame to directly reflow the scrolled frame as its own child, rather than working indirectly through an intermediate frame. So I'm working on eliminating all dependencies on nsScrollBoxFrame. Ultimately an nsHTML/XULScrollFrame will have its scrolled frame as a direct child (i.e. sibling of the scrollbars and scrollcorner). The nsIScrollingView will be managed as an anonymous child view in much the same way that nsSubDocumentFrames manage their inner view. This patch starts breaking those nsScrollBoxFrame dependencies by changing the implementation of <scrollbox> from an nsScrollBoxFrame to a regular box frame which just happens to be 'overflow:hidden'. The boxObject API on <scrollbox> is modified so that instead of depending on nsScrollBoxFrame, we get the nsIScrollableFrame interface for the <scrollbox> scrollframe and work through GetScrollingView/GetScrolledFrame. This works now and will continue to work as the implementation of scrollframes evolves.

Comment on attachment 152736 [details] [diff] [review] more progress A reasonably safe fix, as modifying XUL goes :-)

checked that in.

toolkit/content/xul.css part of attachment 152736 [details] [diff] [review] landed again on trunk (bug 272561).

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

Here it is! This patch fixes all the testcases in the bugs depending on this bug, except for one case where I think it's actually an nsAbsoluteContainingBlock bug. Here's a guided tour of the patch: nsIScrollingView::ComputeScrollOffsets does nothing and is removed. I factored out some logic from nsBlockFrame into nsLayoutUtils::ApplyMinMaxConstraints so that nsHTMLScrollFrame could use it. Probably other frames could use it too. All the crufty hacks in nsFrame (ex nsBoxToBlockAdaptor) to propagate HTML reflow state through scrollframes are no longer needed, so I've removed them. I moved nsHTML/XULScrollFrame::GetDesiredScrollbarSizes into a shared method in nsGfxScrollFrameInner. We can't override GetPadding anymore to kill the padding on nsHTMLScrollFrame. So I hacked nsHTMLReflowState to detect when the padding should be eliminated. I can't think of a better way to do this, maybe someone else can. The guts of the new reflow logic are TryLayout and ReflowContents. Basically we try a number of layouts in order of preference, looking for one that's "logically consistent" (as defined in the comments for TryLayout). The XUL-specific reflow logic and state in nsGfxScrollFrameInner got moved to nsXULScrollFrame, but I shared as much as I could, which is basically all the code to actually position and reflow scrollbars. As described in the patch, scrollbar prefsizes are quite useless, and I shifted to using only the min-sizes with some debug code to check that the prefsize is consistent. The AdjustReflowStateForPrintPreview hack seems to not be needed anymore. We change the reflow reason to Resize for all but the first reflow of the scrolled content, and I think that takes care of this. I've decided not to post scrollport events for HTML frames.

Comment on attachment 179693 [details] [diff] [review] fix this patch has some stuff in it that I moved out to other bugs. I'll fix it up and update to trunk.

This does fix lots of bugs and I think we should take it for 1.8b2 if we possibly can.

Comment on attachment 179984 [details] [diff] [review] updated fix oops, two more unwanted hunks.

hopefully that's it :-)

ROC, is this something that absolutely needs to happen for 1.8b2?

It doesn't absolutely need to be in b2, but things are going to suck if it doesn't make FF 1.1.

Comment on attachment 179988 [details] [diff] [review] updated^2 fix I thought I was going to have time to really review this, but then I got dragged onto security firedrills. r+sr=dbaron

Comment on attachment 179988 [details] [diff] [review] updated^2 fix this fixes many reflow bugs involving scrolling and the 'overflow' property, but it will change the layout of some pages. the sooner it lands, the better

Comment on attachment 179988 [details] [diff] [review] updated^2 fix a=asa

checked in

Could this have caused Bug 292299?

This did cause a pretty serious Tdhtml regression ... about 15%. I'll look into it.

Jim: probably. Related to 292286?

Shouldn't we file a new bug about the DHTML regression? (I see actually 20% performance loss - http://build- graphs.mozilla.org/graph/query.cgi? testname=dhtml&tbox=luna.mozilla.org&autoscale=1&days=7&avg=1&showpoint=2005:04 :28:23:55:58,2063 )

bug 292431 filed for the regression mentioned in comment 32

This appears to have caused bug 292657

The checkin for this bug is the prime suspect for causing bug 306349 (I think).

roc, did you have a look at bug 305970? It seems to claim a regression caused by this patch (by narrowing down the time the regression started to the patch' commit time).

Could this have caused bug 321799?

Yes.