Closed
Bug 173277
Opened 22 years ago
Closed 21 years ago
investigate overflow area handling inside table layout
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
People
(Reporter: bernd_mozilla, Assigned: bernd_mozilla)
References
()
Details
(Whiteboard: [whitebox])
Attachments
(6 files, 3 obsolete files)
17.79 KB,
patch
|
Details | Diff | Splinter Review | |
37.10 KB,
patch
|
john
:
review+
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
50.46 KB,
patch
|
Details | Diff | Splinter Review | |
671 bytes,
text/html; charset=UTF-8
|
Details | |
584 bytes,
text/html; charset=UTF-8
|
Details | |
62.15 KB,
patch
|
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
the search is currently empty I doubt that this correct, see bug 172896 as a starting point. I know definetely that something should happen for nsTableOuterFrame otherwise the CSS2 caption-side example will not work.
Updated•22 years ago
|
OS: Windows 98 → All
Hardware: PC → All
Updated•22 years ago
|
Whiteboard: [whitebox]
Blocks: 105830
the testplan for this bug can be found at http://mitglied.lycos.de/captionside/overflow_testplan.html and will be updated as I progress. Currently my debug build fixes both dependent bugs. Once this is finished the testcases will go into tests/table/marvin
I updated the testplan and the corresponding testcases. This patch passes all the testcases there and fixes all dependent bugs.
Comment on attachment 124146 [details] [diff] [review] patch for review the patch is depends on the patch for bug 197581.
Attachment #124146 -
Flags: review?(jkeiser)
Attachment #124146 -
Flags: superreview?(dbaron)
Comment 7•21 years ago
|
||
Comment on attachment 124146 [details] [diff] [review] patch for review Scary but looking good.
Attachment #124146 -
Flags: review?(jkeiser) → review+
Comment on attachment 124146 [details] [diff] [review] patch for review This patch is full of "if(" (about half the time), but the surrounding style is "if (". Could you use "if ("? One big thing I'm wondering, overall, is whether it would be better to store the overflow area property in frame coordinates rather than parent coordinates. It looks (I think) like a majority of uses here want it in parent coordinates. The existing code could probably be changed rather easily. ( Also, I wonder if we should have the OVERFLOW_HIDDEN tests scattered all over the code or whether that should be done in |StoreOverflow| or any other place mOverflowArea is actually used. This would require changing existing block code, though. (I'm not sure all the block code is quite right, either.) ) > Index: nsTableCellFrame.cpp > @@ -664,6 +664,18 @@ > kidYTop = nsTableFrame::RoundToPixel(kidYTop, p2t, eAlwaysRoundDown); > } > firstKid->MoveTo(aPresContext, kidRect.x, kidYTop); > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { > + nsRect* kidOverflowArea = firstKid->GetOverflowAreaProperty(aPresContext); > + if (kidOverflowArea) { > + nsRect* cellOverflowArea = GetOverflowAreaProperty(aPresContext,PR_TRUE); > + if(cellOverflowArea) { > + nsRect tempRect(*kidOverflowArea); > + tempRect.MoveBy(kidRect.x, kidYTop); > + tempRect.UnionRect(tempRect, mRect); > + *cellOverflowArea = tempRect; > + } > + } > + } This code creates the overflow area property in more cases than necessary. (Consider a table cell whose block has overflow below its rect, but that is in a row with other larger cells so that the overflow doesn't extend outside the cell box.) That means the code should look like this: if (NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { nsRect* kidOverflowArea = firstKid->GetOverflowAreaProperty(aPresContext); if (kidOverflowArea) { nsRect tempRect(*kidOverflowArea); nsRect thisRect(0, 0, mRect.width, mRect.height); tempRect.MoveBy(kidRect.x, kidYTop); tempRect.UnionRect(tempRect, thisRect); if (tempRect != thisRect) { nsRect* cellOverflowArea = GetOverflowAreaProperty(aPresContext,PR_TRUE); if (cellOverflowArea) { *cellOverflowArea = tempRect; } } } } Note that thisRect is used instead of mRect to fix coordinate system differences. > @@ -1040,10 +1052,17 @@ > aDesiredSize.height = cellHeight; > aDesiredSize.ascent = topInset; > aDesiredSize.descent = bottomInset; > - > + > aDesiredSize.ascent += kidSize.ascent; ugh! > @@ -1091,6 +1110,7 @@ > > // remember the desired size for this reflow > SetDesiredSize(aDesiredSize); > + StoreOverflow(aPresContext, aDesiredSize); > > #if defined DEBUG_TABLE_REFLOW_TIMING > nsTableFrame::DebugReflow(this, (nsHTMLReflowState&)aReflowState, &aDesiredSize, aStatus); Given that you're going to call VerticallyAlignChild later (or does that not happen all the time?), should you really be calling |StoreOverflow| here? (If |VerticallyAlignChild| isn't always called, then you probably should be calling it here, but |VerticallyAlignChild| would then need to remove a now-unneeded overflow area.) > Index: nsTableFrame.cpp > @@ -2000,7 +2000,8 @@ > NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE); > // reflow the children > nsIFrame *lastReflowed; > - ReflowChildren(aPresContext, reflowState, !HaveReflowedColGroups(), PR_FALSE, aStatus, lastReflowed); > + ReflowChildren(aPresContext, reflowState, !HaveReflowedColGroups(), PR_FALSE, aStatus, > + lastReflowed, aDesiredSize.mOverflowArea); > } > mTableLayoutStrategy->Initialize(aPresContext, aReflowState); > } If you're going to wrap at all, please wrap at a standard width, i.e., something under 80 characters. > @@ -2162,10 +2171,18 @@ > // If we reflowed all the rows, then invalidate the largest possible area that either the > // table occupied before this reflow or will occupy after. > if (reflowedChildren) { > - Invalidate(aPresContext, nsRect(0, 0, PR_MAX(mRect.width, aDesiredSize.width), > - PR_MAX(mRect.height, aDesiredSize.height))); > + nsRect damage(0, 0, aDesiredSize.width, aDesiredSize.height); > + damage.UnionRect(damage, mRect); Isn't mRect in the wrong coordinate system? > + damage.UnionRect(damage, aDesiredSize.mOverflowArea); > + nsRect* overflowArea = GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + nsRect oldOverflowArea(*overflowArea); > + damage.UnionRect(damage, oldOverflowArea); Why not just damage.UnionRect(damage, *overflowArea); > + } > + Invalidate(aPresContext, damage); > } > > + StoreOverflow(aPresContext, aDesiredSize); > @@ -2791,7 +2809,9 @@ > aReflowState.availSize.width, aReflowState.availSize.height); > nsIFrame* lastReflowed; > PRBool reflowedAtLeastOne; > - ReflowChildren(aPresContext, reflowState, PR_FALSE, PR_TRUE, aStatus, lastReflowed, &reflowedAtLeastOne); > + nsRect overflowArea; > + ReflowChildren(aPresContext, reflowState, PR_FALSE, PR_TRUE, aStatus, lastReflowed, > + overflowArea, &reflowedAtLeastOne); > if (!reflowedAtLeastOne) > // XXX For now assume the worse > SetNeedStrategyInit(PR_TRUE); This seems wrong (although I haven't gotten to |ReflowChildren| yet). > @@ -3316,6 +3366,12 @@ > nsIFrame* nextKid = (childX + 1 < numRowGroups) ? (nsIFrame*)rowGroups.ElementAt(childX + 1) : nsnull; > pageBreak = PageBreakAfter(*kidFrame, nextKid); > } > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.reflowState.mStyleDisplay->mOverflow) { > + nsRect kidOverflowArea = desiredSize.mOverflowArea; > + kidOverflowArea.MoveBy(aReflowState.x, aReflowState.y); > + aOverflowArea.UnionRect(aOverflowArea, kidOverflowArea); > + } > + > // Place the child > PlaceChild(aPresContext, aReflowState, kidFrame, desiredSize); > > @@ -3363,6 +3419,11 @@ > rv = ReflowChild(repeatedFooter, aPresContext, desiredSize, footerReflowState, > aReflowState.x, aReflowState.y, 0, footerStatus); > PlaceChild(aPresContext, aReflowState, repeatedFooter, desiredSize); > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.reflowState.mStyleDisplay->mOverflow) { > + nsRect kidOverflowArea = desiredSize.mOverflowArea; > + kidOverflowArea.MoveBy(aReflowState.x, aReflowState.y); > + aOverflowArea.UnionRect(aOverflowArea, kidOverflowArea); > + } > } Can this chunk of code go inside |PlaceChild|? These are the only two callers of |PlaceChild|, and it seems silly to duplicate it. > break; > } > @@ -3379,10 +3440,23 @@ > Invalidate(aPresContext, kidRect); // invalidate the new position > } > } > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.reflowState.mStyleDisplay->mOverflow) { > + nsRect kidOverflowArea = kidRect; > + nsTableRowGroupFrame* rg; > + rg = (nsTableRowGroupFrame*) kidFrame; > + if (rg) { > + nsRect* overflowArea =rg->GetOverflowAreaProperty(aPresContext); I don't see any reason to cast. Furthermore |kidFrame| is already known to be non-null, so there's no point null-checking it. > + if (overflowArea) { > + kidOverflowArea = *overflowArea ; Extra whitespace. > + kidOverflowArea.MoveBy(kidRect.x, kidRect.y); > + } > + } > + aOverflowArea.UnionRect(aOverflowArea, kidOverflowArea); > + } > aReflowState.y += cellSpacingY + kidRect.height; > } > } > - > + > // if required, give the colgroups their initial reflows > if (aDoColGroups) { > nsHTMLReflowMetrics kidMet(nsnull); > @@ -3552,7 +3626,8 @@ > nsTableRowGroupFrame* rgFrame = aTableFrame.GetRowGroupFrame((nsIFrame*)rowGroups.ElementAt(rgX)); > nsTableRowFrame* rowFrame = rgFrame->GetFirstRow(); > while (rowFrame) { > - rowFrame->DidResize(aPresContext, aReflowState); > + nsHTMLReflowMetrics kidMet(nsnull); > + rowFrame->DidResize(aPresContext, kidMet, aReflowState); > rowFrame = rowFrame->GetNextRow(); > } > } This also seems wrong. Where does the information go? > Index: nsTableOuterFrame.cpp > =================================================================== > RCS file: /cvsroot/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,v > retrieving revision 3.236 > diff -u -r3.236 nsTableOuterFrame.cpp > --- nsTableOuterFrame.cpp 9 Apr 2003 21:14:51 -0000 3.236 > +++ nsTableOuterFrame.cpp 24 May 2003 18:16:49 -0000 > @@ -577,13 +577,24 @@ > PRUint8 aCaptionSide, > nsSize& aOuterSize, > PRBool aInnerChanged, > - PRBool aCaptionChanged) > + PRBool aCaptionChanged, > + nsRect* aOldOverflowArea) > { > if (!aInnerChanged && !aCaptionChanged) return; > > nsRect damage; > if (aInnerChanged && aCaptionChanged) { > damage = nsRect(0, 0, aOuterSize.width, aOuterSize.height); > + nsRect* overflowArea = GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + nsRect kidOverflowArea; > + kidOverflowArea = *overflowArea; > + damage.UnionRect(damage, kidOverflowArea); > + if(aOldOverflowArea) { > + kidOverflowArea = *aOldOverflowArea; > + damage.UnionRect(damage, kidOverflowArea); > + } > + } What if |aOldOverflowArea| is non-null but |overflowArea| is null? I'd think you'd want these null-checks to be sequential rather than nested. Also, does |InvalidateDamage| need so much code? Shouldn't the rects in question have zeros when needed so that you don't need a switch over the possible sides? > @@ -642,6 +653,26 @@ > } > break; > } > + nsRect kidOverflowArea(0,0,0,0); > + if(aCaptionChanged) { > + nsRect* overflowArea = mCaptionFrame->GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + kidOverflowArea = *overflowArea; > + } > + kidOverflowArea.MoveBy(captionRect.x, captionRect.y); > + } > + else { > + nsRect* overflowArea = mInnerTableFrame->GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + kidOverflowArea = *overflowArea; > + } > + kidOverflowArea.MoveBy(innerRect.x, innerRect.y); > + } You can condense by doing nsIFrame* kidFrame = aCaptionChanged ? mCaptionFrame : mInnerTableFrame; and then getting the overflow area from |kidFrame|. > + damage.UnionRect(damage, kidOverflowArea); > + if(aOldOverflowArea) { > + kidOverflowArea = *aOldOverflowArea; > + damage.UnionRect(damage, kidOverflowArea); > + } This could just do damage.UnionRect(damage, *aOldOverflowArea) and skip the copy into |kidOverflowArea|. > @@ -1374,57 +1405,36 @@ > if (aMet.mFlags & NS_REFLOW_CALC_MAX_WIDTH) { > aMet.mMaximumWidth = GetMaxWidth(aCaptionSide, aInnerMarginNoAuto, aCaptionMarginNoAuto); > } > - [...] > + > + nsRect tableOverflowArea(0, 0, 0, 0); > + nsRect tableRect; > + mInnerTableFrame->GetRect(tableRect); > + nsRect* overflowArea =mInnerTableFrame->GetOverflowAreaProperty(aPresContext); Missing space after =. > + if (overflowArea) { > + tableOverflowArea = *overflowArea; > + tableOverflowArea.MoveBy(tableRect.x, tableRect.y); // transfer into our coord. system > + } > + tableOverflowArea.UnionRect(tableOverflowArea, tableRect); > + > + nsRect captionOverflowArea(0, 0, 0, 0); > + nsRect captionRect; > + if (mCaptionFrame) { > + mCaptionFrame->GetRect(captionRect); > + captionOverflowArea = captionRect; > + overflowArea =((nsTableCaptionFrame*) mCaptionFrame)->GetOverflowAreaProperty(aPresContext); Missing space after =, and please use NS_STATIC_CAST. > + if (overflowArea) { > + > + captionOverflowArea = *overflowArea; > + captionOverflowArea.MoveBy(captionRect.x, captionRect.y); > + } > + captionOverflowArea.UnionRect(captionOverflowArea, captionRect); > + } > + > + nsRect outerRect(0,0,aMet.width, aMet.height); > + aMet.mOverflowArea.UnionRect(outerRect, captionOverflowArea); > + aMet.mOverflowArea.UnionRect(aMet.mOverflowArea, tableOverflowArea); > + > + StoreOverflow(aPresContext, aMet); Why so many temporaries? Can't you union things into |aMet.mOverflowArea| along the way and get rid of a bunch of the temporaries? > @@ -1652,7 +1662,20 @@ > sizeSet = PR_TRUE; > // Repaint the inner's entire bounds if it moved > if ((innerRect.x != innerOrigin.x) || (innerRect.y != innerOrigin.y)) { > - Invalidate(aPresContext, nsRect(0, 0, aDesiredSize.width, aDesiredSize.height)); > + nsRect damage(0, 0, aDesiredSize.width, aDesiredSize.height); > + nsRect kidOverflowArea(0, 0, 0, 0); > + nsRect* overflowArea = mInnerTableFrame->GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + kidOverflowArea = *overflowArea; > + } > + kidOverflowArea.MoveBy(innerRect.x, innerRect.y); > + damage.UnionRect(damage, kidOverflowArea); Can't you move these two lines inside the if? > + overflowArea = GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + nsRect oldOverflowArea(*overflowArea); > + damage.UnionRect(damage, oldOverflowArea); > + } (a) why do you need to do this if it's the inner frame that moved? (b) if you do, don't do the unnecessary copy > + Invalidate(aPresContext, damage); > } > } > if (!sizeSet) { > Index: nsTableRowFrame.cpp > =================================================================== > RCS file: /cvsroot/mozilla/layout/html/table/src/nsTableRowFrame.cpp,v > retrieving revision 3.323 > diff -u -r3.323 nsTableRowFrame.cpp > --- nsTableRowFrame.cpp 5 Apr 2003 15:36:01 -0000 3.323 > +++ nsTableRowFrame.cpp 24 May 2003 18:16:54 -0000 > @@ -394,16 +367,19 @@ > */ > void > nsTableRowFrame::DidResize(nsIPresContext* aPresContext, > + nsHTMLReflowMetrics& aDesiredSize, > const nsHTMLReflowState& aReflowState) > { > // Resize and re-align the cell frames based on our row height > nsTableFrame* tableFrame; > nsTableFrame::GetTableFrame(this, tableFrame); > if (!tableFrame) return; > - > + > nsTableIterator iter(aPresContext, *this, eTableDIR); > nsIFrame* childFrame = iter.First(); > - > + aDesiredSize.width = mRect.width; > + aDesiredSize.height = mRect.height; > + aDesiredSize.mOverflowArea = mRect; mRect is in the wrong coordinate system. > while (childFrame) { > nsCOMPtr<nsIAtom> frameType; > childFrame->GetFrameType(getter_AddRefs(frameType)); > @@ -433,12 +409,16 @@ > //ReflowChild(cellFrame, aPresContext, desiredSize, kidReflowState, status); > > cellFrame->VerticallyAlignChild(aPresContext, aReflowState, mMaxCellAscent); > + nsRect* overflowArea =cellFrame->GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + aDesiredSize.mOverflowArea.UnionRect(aDesiredSize.mOverflowArea, *overflowArea); > + } Do you need to transform coordinates? Also, the line inside the if is over-indented. > } > } > // Get the next child > childFrame = iter.Next(); > } > - > + StoreOverflow(aPresContext, aDesiredSize); > // Let our base class do the usual work > } > Is |DidResize| called on every reflow of something inside a table row? If not, might you need to call StoreOverflow elsewhere? > @@ -626,6 +606,7 @@ > return skip; > } > > +#if 0 > /** overloaded method from nsContainerFrame. The difference is that > * we don't want to clip our children, so a cell can do a rowspan > */ Don't |#if 0|, remove. > @@ -1080,7 +1063,12 @@ > nsSize priorSize = cellFrame->GetDesiredSize(); > desiredSize.width = priorSize.width; > desiredSize.height = priorSize.height; > - > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { > + nsRect* overflowArea =cellFrame->GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + desiredSize.mOverflowArea = *overflowArea ; > + } Shouldn't this be a UnionRect? > @@ -1136,6 +1130,16 @@ > // we need to account for the cell's width even if it isn't reflowed > nsRect rect; > kidFrame->GetRect(rect); > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { > + // Restore the overflowarea > + nsRect kidOverflowArea(0,0,0,0); > + nsRect* overflowArea =((nsTableCellFrame*) kidFrame)->GetOverflowAreaProperty(aPresContext); > + if (overflowArea) { > + kidOverflowArea = *overflowArea ; > + } > + kidOverflowArea.MoveBy(rect.x, rect.y); > + aDesiredSize.mOverflowArea.UnionRect(aDesiredSize.mOverflowArea, kidOverflowArea); shouldn't these two lines move inside the if, assuming cells can never overflow rows (which may actually be true)? > + } > x += rect.width; > } > > @@ -1177,7 +1181,11 @@ > aDesiredSize.height = PR_MIN(aDesiredSize.height, aReflowState.availableHeight); > } > } > - > + if(NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { > + nsRect rowRect(0, 0, aDesiredSize.width, aDesiredSize.height); > + aDesiredSize.mOverflowArea.UnionRect(aDesiredSize.mOverflowArea, rowRect); > + } I'd think you'd want that (which is just the mRect, translated) to be in the overflow area even if the 'overflow' style is 'hidden'. > + StoreOverflow(aPresContext, aDesiredSize); > return rv; > } > > @@ -1403,6 +1411,15 @@ I'm going to stop reviewing at this point because I suspect my comments on the remaining part of the patch will be very similar to my comments on the patch so far, and I think this does need a bit of work.
Attachment #124146 -
Flags: superreview?(dbaron) → superreview-
> One big thing I'm wondering, overall, is whether it would be better to
> store the overflow area property in frame coordinates rather than parent
I meant that the other way around.
*** Bug 201066 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 11•21 years ago
|
||
Assignee | ||
Comment 12•21 years ago
|
||
>This patch is full of "if(" (about half the time), but the surrounding >style is "if (". Could you use "if ("? I grep'ed table/src and removed all if('s most of them have been introduced by me anyway. Further I tried to minimize the warnings at http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl there are still 5 warnings left but I dont know how to wrap sensible: nsRect* overflowArea = cellFrame->GetOverflowAreaProperty(aPresContext); >One big thing I'm wondering, overall, is whether it would be better to >store the overflow area property in frame coordinates rather than parent >coordinates. It looks (I think) like a majority of uses here want it in >parent coordinates. The existing code could probably be changed rather >easily. The current situation separates the overflow of a given frame from its coordinates in the parent frame. If the overflow is stored in parent coordinates it combines the overflow and the child position inside the parent frame. If then the child frame is moved without beeing reflowed one need to separate what was the childs overflow and what was the position. I am not sure that this can be done correctly. >( Also, I wonder if we should have the OVERFLOW_HIDDEN tests scattered all >over the code or whether that should be done in |StoreOverflow| or any >other place mOverflowArea is actually used. This would require changing >existing block code, though. (I'm not sure all the block code is quite >right, either.) ) Comments by files: nsTableCellFrame.cpp >This code creates the overflow area property in more cases than >necessary. (Consider a table cell whose block has overflow below its >rect, but that is in a row with other larger cells so that the overflow >doesn't extend outside the cell box.) That means the code should look >like this: fixed >ugh! fixed >Given that you're going to call VerticallyAlignChild later (or does that >not happen all the time?), should you really be calling |StoreOverflow| >here? (If |VerticallyAlignChild| isn't always called, then you probably >should be calling it here, but |VerticallyAlignChild| would then need to >remove a now-unneeded overflow area.) added a comment that the alignment is done in vertical align child nsTableFrame.cpp >If you're going to wrap at all, please wrap at a standard width, i.e., >something under 80 characters. fixed >Isn't mRect in the wrong coordinate system? fixed >Why not just > damage.UnionRect(damage, *overflowArea); fixed >This seems wrong (although I haven't gotten to |ReflowChildren| yet). >Can this chunk of code go inside |PlaceChild|? These are the only two >callers of |PlaceChild|, and it seems silly to duplicate it. done >I don't see any reason to cast. Furthermore |kidFrame| is already known >to be non-null, so there's no point null-checking it. fixed that was code written befor the method was available for iframes >Extra whitespace. fixed >This also seems wrong. Where does the information go? fixed nsTableOuterFrame.cpp >What if |aOldOverflowArea| is non-null but |overflowArea| is null? I'd >think you'd want these null-checks to be sequential rather than nested. fixed >Also, does |InvalidateDamage| need so much code? Shouldn't the rects in >question have zeros when needed so that you don't need a switch over the >possible sides? We need to invalidate the margin space >You can condense by doing >nsIFrame* kidFrame = aCaptionChanged ? mCaptionFrame : mInnerTableFrame; >and then getting the overflow area from |kidFrame|. done >This could just do >damage.UnionRect(damage, *aOldOverflowArea) >and skip the copy into |kidOverflowArea|. fixed >Missing space after =. fixed >Missing space after =, fixed >and please use NS_STATIC_CAST. changed to nsIFrame >Why so many temporaries? Can't you union things into >|aMet.mOverflowArea| along the way and get rid of a bunch of the >temporaries? done >Can't you move these two lines inside the if? fixed > (a) why do you need to do this if it's the inner frame that moved? the inner frame can have a overflow and when it moves we need to invalidate the area that the overflow previously covered >(b) if you do, don't do the unnecessary copy done nsTableRowFrame.cpp >mRect is in the wrong coordinate system. fixed >Do you need to transform coordinates? yes >Is |DidResize| called on every reflow of something inside a table row? >If not, might you need to call StoreOverflow elsewhere? Its called from ResizeCells and DidResizeRows, but they are not always called >Don't |#if 0|, remove. done >Shouldn't this be a UnionRect? No, comment added >shouldn't these two lines move inside the if, assuming cells can never >overflow rows (which may actually be true)? This assumption is wrong, rowspanning cells overflow rows >I'd think you'd want that (which is just the mRect, translated) to be in >the overflow area even if the 'overflow' style is 'hidden'. indeed >I'm going to stop reviewing at this point because I suspect my comments >on the remaining part of the patch will be very similar to my comments >on the patch so far, and I think this does need a bit of work. only nsTableRowGroupFrame.cpp did not get reviewed due to this
Assignee | ||
Comment 13•21 years ago
|
||
Comment on attachment 126669 [details] [diff] [review] revised patch David could you please reevaluate, thanks for your patience
Attachment #126669 -
Flags: superreview?(dbaron)
I'm not sure what the sizing bug is on this testcase, though.
Attachment #127583 -
Attachment is obsolete: true
Comment on attachment 126669 [details] [diff] [review] revised patch >Index: base/src/nsFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v >retrieving revision 3.430 >diff -u -r3.430 nsFrame.cpp >--- base/src/nsFrame.cpp 31 May 2003 10:32:18 -0000 3.430 >+++ base/src/nsFrame.cpp 28 Jun 2003 07:38:28 -0000 >@@ -4630,6 +4630,24 @@ > } > } > >+void >+nsFrame::ConsiderChildOverflow(nsIPresContext* aPresContext, >+ nsHTMLReflowMetrics& aMetrics, >+ nsIFrame* aChildFrame) >+{ >+ const nsStyleDisplay* disp = GetStyleDisplay(); >+ if (NS_STYLE_OVERFLOW_HIDDEN != disp->mOverflow) { This could just be if (GetStyleDisplay()->mOverflow != NS_STYLE_OVERFLOW_HIDDEN) { >+ nsRect* overflowArea = aChildFrame->GetOverflowAreaProperty(aPresContext); >+ if (overflowArea) { >+ nsRect childRect; >+ aChildFrame->GetRect(childRect); >+ nsRect childOverflow(*overflowArea); >+ childOverflow.MoveBy(childRect.x,childRect.y); >+ aMetrics.mOverflowArea.UnionRect(aMetrics.mOverflowArea, >+ childOverflow); >+ } If there's no overflow area property, don't you want to consider the child's rect? Also, you should probably be using nsIFrame::GetRect() rather than nsIFrame::GetRect(nsRect&), so that this could look like: if (overflowArea) { nsRect childOverflow(*overflowArea); childOverflow.MoveBy(aChildFrame->GetPosition()); aMetrics.mOverflowArea.UnionRect(aMetrics.mOverflowArea, childOverflow); } else { aMetrics.mOverflowArea.UnionRect(aMetrics.mOverflowArea, aChildFrame->GetRect()); } (or at least it could if there were an nsRect::MoveBy(const nsPoint&), which there should be...) Does the patch do the right thing on the testcases that I attached?
Assignee | ||
Comment 18•21 years ago
|
||
the patch changes the code as dbaron proposed it removes all nsIFrame::GetRect(nsRect&) from layout/html/table/src it addes the MoveBy Function to nsRect.h it passes, as the previous patch, the attached testcases.
Assignee | ||
Comment 19•21 years ago
|
||
Comment on attachment 128225 [details] [diff] [review] patch updating the tree from time to time seems to be a good idea :-(
Attachment #128225 -
Attachment is obsolete: true
Assignee | ||
Comment 20•21 years ago
|
||
Attachment #128342 -
Flags: superreview?(dbaron)
Comment on attachment 128342 [details] [diff] [review] revised patch It's a little hard for me to check that you're always including nsRect(0, 0, mRect.width, mRect.height) in the overflow area. If you know of any cases where you aren't, I think they need to be fixed. (Consider frames like this: 1-------------------------------------- | | | 2-------------------------------- | | | | | 3------------------------- | | | | | | | | | -------------------------- | | | --------------------------------- --------------------------------------- where 3 is a child of 2, which is a child of 1. If 2 doesn't include its own rect in its overflow area, we won't detect that 1 has overflow.) Likewise, I probably didn't check that you were always ignoring the children when overflow was 'hidden'. (Should that check be done at a central point?) Index: layout/html/table/src/nsTableCellFrame.cpp + nsHTMLReflowMetrics desiredSize(nsnull); PR_FALSE, not nsnull + // the overflow are will be computed when the child will be vertical aligned s/are/area/ s/vertical/vertically/ Index: layout/html/table/src/nsTableFrame.cpp - // NS_ASSERTION(PR_FALSE, "intial reflow called twice"); + NS_ASSERTION(PR_FALSE, "intial reflow called twice"); NS_NOTREACHED("initial reflow called twice"); but you might want an XXX to remove the if eventually and make it an assertion that the if-expression is false. - Invalidate(aPresContext, nsRect(0, 0, PR_MAX(mRect.width, aDesiredSize.width), - PR_MAX(mRect.height, aDesiredSize.height))); + nsRect damage(0, 0, aDesiredSize.width, aDesiredSize.height); + damage.UnionRect(damage, nsRect(0, 0, mRect.width, mRect.height)); You could still use PR_MAX to combine these two, although what you currently have might be clearer. It might be nice to make nsTableFrame::GetBCMargin parallel to GetBCBorder -- i.e., make it return an nsMargin, and skip the input nsMargin&. (Actually, nobody passes PR_TRUE for aInnerBorderOnly, so you could remove that too.) And while you're cleaning up the two callers of GetBCMargin, GetContentAreaOffset doesn't need to null-check mStyleContext. The change to nsTableFrame::PlaceChild doesn't seem like it's going to do the right thing when a footer frame is present, since the final position hasn't yet been determined. (Speaking of which, doesn't the view positioning code in FinishReflowChild have the same problem?) + nsHTMLReflowMetrics tableDesiredSize(nsnull); + nsHTMLReflowMetrics desiredSize(nsnull); again, PR_FALSE, not nsnull. Also, perhaps call the latter |groupDesiredSize|? + aTableFrame.ConsiderChildOverflow(aPresContext, desiredSize, rgFrame); This could just be: // make the coordinates of |desiredSize.mOverflowArea| incorrect // since it's about to go away: desiredSize.mOverflowArea.MoveBy(rgFrame->GetOrigin()); tableDesiredSize.mOverflowArea.UnionRect(desiredSize.mOverflowArea); since you already have the overflow and you don't need to get it from the frame property again. Index: layout/html/table/src/nsTableFrame.h + // get the area that the border leak out from the inner table frame into + // the surrounding margin space + nsMargin GetBCMargin(nsIPresContext* aPresContext); The function should be |const| for consistency with GetBCBorder. Why does nsTableOuterFrame::InvalidateDamage need a switch statement at all? Why can't it just invalidate whichever of the caption or table changed, including their overflow areas? (It looks like many of the cases currently invalidate the margin between the table and the caption, but I think a bunch of them are wrong in various ways as well, although tending towards over-invalidation.) The |default| case is also mis-documented -- it's NS_SIDE_TOP. + StoreOverflow(aPresContext, aMet); Does anything use the overflow stored by the outer table frame? Should we assume that something will in the future (bug 197581)? - Invalidate(aPresContext, nsRect(0, 0, aDesiredSize.width, aDesiredSize.height)); + nsRect damage(0, 0, aDesiredSize.width, aDesiredSize.height); + nsRect* overflowArea = mInnerTableFrame->GetOverflowAreaProperty(aPresContext); + if (overflowArea) { + nsRect kidOverflowArea = *overflowArea; + kidOverflowArea.MoveBy(innerRect.x, innerRect.y); + damage.UnionRect(damage, kidOverflowArea); + } + overflowArea = GetOverflowAreaProperty(aPresContext); + if (overflowArea) { + // Include the old overflow area + damage.UnionRect(damage, *overflowArea); + } + Invalidate(aPresContext, damage); This seems over-complicated and it doesn't handle some cases where the inner had negative margins. (Consider removal of a top caption if the table has a negative top margin.) Since this is just a translation of the inner table frame, why not get the size of the inner table frame (from its rect and perhaps its overflow area property) and include that size at both the old and new positions? Index: layout/html/table/src/nsTableRowFrame.cpp + const nsStyleDisplay* disp = GetStyleDisplay(); + PRBool considerOverflow = (NS_STYLE_OVERFLOW_HIDDEN != disp->mOverflow); Perhaps cleaner as: PRBool considerOverflow = NS_STYLE_OVERFLOW_HIDDEN != GetStyleDisplay()->mOverflow; (one line; no over-parenthesization) + // a cell that has a rowspan != 1 causes a overflow "causes overflow" + nsRect cellOverflowArea(0, 0, cellFrame->GetSize().width, cellHeight); + if (considerOverflow) { + nsRect* overflowArea = cellFrame->GetOverflowAreaProperty(aPresContext); + if (overflowArea) { + cellOverflowArea = *overflowArea; + } + } + cellOverflowArea.MoveBy(cellFrame->GetPosition()); + desiredSize.mOverflowArea.UnionRect(desiredSize.mOverflowArea, + cellOverflowArea); Shouldn't this whole thing be inside |if (considerOverflow)|? Also, why not initialize cellOverflowArea to cellFrame->GetRect() and do the MoveBy in the |if (overflowArea)|? + if (NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { Isn't that checking the row's overflow when you should be checking the cell's? Index: layout/html/table/src/nsTableRowGroupFrame.cpp + nsSize kidSize; + kidFrame->GetSize(kidSize); nsSize kidSize = kidFrame->GetSize(); The row group frame never seems to include nsRect(0, 0, mRect.width, mRect.height) in its overflow area. Why did you set the overflow area to nsRect(0, 0, 0, 0) in DidResizeRows?
Attachment #126669 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 22•21 years ago
|
||
There is a inconsistent use of pointer and references to the prescontext inside the table code. I made the functions consistent that dbaron requested, and kicked one unused prescontext. My feeling is that there should be only pointer to the prescontext, but I would like to fix this outside this bug as the patch with all these cleanings is now already too large (for me at least).
Attachment #128342 -
Attachment is obsolete: true
Attachment #128342 -
Flags: superreview?(dbaron)
Attachment #129498 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 23•21 years ago
|
||
>It's a little hard for me to check that you're always including >nsRect(0, 0, mRect.width, mRect.height) in the overflow area. If you >know of any cases where you aren't, I think they need to be fixed. >(Consider frames like this: > 1-------------------------------------- > | | > | 2-------------------------------- > | | | | > | 3------------------------- | | > | | | | | | > | -------------------------- | | > | --------------------------------- > --------------------------------------- >where 3 is a child of 2, which is a child of 1. If 2 doesn't include >its own rect in its overflow area, we won't detect that 1 has overflow.) > >Likewise, I probably didn't check that you were always ignoring the >children when overflow was 'hidden'. (Should that check be done at a >central point?) Its done now at a central point, the childs overflow area is only retrieved via ConsiderChildOverflow there is the childRect included and the overflow:hidden is checked. >Index: layout/html/table/src/nsTableCellFrame.cpp >+ nsHTMLReflowMetrics desiredSize(nsnull); >PR_FALSE, not nsnull I removed all instances of this pattern, but I am still so used to it. >+ // the overflow are will be computed when the child will be vertical aligned > >s/are/area/ >s/vertical/vertically/ done >Index: layout/html/table/src/nsTableFrame.cpp > >- // NS_ASSERTION(PR_FALSE, "intial reflow called twice"); >+ NS_ASSERTION(PR_FALSE, "intial reflow called twice"); > >NS_NOTREACHED("initial reflow called twice"); >but you might want an XXX to remove the if eventually and make it an >assertion that the if-expression is false. changed >- Invalidate(aPresContext, nsRect(0, 0, PR_MAX(mRect.width, >aDesiredSize.width), >- PR_MAX(mRect.height, >aDesiredSize.height))); >+ nsRect damage(0, 0, aDesiredSize.width, aDesiredSize.height); >+ damage.UnionRect(damage, nsRect(0, 0, mRect.width, mRect.height)); >You could still use PR_MAX to combine these two, although what you >currently have might be clearer. >It might be nice to make nsTableFrame::GetBCMargin parallel to >GetBCBorder -- i.e., make it return an nsMargin, and skip the input >nsMargin&. (Actually, nobody passes PR_TRUE for aInnerBorderOnly, so >you could remove that too.) done that, >And while you're cleaning up the two >callers of GetBCMargin, GetContentAreaOffset doesn't need to null-check >mStyleContext. done that, but if you could explain why I dont need to null check I would feels more secure. >The change to nsTableFrame::PlaceChild doesn't seem like it's going to >do the right thing when a footer frame is present, since the final >position hasn't yet been determined. I moved the call outside the PlaceChild routine towards the end of the for loop >(Speaking of which, doesn't the >view positioning code in FinishReflowChild have the same problem?) I am not sure what do you mean with this. >+ nsHTMLReflowMetrics tableDesiredSize(nsnull); >+ nsHTMLReflowMetrics desiredSize(nsnull); >again, PR_FALSE, not nsnull. Also, perhaps call the latter >|groupDesiredSize|? >+ aTableFrame.ConsiderChildOverflow(aPresContext, desiredSize, rgFrame); >This could just be: > // make the coordinates of |desiredSize.mOverflowArea| incorrect > // since it's about to go away: > desiredSize.mOverflowArea.MoveBy(rgFrame->GetOrigin()); > tableDesiredSize.mOverflowArea.UnionRect(desiredSize.mOverflowArea); >since you already have the overflow and you don't need to get it from >the frame property again. done Index: layout/html/table/src/nsTableFrame.h >+ // get the area that the border leak out from the inner table frame into >+ // the surrounding margin space >+ nsMargin GetBCMargin(nsIPresContext* aPresContext); >The function should be |const| for consistency with GetBCBorder. fixed >Why does nsTableOuterFrame::InvalidateDamage need a switch statement at >all? Why can't it just invalidate whichever of the caption or table >changed, including their overflow areas? (It looks like many of the >cases currently invalidate the margin between the table and the caption, >but I think a bunch of them are wrong in various ways as well, although >tending towards over-invalidation.) If I remember it correctly inlcuding the margins is the thing Chris told me to do. The issue is that the outertable frame does not paint the margin space it returns PR_FALSE for can paint background. >The |default| case is also mis-documented -- it's NS_SIDE_TOP. fixed >+ StoreOverflow(aPresContext, aMet); > >Does anything use the overflow stored by the outer table frame? Should >we assume that something will in the future (bug 197581)? The outer table frame itself, and I believe the block around should use it >- Invalidate(aPresContext, nsRect(0, 0, aDesiredSize.width, >aDesiredSize.height)); >+ nsRect damage(0, 0, aDesiredSize.width, aDesiredSize.height); >+ nsRect* overflowArea = >mInnerTableFrame->GetOverflowAreaProperty(aPresContext); >+ if (overflowArea) { >+ nsRect kidOverflowArea = *overflowArea; >+ kidOverflowArea.MoveBy(innerRect.x, innerRect.y); >+ damage.UnionRect(damage, kidOverflowArea); >+ } >+ overflowArea = GetOverflowAreaProperty(aPresContext); >+ if (overflowArea) { >+ // Include the old overflow area >+ damage.UnionRect(damage, *overflowArea); >+ } >+ Invalidate(aPresContext, damage); >This seems over-complicated and it doesn't handle some cases where the >inner had negative margins. (Consider removal of a top caption if the >table has a negative top margin.) Since this is just a translation of >the inner table frame, why not get the size of the inner table frame >(from its rect and perhaps its overflow area property) and include that >size at both the old and new positions? I changed that to a call to InvalidateDamage that should be more correct. Index: layout/html/table/src/nsTableRowFrame.cpp >+ const nsStyleDisplay* disp = GetStyleDisplay(); >+ PRBool considerOverflow = (NS_STYLE_OVERFLOW_HIDDEN != disp->mOverflow); >Perhaps cleaner as: > PRBool considerOverflow = > NS_STYLE_OVERFLOW_HIDDEN != GetStyleDisplay()->mOverflow; >(one line; no over-parenthesization) >+ // a cell that has a rowspan != 1 causes a overflow >"causes overflow" >+ nsRect cellOverflowArea(0, 0, cellFrame->GetSize().width, cellHeight); >+ if (considerOverflow) { >+ nsRect* overflowArea = >cellFrame->GetOverflowAreaProperty(aPresContext); >+ if (overflowArea) { >+ cellOverflowArea = *overflowArea; >+ } >+ } >+ cellOverflowArea.MoveBy(cellFrame->GetPosition()); >+ desiredSize.mOverflowArea.UnionRect(desiredSize.mOverflowArea, >+ cellOverflowArea); > >Shouldn't this whole thing be inside |if (considerOverflow)|? >Also, why not initialize cellOverflowArea to cellFrame->GetRect() and do >the MoveBy in the |if (overflowArea)|? I replaced all the stuff with a single ConsiderChildOverflow which does exactly what we want. + if (NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow) { I>sn't that checking the row's overflow when you should be checking the cell's? >Index: layout/html/table/src/nsTableRowGroupFrame.cpp >+ nsSize kidSize; >+ kidFrame->GetSize(kidSize); >nsSize kidSize = kidFrame->GetSize(); I replaced the code with a ConsiderChildOverflow call >The row group frame never seems to include nsRect(0, 0, mRect.width, >mRect.height) in its overflow area. fixed >Why did you set the overflow area to nsRect(0, 0, 0, 0) in >DidResizeRows? the idea was to clear the overflow area when we resize all rows
>done that, but if you could explain why I dont need to null check I would feels >more secure. Frames are created based on information in style contexts. They can't exist without a style context. (And even if things didn't work that way, it would be good to make it that way artificially so that the null-check could be at one central point instead of all over.) >If I remember it correctly inlcuding the margins is the thing Chris told me to >do. The issue is that the outertable frame does not paint the margin space it >returns PR_FALSE for can paint background. Why do you need to invalidate the margin space?
Attachment #129498 -
Flags: superreview?(dbaron) → superreview+
Blocks: 215721
Blocks: 218862
Assignee | ||
Comment 25•21 years ago
|
||
fix checked in, followup bugs are bug 219141 and bug 219142
Assignee | ||
Comment 26•21 years ago
|
||
marking fixed
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 27•21 years ago
|
||
Someone should check the dependent bugs to see whether any of those are fixed...
Comment 28•21 years ago
|
||
I have checked all blocked bugs and most of them were fixed by this bug, but there are several problems still around. bug 219220 - The overflow is not visible. bug 194822, bug 204200, bug 211641 and (in part) bug 219220 - The vertical scrollbar is not triggered by the overflow. bug 218218 - The overflowed part of a button is not repainted correctly. bug 206343 - A menu does not appear (this could be invalid though, since the testcases work fine). Also, the first testcase in bug 155955 shows that there is a pixel-rounding(?) problem between the overflowed half of the border and the other half, I think it could be related to this bug.
Comment 29•21 years ago
|
||
Bug 28800 should depend on this bug. The recent fix here improved that bug. Content that was previously hidden is now accessible, although it requires horizontal scrolling to access.
Comment 30•21 years ago
|
||
*** Bug 223793 has been marked as a duplicate of this bug. ***
Comment 31•21 years ago
|
||
*** Bug 225605 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•