Status

()

defect
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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.
Posted patch cleanup patchSplinter Review
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)
Attachment #146902 - Flags: superreview?(dbaron)
Attachment #146902 - Flags: review?(dbaron)
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
Attachment #146902 - Flags: superreview?(dbaron)
Attachment #146902 - Flags: superreview+
Attachment #146902 - Flags: review?(dbaron)
Attachment #146902 - Flags: review+
Posted patch progressSplinter Review
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.
Attachment #148553 - Flags: superreview?(dbaron)
Attachment #148553 - Flags: review?(dbaron)
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...
Attachment #148553 - Flags: superreview?(dbaron)
Attachment #148553 - Flags: superreview+
Attachment #148553 - Flags: review?(dbaron)
Attachment #148553 - Flags: review+
I think that ultimately there will be very little duplicated code; the reflow
strategy will end up being quite different.
See, I'm hoping that refactoring reflow would mean it could be more similar...
Posted patch more progressSplinter Review
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 :-)
Attachment #152736 - Flags: superreview?(dbaron)
Attachment #152736 - Flags: review?(dbaron)
Attachment #152736 - Flags: superreview?(dbaron)
Attachment #152736 - Flags: superreview+
Attachment #152736 - Flags: review?(dbaron)
Attachment #152736 - Flags: review+

Updated

15 years ago
Blocks: 252350

Updated

15 years ago
Blocks: 233869

Comment 18

15 years ago
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. ***
Posted patch fix (obsolete) — Splinter Review
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.
Attachment #179693 - Flags: superreview?(dbaron)
Attachment #179693 - Flags: review?(dbaron)
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.
Attachment #179693 - Flags: superreview?(dbaron)
Attachment #179693 - Flags: review?(dbaron)
Posted patch updated fix (obsolete) — Splinter Review
This does fix lots of bugs and I think we should take it for 1.8b2 if we
possibly can.
Attachment #179693 - Attachment is obsolete: true
Attachment #179984 - Flags: superreview?(dbaron)
Attachment #179984 - Flags: review?(dbaron)
Comment on attachment 179984 [details] [diff] [review]
updated fix

oops, two more unwanted hunks.
Attachment #179984 - Flags: superreview?(dbaron)
Attachment #179984 - Flags: review?(dbaron)
Posted patch updated^2 fixSplinter Review
hopefully that's it :-)
Attachment #179984 - Attachment is obsolete: true
Attachment #179988 - Flags: superreview?(dbaron)
Attachment #179988 - Flags: review?(dbaron)
Flags: blocking1.8b2?

Updated

14 years ago
Flags: blocking1.8b2? → blocking1.8b2+
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
Attachment #179988 - Flags: superreview?(dbaron)
Attachment #179988 - Flags: superreview+
Attachment #179988 - Flags: review?(dbaron)
Attachment #179988 - Flags: review+
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
Attachment #179988 - Flags: approval1.8b2?
Comment on attachment 179988 [details] [diff] [review]
updated^2 fix

a=asa
Attachment #179988 - Flags: approval1.8b2? → approval1.8b2+
checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 277820
Blocks: 287859
Blocks: 264872
Blocks: 264866
Blocks: 262437
Blocks: 256282
Blocks: 254746

Comment 31

14 years ago
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?

Comment 34

14 years ago
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 )
Blocks: 262070
Blocks: 261376
Blocks: 274463
Blocks: 263687
Depends on: 292311
Depends on: 292326
Blocks: 292078

Updated

14 years ago
Blocks: 292371

Updated

14 years ago
Blocks: 238998

Updated

14 years ago
Blocks: 292431

Comment 35

14 years ago
bug 292431 filed for the regression mentioned in comment 32
No longer blocks: 292431

Updated

14 years ago
Blocks: 292549

Updated

14 years ago
Blocks: 292656
This appears to have caused bug 292657
Depends on: 293761
Depends on: 292690

Updated

14 years ago
Blocks: 295292
Depends on: 295459

Updated

14 years ago
Blocks: 295571

Updated

14 years ago
Blocks: 295814
The checkin for this bug is the prime suspect for causing bug 306349 (I think).
Depends on: 295815
Depends on: 306349

Updated

14 years ago
Depends on: 305970

Comment 38

14 years ago
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).

Updated

14 years ago
Depends on: 317105
Could this have caused bug 321799?
Depends on: 321799
Depends on: 325486
You need to log in before you can comment on or make changes to this bug.