Last Comment Bug 197581 - need to recover overflow area properly during incremental reflow
: need to recover overflow area properly during incremental reflow
[a=rjesup,sspitzer for 1.4 final]
: fixed1.4
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: All All
P1 normal (vote)
: mozilla1.4final
Assigned To: David Baron :dbaron: ⌚️UTC-8
: Nobody; OK to take it and work on it
: Jet Villegas (:jet)
Depends on:
Blocks: 96670 173277 188153 197311 201066 205165
  Show dependency treegraph
Reported: 2003-03-15 11:00 PST by David Baron :dbaron: ⌚️UTC-8
Modified: 2008-01-13 13:28 PST (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.99 KB, patch)
2003-05-07 11:44 PDT, Bernd
no flags Details | Diff | Splinter Review
revised patch (11.13 KB, patch)
2003-05-15 21:39 PDT, Bernd
kinmoz: review+
dbaron: superreview+
rjesup: approval1.4+
Details | Diff | Splinter Review

Description User image David Baron :dbaron: ⌚️UTC-8 2003-03-15 11:00:32 PST
We have problems where we don't recover the overflow area properly during
incremental reflow.  (Bernd mentioned problems with tables, and I'd be surprised
if we don't have others.)

roc had some progress on fixing this in bug 170330, but that's not a complete
fix.  We really need exact recovery of the overflow area so that 'overflow:
auto' and 'overflow: scroll' work correctly.  One possibility is storing the
overflow area on some frames.  Another possibility is recovering it with the
help of the NS_FRAME_OUTSIDE_CHILDREN bit, which should be effectively as fast
for the normal case where most frames don't overflow.
Comment 1 User image John Keiser (jkeiser) 2003-04-08 22:37:29 PDT
Since even an ordinary div could overflow if it is set to a fixed width bigger
than its parent div (so that pretty much all frames *could* overflow), we'd have
to store these 4 nscoords in every frame.  However, we might be able to avoid
that with a hashtable in presshell.  If < 5% of frames have overflow areas that
would avoid most of the bloat that could arise from such a plan.

The problem with tables as bernd explained it is that they don't re-reflow their
children all the time (an optimization) and so miss setting the overflow area
bit sometimes since that information only comes down through reflow.
Comment 2 User image Brendan Eich [:brendan] 2003-04-08 22:57:40 PDT
"re-reflow" -- *shudder*.


Comment 3 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2003-04-09 00:11:05 PDT
> However, we might be able to avoid that with a hashtable in presshell

The frame manager lets you get and set frame properties which are implemented
this way, so this part is easy.

I think exact recovery of overflow area in incremental reflow is just a matter
of fixing bugs here and there, although it may be that by just storing it, we
would simplify our code significantly.
Comment 4 User image David Baron :dbaron: ⌚️UTC-8 2003-04-09 06:23:37 PDT
We may not need to store anything that we're not storing now.  Having overflow
is an unusual case, and we can just descend into any frames that have
Comment 5 User image Bernd 2003-04-30 21:49:29 PDT
I looked at
Couldn't we use that mechanism and copy the GetOverflowAreaProperty routine to
nsFrame.cpp. Is there anything wrong with that? If not, I would use this
mechanism to propagate the overflow area inside the table frames to verify that
it works.
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2003-05-01 05:15:02 PDT
Yeah, we could use that.
Comment 7 User image Bernd 2003-05-07 11:44:32 PDT
Created attachment 122683 [details] [diff] [review]
Comment 8 User image Bernd 2003-05-11 10:27:16 PDT
Comment on attachment 122683 [details] [diff] [review]

I believe this must work for a nsIFrame*. The implementation in
nsAbsoluteContainingBlock needs to removed otherwise there will be two
destructor functions for the same property.
Comment 9 User image Bernd 2003-05-11 10:38:59 PDT
stealing the bug
Comment 10 User image Bernd 2003-05-15 21:39:14 PDT
Created attachment 123477 [details] [diff] [review]
revised patch
Comment 11 User image kinmoz 2003-05-22 22:15:48 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

FYI, bernd's patch here allows us to fix bug 205165 rather easily. You can see
the small tweak I made in bug 205165 comment 31.
Comment 12 User image kinmoz 2003-05-28 13:17:09 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

These changes look ok to me,, just a few nit-picks.

====  Capitalize the 'c' in "create" to be consistent with other comments in
the  file:

+  /** create or retrieve the previously stored overflow area, if the frame
+    * not overflow and no creation is required return nsnull.

==== Not a big deal, but I'll ask anyways, in addition to unsetting the
NS_FRAME_OUTSIDE_CHILDREN bit, should |nsFrame::StoreOverflow()| also remove
any existing overflowAreaProperty so it can delete it's associated rect? I know
it will eventually get deleted, but I'm just thinking about dhtml and composer.

==== Capitalize the 'r' in "return", get rid of one of the spaces between
"overflow  and" and put a '.' at the end of the comment:

+  // return the previously stored overflow area, if the frame does not 
+  // overflow	and a creation is not requested it will return nsnull

==== I'd probably rework this comment:

+  // if necessary set the NS_FRAME_OUTSIDE_CHILDREN flag and store the
+  // area via the frame manager, by this it can be retrieved later without 
+  // reflowing the frame

Maybe something like this:

+  // Set/unset the NS_FRAME_OUTSIDE_CHILDREN flag and store the overflow area
+  // as a frame property in the frame manager so that it can be retrieved
+  // without reflowing the frame.
Comment 13 User image kinmoz 2003-05-28 14:23:12 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

Oh yeah, and remove the 2 blank lines this change leaves at the end of this

@@ -1542,17 +1526,7 @@

   ComputeCombinedArea(aReflowState, aMetrics);

-  // If the combined area of our children exceeds our bounding box
-  // then set the NS_FRAME_OUTSIDE_CHILDREN flag, otherwise clear it.
-  if ((aMetrics.mOverflowArea.x < 0) ||
-      (aMetrics.mOverflowArea.y < 0) ||
-      (aMetrics.mOverflowArea.XMost() > aMetrics.width) ||
-      (aMetrics.mOverflowArea.YMost() > aMetrics.height)) {
-  }
-  else {
-  }
Comment 14 User image John Keiser (jkeiser) 2003-05-29 12:13:22 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

dbaron, could you sr?  We need this for bug 205165 and all Kin's comments
except one were purely syntactical.
Comment 15 User image John Keiser (jkeiser) 2003-05-29 12:43:45 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

I spoke too soon, bernd says there *may* be a problem with the patch causing
empty rect invalidates--unsure whether this patch is the cause yet.
Comment 16 User image kinmoz 2003-05-29 13:10:51 PDT
I just used one of my debug builds, without bernd's patch, to go to the test
page he was using and I see the same empty invalidate rect warnings in my
console, so they happen even without his patch.
Comment 17 User image Bernd 2003-05-31 06:03:24 PDT
fix checked in
Comment 18 User image Bernd 2003-05-31 12:27:57 PDT
giving the bug back to David. The patch that I checked in is only the
infrastructure for the real fix. Bug 173277 covers the overflow area recovery
for table frames. I don't know block and line layout good enough to fix the
incr. reflow problems there.
Comment 19 User image John Keiser (jkeiser) 2003-06-02 10:21:40 PDT
We should aim this patch at 1.4 after it bakes a few days ... bug 205165 depends
on it.
Comment 20 User image Samir Gehani 2003-06-06 10:57:34 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

Seeking approval from drivers for kin to land this on the 1.4 branch since
Netscape is very interested in taking bug 205165, an nsbeta1+/adt1 which
depends on this fix landing on the 1.4 branch.	Thanks.
Comment 21 User image Randell Jesup [:jesup] 2003-06-09 11:57:18 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

Please mark with fixed1.4 when checked into 1.4 branch.
Comment 22 User image kinmoz 2003-06-09 15:48:41 PDT
Fix checked into the MOZILLA_1_4_BRANCH:

  mozilla/layout/html/base/src/nsFrame.h                      revision
  mozilla/layout/html/base/src/nsFrame.cpp                    revision 3.429.2.1
  mozilla/layout/html/base/src/nsBlockFrame.cpp               revision 3.572.2.2
  mozilla/layout/html/base/src/nsAbsoluteContainingBlock.cpp  revision
  mozilla/layout/base/public/nsIFrame.h                       revision
Comment 23 User image Markus Hübner 2003-08-10 10:29:42 PDT
Can't we mark this one fixed?
Comment 24 User image David Baron :dbaron: ⌚️UTC-8 2003-08-10 12:02:53 PDT
No.  It's not fixed.
Comment 25 User image Markus Hübner 2004-08-12 01:47:07 PDT
How does this interact with bug 170330 ?
Comment 26 User image Bernd 2008-01-13 08:20:41 PST
David, isn't this bug obsolete after the landing of the reflow branch?
Comment 27 User image David Baron :dbaron: ⌚️UTC-8 2008-01-13 10:03:25 PST
Not reflow branch, but I think roc fixed it at a different point... I should find the bug.

Note You need to log in before you can comment on or make changes to this bug.