173277 - investigate overflow area handling inside table layout

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

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.

Comment 1

[Bernd]

Attachment: patch

Comment 2

[Bernd]

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

Comment 3

[saari (gone)]

we should try to get this fixed

Comment 4

[Bernd]

Attachment: patch for review

I updated the testplan and the corresponding testcases. This patch passes all
the testcases there and fixes all dependent bugs.

Comment 5

[Bernd]

Comment on attachment 124146 [details] [diff] [review]
patch for review

the patch is depends on the patch for bug 197581.

Comment 6

[Samir Gehani]

adt: nsbeta1-

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 124146 [details] [diff] [review]
patch for review

Scary but looking good.

Comment 8

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

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.

Comment 9

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

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

Comment 10

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

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

Comment 11

[Bernd]

Attachment: revised patch

Comment 12

[Bernd]

>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

Comment 13

[Bernd]

Comment on attachment 126669 [details] [diff] [review]
revised patch

David could you please reevaluate, thanks for your patience

Comment 14

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

Attachment: testcase - absolute positioning (the scrollbars should allow scrolling to the edges of the green box)

I'm not sure what the sizing bug is on this testcase, though.

Comment 15

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

Attachment: testcase - relative positioning (the scrollbars should allow scrolling to the edges of the green box)

Comment 16

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

Attachment: testcase - relative positioning (the scrollbars should allow scrolling to the edges of the green box)

Comment 17

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

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?

Comment 18

[Bernd]

Attachment: patch

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.

Comment 19

[Bernd]

Comment on attachment 128225 [details] [diff] [review]
patch

updating the tree from time to time seems to be a good idea :-(

Comment 20

[Bernd]

Attachment: revised patch

Comment 21

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

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?

Comment 22

[Bernd]

Attachment: next revision

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

Comment 23

[Bernd]

>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

Comment 24

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

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

Comment 25

[Bernd]

fix checked in, followup bugs are bug 219141 and bug 219142

Comment 26

[Bernd]

marking fixed

Comment 27

[Boris Zbarsky [:bzbarsky]]

Someone should check the dependent bugs to see whether any of those are fixed...

Comment 28

[Mats Palmgren (inactive)]

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

[Hanspeter Niederstrasser]

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

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