need to recover overflow area properly during incremental reflow

RESOLVED FIXED in mozilla1.4final

Status

()

Core
Layout: Misc Code
P1
normal
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({fixed1.4})

Trunk
mozilla1.4final
fixed1.4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [a=rjesup,sspitzer for 1.4 final])

Attachments

(1 attachment, 1 obsolete attachment)

11.13 KB, patch
kinmoz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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.

Updated

15 years ago
Blocks: 197311
(Assignee)

Updated

15 years ago
Blocks: 96670

Updated

14 years ago
Blocks: 201066

Comment 1

14 years ago
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.
"re-reflow" -- *shudder*.

;-)

/be
> 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.
(Assignee)

Comment 4

14 years ago
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
NS_FRAME_OUTSIDE_CHILDREN set.
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Future

Comment 5

14 years ago
I looked at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsAbsoluteContainingBlock.cpp#255
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.
Yeah, we could use that.

Comment 7

14 years ago
Created attachment 122683 [details] [diff] [review]
patch

Updated

14 years ago
Attachment #122683 - Flags: review?(roc+moz)

Comment 8

14 years ago
Comment on attachment 122683 [details] [diff] [review]
patch

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.
Attachment #122683 - Attachment is obsolete: true
Attachment #122683 - Flags: review?(roc+moz)

Comment 9

14 years ago
stealing the bug
Assignee: dbaron → bernd_mozilla
Status: ASSIGNED → NEW

Comment 10

14 years ago
Created attachment 123477 [details] [diff] [review]
revised patch

Updated

14 years ago
Blocks: 188153

Comment 11

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

Updated

14 years ago
Blocks: 205165

Updated

14 years ago
Attachment #123477 - Flags: review?(kin)

Updated

14 years ago
Blocks: 173277

Comment 12

14 years ago
Comment on attachment 123477 [details] [diff] [review]
revised patch

These changes look ok to me, r=kin@netscape.com, 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
does 
+    * 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
overflow
+  // 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
later
+  // without reflowing the frame.
Attachment #123477 - Flags: review?(kin) → review+

Comment 13

14 years ago
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
function:


@@ -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)) {
-    mState |= NS_FRAME_OUTSIDE_CHILDREN;
-  }
-  else {
-    mState &= ~NS_FRAME_OUTSIDE_CHILDREN;
-  }
+  
 }
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.
Attachment #123477 - Flags: superreview?(dbaron)
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.
Attachment #123477 - Flags: superreview?(dbaron)

Comment 16

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

Updated

14 years ago
Attachment #123477 - Flags: superreview+

Comment 17

14 years ago
fix checked in

Comment 18

14 years ago
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.
Assignee: bernd_mozilla → dbaron
We should aim this patch at 1.4 after it bakes a few days ... bug 205165 depends
on it.

Comment 20

14 years ago
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.
Attachment #123477 - Flags: approval1.4?
Comment on attachment 123477 [details] [diff] [review]
revised patch

Please mark with fixed1.4 when checked into 1.4 branch.
Attachment #123477 - Flags: approval1.4? → approval1.4+
Whiteboard: [a=rjesup,sspitzer for 1.4 final]
Target Milestone: Future → mozilla1.4final

Comment 22

14 years ago
Fix checked into the MOZILLA_1_4_BRANCH:

  mozilla/layout/html/base/src/nsFrame.h                      revision 3.188.2.1
  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 1.44.16.1
  mozilla/layout/base/public/nsIFrame.h                       revision 3.219.2.1
Keywords: fixed1.4

Comment 23

14 years ago
Can't we mark this one fixed?
(Assignee)

Comment 24

14 years ago
No.  It's not fixed.

Comment 25

13 years ago
How does this interact with bug 170330 ?

Comment 26

10 years ago
David, isn't this bug obsolete after the landing of the reflow branch?
(Assignee)

Comment 27

10 years ago
Not reflow branch, but I think roc fixed it at a different point... I should find the bug.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.