240276 - Reorganize nsGfxScrollFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

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.

Comment 1

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

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.

Comment 2

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

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.

Comment 3

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

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.

Comment 4

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

Attachment: cleanup patch

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 5

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

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 6

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

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

Comment 7

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

Roger that. Thanks.

Comment 8

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

checked in.

Comment 9

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

Attachment: progress

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 10

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

Comment on attachment 148553 [details] [diff] [review]
progress

suitable for checkin.

Comment 11

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

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

Comment 12

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

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

Comment 13

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

checked that in.

Comment 14

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

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

Comment 15

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

Attachment: more progress

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 16

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

Comment on attachment 152736 [details] [diff] [review]
more progress

A reasonably safe fix, as modifying XUL goes :-)

Comment 17

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

checked that in.

Comment 18

[Steffen Wilberg]

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

Comment 19

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

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

Comment 20

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

Attachment: fix

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 21

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

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.

Comment 22

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

Attachment: updated fix

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

Comment 23

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

Comment on attachment 179984 [details] [diff] [review]
updated fix

oops, two more unwanted hunks.

Comment 24

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

Attachment: updated^2 fix

hopefully that's it :-)

Comment 25

[Asa Dotzler [:asa]]

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

Comment 26

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

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

Comment 27

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

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 28

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

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 29

[Asa Dotzler [:asa]]

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

a=asa

Comment 30

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

checked in

Comment 31

[Jim Ginn]

Could this have caused Bug 292299?

Comment 32

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

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

Comment 33

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

Jim: probably. Related to 292286?

Comment 34

[Markus Hübner]

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 )

Comment 35

[José Jeria]

bug 292431 filed for the regression mentioned in comment 32

Comment 36

[Crispin Flowerday (not receiving bug mail)]

This appears to have caused bug 292657

Comment 37

[Uri Bernstein]

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

Comment 38

[Ilya Konstantinov]

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

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Could this have caused bug 321799?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Yes.