(no longer active) Assignee
	Description • 13 years ago

      No description provided.

	David Baron :dbaron:
	Comment 1 • 13 years ago

I think this is as intended, and in fact even what the spec calls for.

	(no longer active) Assignee
	Comment 2 • 13 years ago

(In reply to comment #1)
> I think this is as intended, and in fact even what the spec calls for.

Dbaron convinced me about this on IRC.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 13 years ago

Can you convince me too?

The spec says that table margins should be outside the caption, i.e. outside the table outer frame. So when section 10.3.3 of CSS 2.1 kicks in for the table and we hit the over-constrained case, the right margin is adjusted to fit the width of the containing block. The table's right margin should not be included in the border-box of the table outer frame.

The spec also says "The width of the table wrapper box is the border-edge width of the table box inside it." which pretty much states that this bug is valid, doesn't it?

http://www.w3.org/TR/CSS21/tables.html#model
http://www.w3.org/TR/CSS21/visudet.html#blockwidth

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 13 years ago

And the key part there is that "Percentages on 'width' and 'height' on the table are relative to the table wrapper box's containing block, not the table wrapper box itself"...

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 13 years ago

(In reply to comment #3)
> The spec says that table margins should be outside the caption, i.e. outside
> the table outer frame.

I guess this doesn't necessarily follow; we could have an approach where the table margins are applied inside the wrapper box but outside the caption.

But "the width of the table wrapper box is the border-edge width of the table box inside it" text settles it: the margins have to go outside the table wrapper box.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 13 years ago

I think we need to have the -moz-table-outer rule in ua.css use "margin:inherit". Then we need to disable margins on the inner table frame; I think it might be enough to just remove 'innerMargin' in nsTableOuterFrame::Reflow and treat it as zero wherever we use it now.

This might fix bug 87277 as well.

	David Baron :dbaron:
	Comment 7 • 13 years ago

That (last para. of comment 5) only makes sense for two of the six values of 'caption-side' that we implement -- the two that are described in CSS 2.1.  The other four are the four described in CSS 2.0.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 13 years ago

Yes. If the other caption-side values are added back to CSS, that text would have to be changed. But the intent is still pretty clear...

	(no longer active) Assignee
	Comment 9 • 13 years ago

(In reply to comment #6)
> I think we need to have the -moz-table-outer rule in ua.css use
> "margin:inherit". Then we need to disable margins on the inner table frame;
> I think it might be enough to just remove 'innerMargin' in
> nsTableOuterFrame::Reflow and treat it as zero wherever we use it now.

This mostly works, but it breaks the cases where we need auto margins on the table, such as with <table align="center">...

	(no longer active) Assignee
	Comment 10 • 13 years ago

So, here's how table positioning works right now.  We set the width of the outer frame to its containing block width, and we reflow the children, then we get the inner frame's width, and if they are different, we reposition the inner frame with respect to the outer frame based on the difference in widths of the inner and outer frame, and the table alignment rules.  So for <table align=center> for example, we have the outer table frame as wide as the containing block, and we position the inner frame with an X coordinate of (outerWidth - innerWidth)/2, roughly speaking.

This model breaks if the outer frame width is supposed to be the same as the inner frame width, since the positioning of the outer frame is done before its Reflow method is called, using the computed margins calculated in the outer table frame's reflow state (in the InitConstraints method) and the position is considered to be constant for the duration of the reflow of the outer frame.

David, roc: do you have any ideas how I can position the outer table frame correctly with respect to its parent, so making the inner table margins 0 doesn't break table positioning?

	David Baron :dbaron:
	Comment 11 • 13 years ago

For 'top-outside', 'bottom-outside', 'right', and 'left', I could go either way about whether the margins on the table go on the table box or the table wrapper box, but it's probably best to be consistent and put them on the table wrapper box throughout.  This requires making the table wrapper inherit the margins through ua.css, and then making nsTableFrame ignore its own margins (nsTableFrame::GetUsedMargin plus an nsHTMLReflowState hack, I think).

For sizing I think there are three basic cases:

 'top' and 'bottom' captions (the two values specified in CSS 2.1)

    I think the thing to do here is rename nsTableFrame::ComputeSize to a
    non-virtual DoComputeSize, and for the top and bottom caption case, make
    nsTableOuterFrame::ComputeSize delegate to the table.  Then
    nsTableFrame::ComputeSize, correspondingly, for the top and bottom case
    just uses its parent's size, and for the other cases falls through to
    its current behavior.  Given this change we should probably also change
    nsTableOuterFrame::GetMinWidth/GetPrefWidth to *ignore* the caption
    frame, at least if that's compatible with other browsers (otherwise we
    probably need to revise the spec).  Right now we do honor the caption's
    *min* width in these cases, but not its pref width.  In any case, the
    way we handle the caption's min width needs to be consistent between
    min/pref calculation and layout.

    Then we need to make sure that alignment gets applied to the wrapper box
    (and possibly *also* the table box if we do continue to honor the
    caption's min width).  (We used to do that pre-reflow branch, but I
    recall it being ugly for some reason.)

 'top-outside' and 'bottom-outside' captions (which implement what was 'top'
  and 'bottom' in CSS 2.0):

    For these cases, I think we should keep the current behavior where the table
    wrapper box is the same size as its container.

  'right' and 'left' captions

    Same.

	(no longer active) Assignee
	Comment 12 • 13 years ago

This is my work in progress patch.

Using available width in nsTableFrame::ComputeSize doesn't seem quite correct.  This causes the width of the outer table in this test case to be wider than it would be otherwise: <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/444015-1-ref.html?force=1>.

I've run out of ideas.  David, do you see anything that I might have missed here?

	David Baron :dbaron:
	Comment 13 • 13 years ago

You could just run ComputeSize twice; I think it should be relatively cheap.

	(no longer active) Assignee
	Comment 14 • 13 years ago

(In reply to comment #13)
> You could just run ComputeSize twice; I think it should be relatively cheap.

I'm not sure how that would calling nsTableFrame::ComputeSize twice help...

	David Baron :dbaron:
	Comment 15 • 13 years ago

i.e., don't bother with the DoComputeSize thing, and just leave nsTableFrame::ComputeSize as it is, but call it from nsTableOuterFrame::ComputeSize in some cases (so that it runs twice in those cases).

	(no longer active) Assignee
	Comment 16 • 13 years ago

The outer frame inherits its style context from the inner frame's.  Adding margin: inherit to ua.css for ::-moz-table-outer causes the outer frame to get the correct margin value, but it also causes the margin value to be left on the inner table frame as well, which breaks the calculations we perform in nsLayoutUtils::IntrinsicForContainer.  Is there a clean way of making sure that the margin on the inner table frame is always set to 0, while not breaking the inheritance of the specified value to the outer table frame?

	(no longer active) Assignee
	Comment 17 • 13 years ago

Still a long way to go :(  I've mostly been focusing on fixing reftest failures...

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 13 years ago

(In reply to comment #16)
> The outer frame inherits its style context from the inner frame's.  Adding
> margin: inherit to ua.css for ::-moz-table-outer causes the outer frame to
> get the correct margin value, but it also causes the margin value to be left
> on the inner table frame as well, which breaks the calculations we perform
> in nsLayoutUtils::IntrinsicForContainer.  Is there a clean way of making
> sure that the margin on the inner table frame is always set to 0, while not
> breaking the inheritance of the specified value to the outer table frame?

You should probably override nsTableFrame::IntrinsicWidthOffsets to set the margin to zero, the same way nsTableCellFrame does. You should also override nsTableFrame::GetUsedMargin (nsTableCellFrame does that too).

	(no longer active) Assignee
	Comment 20 • 13 years ago

One of the reftests that is failing with this patch is <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/478614-1.html?force=1>.  What's happening is that the test file has a table with 100px top margin, and now that we apply the margin to the outer table frame, it gets collapsed with the default top margin of the body element (8px), while the reference file <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/478614-1-ref.html?force=1> doesn't have a top margin on the table element, so the 8px top margin of the body element kicks in, causing the reference to be rendered 8 pixels below the test file.

I'm not sure if this is the wrong behavior.  If you agree, I can just add a margin-top: 0 rule to the body element in these tests...

	David Baron :dbaron:
	Comment 21 • 13 years ago

Yes, that test looks incorrect, because it expects us to have bug 87277.  Does your patch fix that?

	(no longer active) Assignee
	Comment 22 • 13 years ago

(In reply to comment #21)
> Yes, that test looks incorrect, because it expects us to have bug 87277. 
> Does your patch fix that?

Yes! :-)

	(no longer active) Assignee
	Comment 23 • 13 years ago

This is a milestone, in that this patch should pass the entire reftest suite (I think, I'm testing this assertion locally).

After this, I'll look into a mochitest failure which is happening because of this, and then I'll clean it up and submit it for review.

	(no longer active) Assignee
	Comment 24 • 13 years ago

OK, I think this is finally ready for review.  I hope I've caught all of the issues with this patch...

	(no longer active) Assignee
	Comment 25 • 13 years ago

Boris, roc, if you guys also wanna take a look at the patch, be my guest.  :-)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 13 years ago

Comment on attachment 538158 [details] [diff] [review]
Patch (v1)

Review of attachment 538158 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +293,5 @@
>  }
>  
> +static PRBool IsOutsideCaptionFrame(nsIFrame *frame)
> +{
> +  if (frame->GetType() == nsGkAtoms::tableCaptionFrame) {

The GetType() call is not free. Is there a way to avoid it? Perhaps we can pass in the result of GetType() from somewhere else we're already calling it?

@@ +2199,5 @@
> +  // table inner frames don't have their own margins
> +  nsIAtom* frameType = frame->GetType();
> +  if (frameType == nsGkAtoms::tableFrame) {
> +    mComputedMargin.SizeTo(0, 0, 0, 0);
> +    return PR_FALSE;

Do we have to have this here? Table cells don't seem to need it.

::: layout/reftests/bugs/478614-1-ref.html
@@ +2,5 @@
>  <html>
>  <head>
>  <title>Testcase for table + caption vertical margins (bug 478614)</title>
>  <style type="text/css">
> +body {margin-top: 0} /* Don't let collapsing of the body's top margin interfere with positining */

fix spelling (same mistake copied into several places)

::: layout/style/nsComputedDOMStyle.cpp
@@ +3294,5 @@
>  
> +    // For tables, GetUsedMargin always returns an empty margin, so we
> +    // should read the margin from the outer table frame instead.
> +    nscoord value;
> +    if (mInnerFrame->GetType() == nsGkAtoms::tableFrame) {

Maybe the thing to do here is to just add the margins from the inner and outer frames, if the frames are different?

	(no longer active) Assignee
	Comment 27 • 13 years ago

(In reply to comment #26)
> Comment on attachment 538158 [details] [diff] [review] [review]
> Patch (v1)
> 
> Review of attachment 538158 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsHTMLReflowState.cpp
> @@ +293,5 @@
> >  }
> >  
> > +static PRBool IsOutsideCaptionFrame(nsIFrame *frame)
> > +{
> > +  if (frame->GetType() == nsGkAtoms::tableCaptionFrame) {
> 
> The GetType() call is not free. Is there a way to avoid it? Perhaps we can
> pass in the result of GetType() from somewhere else we're already calling it?

Yes.  We also call it in InitFrameType, and I can just cache the value and pass it along.  I'll post an extensive patch to address this.

> @@ +2199,5 @@
> > +  // table inner frames don't have their own margins
> > +  nsIAtom* frameType = frame->GetType();
> > +  if (frameType == nsGkAtoms::tableFrame) {
> > +    mComputedMargin.SizeTo(0, 0, 0, 0);
> > +    return PR_FALSE;
> 
> Do we have to have this here? Table cells don't seem to need it.

Yes, taking this out breaks lots of reftests.

> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +3294,5 @@
> >  
> > +    // For tables, GetUsedMargin always returns an empty margin, so we
> > +    // should read the margin from the outer table frame instead.
> > +    nscoord value;
> > +    if (mInnerFrame->GetType() == nsGkAtoms::tableFrame) {
> 
> Maybe the thing to do here is to just add the margins from the inner and
> outer frames, if the frames are different?

Makes sense.

	(no longer active) Assignee
	Comment 30 • 13 years ago

Sorry, uploaded the wrong patches.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 13 years ago

Comment on attachment 538352 [details] [diff] [review]
Part 2: Avoid calling nsIFrame::GetType needlessly in nsHTMLReflowState

Review of attachment 538352 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +2197,5 @@
>    return lineHeight;
>  }
>  
>  PRBool
> +nsCSSOffsetState::ComputeMargin(nscoord aContainingBlockWidth, nsIAtom* frameType)

aFrameType

	(no longer active) Assignee
	Comment 33 • 13 years ago

(In reply to comment #32)
> >  PRBool
> > +nsCSSOffsetState::ComputeMargin(nscoord aContainingBlockWidth, nsIAtom* frameType)
> 
> aFrameType

Fixed in my local repo.

	(no longer active) Assignee
	Comment 34 • 13 years ago

dbaron: ping

	David Baron :dbaron:
	Comment 35 • 13 years ago

Comment on attachment 538351 [details] [diff] [review]
Patch (v2)

>Bug 659828 - Outer table frame's width is set to the width of the containing block

Please use a commit message describing what you're changing.  I think in
this case you probably want a brief message on the first line (probably
60-160 characters), and then a wrapped multi-line message following it
(e.g., as for http://hg.mozilla.org/mozilla-central/rev/830111e10951
though perhaps not that long).  But both of them should be a description
of what you're changing, not just the summary of the bug.

(Having such a description also makes it much easier for me to review,
since I can review the idea of what you are changing and then review
whether you did it correctly, rather than the reverse.  This especially
saves work when I find problems in the former.)


nsBlockFrame.cpp:

>     {
>-      nsCSSOffsetState tableOS(innerTable, aState.mReflowState.rendContext,
>-                               aState.mContentArea.width);
>-      tableMargin = tableOS.mComputedMargin;
>+      tableMargin = offsetState.mComputedMargin;
>     }

remove braces and unindent since scope is no longer needed.

>+    nscoord widthAdjustment = 0;
>+    // If we don't have a caption frame, then the table margins belong
>+    // to the outer table frame (because the margins of the inner table
>+    // frame will always be zero).  We have to make up for this by
>+    // adjusting the width returned from ComputeSize.
>+    if (!caption) {
>+      widthAdjustment = tableMargin.LeftRight();
>+    }

Why is this code specific to not having a caption frame?  And why
do you need to do this at all?

nsBlockReflowState.cpp:

Given this:
>     // We pass in aReplacedWidth to make handling outer table frames
>     // work correctly.  For outer table frames, we need to subtract off
>     // the margin that's going to be at the edge of them, since we're
>     // dealing with margin that it's really the child's responsibility
>     // to place.
can you now remove the aReplacedWidth parameter entirely (and the code
in ComputeBlockAvailSpace that uses WidthToClearPastFloats in order to
pass it)?  I think that would simplify this and hopefully avoid the need
for the changes mentioned above and the changes you now have in this
file.

Also, don't use "now" in comments.

nsHTMLReflowState.cpp:

>+  if (IsOutsideCaptionFrame(frame)) {
>+    mCBReflowState = parentReflowState->mCBReflowState;
>+  }
>   // Absolutely positioned frames should always be kids of the frames that
>   // determine their containing block, except for inner table frames.
>-  if (parentReflowState->frame == frame->GetContainingBlock() ||
>-      (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {
>+  else if (parentReflowState->frame == frame->GetContainingBlock() ||
>+           (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {

Why are you doing this?  Isn't the plan to switch to the margin on the
table outer frame in all modes?  I don't see why outside captions should
be an exception here.

>-      if (isBlock && !IsSideCaption(frame, mStyleDisplay))
>+      // Exclude inline tables from the block margin calculations
>+      if (isBlock && !IsSideCaption(frame, mStyleDisplay) &&
>+          frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE)

Given the isBlock test this looks like a no-op change.

Instead of changing nsCSSOffsetState::ComputeMargin, please put
the change you're making in there in nsCSSOffsetState::InitOffsets
along with the other very-similar code.

nsComputedDOMStyle.cpp:

Please NS_ASSERTION that value == 0 right before adding the outer
frame's margin.

nsTableFrame.cpp:

In nsTableFrame::GetUsedMargin (and also in
nsCSSOffsetState::InitOffsets), please add a comment explaining that the
margin has been inherited to the wrapper frame via the
::-moz-table-outer rule in ua.css.

nsTableOuterFrame.cpp:

Instead of testing aChildFrame->GetType(), test equality against
mInnerTableFrame.

Secondly, though, I don't see why you need this change to
ChildShrinkWrapWidth at all.  If the margins are now on the outer
table frame, the shrink wrapping behavior should work that way too.
Does that not work for some reason?  (Second, if we needed them, we
already have them in the calling function; recomputing them is
unnecessarily expensive.)

The changes to nsTableOuterFrame::ComputeAutoSize that add margin also
all appear to be wrong.  ComputeAutoSize returns content-box sizes.

Also, I don't think you should assign to aShrinkWrap; just ignore it.

I'm also not sure why you're doing what you're doing in
GetCaptionOrigin, GetInnerOrigin, and Reflow.  Didn't we decide to
switch to the CSS 2.1 margin model for all sides?

	(no longer active) Assignee
	Comment 36 • 13 years ago

(In reply to comment #35)
> Comment on attachment 538351 [details] [diff] [review] [review]
> Patch (v2)
> 
> >Bug 659828 - Outer table frame's width is set to the width of the containing block
> 
> Please use a commit message describing what you're changing.

Yep, sorry.  I will do that when I submit the next patch for review.

> >+    nscoord widthAdjustment = 0;
> >+    // If we don't have a caption frame, then the table margins belong
> >+    // to the outer table frame (because the margins of the inner table
> >+    // frame will always be zero).  We have to make up for this by
> >+    // adjusting the width returned from ComputeSize.
> >+    if (!caption) {
> >+      widthAdjustment = tableMargin.LeftRight();
> >+    }
> 
> Why is this code specific to not having a caption frame?  And why
> do you need to do this at all?

This is necessary because of the way nsTableOuterFrame::ComputeAutoSize works.  For tables with caption frames, it accounts for the margin of the inner table frame in order to calculate the correct total (possibly collapsed) margin.  For tables without caption frames, this is not necessary.  This is fine for in-flow layout, but when accounting for the table placement in the situation where we have adjacent flowing frames, this breaks.  The width adjustment handles this exceptional case.

> nsBlockReflowState.cpp:
> 
> Given this:
> >     // We pass in aReplacedWidth to make handling outer table frames
> >     // work correctly.  For outer table frames, we need to subtract off
> >     // the margin that's going to be at the edge of them, since we're
> >     // dealing with margin that it's really the child's responsibility
> >     // to place.
> can you now remove the aReplacedWidth parameter entirely (and the code
> in ComputeBlockAvailSpace that uses WidthToClearPastFloats in order to
> pass it)?  I think that would simplify this and hopefully avoid the need
> for the changes mentioned above and the changes you now have in this
> file.

No, that would be wrong, since the comment itself is still valid, and doing that would break the positioning of tables in presence of adjacent floating frames (I did test it to make sure).

> nsHTMLReflowState.cpp:
> 
> >+  if (IsOutsideCaptionFrame(frame)) {
> >+    mCBReflowState = parentReflowState->mCBReflowState;
> >+  }
> >   // Absolutely positioned frames should always be kids of the frames that
> >   // determine their containing block, except for inner table frames.
> >-  if (parentReflowState->frame == frame->GetContainingBlock() ||
> >-      (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {
> >+  else if (parentReflowState->frame == frame->GetContainingBlock() ||
> >+           (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {
> 
> Why are you doing this?  Isn't the plan to switch to the margin on the
> table outer frame in all modes?  I don't see why outside captions should
> be an exception here.

That would be the wrong thing to do, according to <http://www.w3.org/TR/2008/REC-CSS2-20080411/tables.html#q6>.  The outside captions should behave like regular block elements above or under the table, so they should get their CB from the outer table frame.

> >-      if (isBlock && !IsSideCaption(frame, mStyleDisplay))
> >+      // Exclude inline tables from the block margin calculations
> >+      if (isBlock && !IsSideCaption(frame, mStyleDisplay) &&
> >+          frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE)
> 
> Given the isBlock test this looks like a no-op change.

No, since mFrameType for inner table frames would be NS_CSS_FRAME_TYPE_BLOCK.

> Instead of testing aChildFrame->GetType(), test equality against
> mInnerTableFrame.

ChildShrinkWrapWidth is not a member, that would lead to an ugly cast.  Do you still want me to do that?

> Secondly, though, I don't see why you need this change to
> ChildShrinkWrapWidth at all.  If the margins are now on the outer
> table frame, the shrink wrapping behavior should work that way too.
> Does that not work for some reason?

That would cause us to get the wrong result from nsTableFrame::ComputeSize.

> (Second, if we needed them, we
> already have them in the calling function; recomputing them is
> unnecessarily expensive.)

Do we?

> The changes to nsTableOuterFrame::ComputeAutoSize that add margin also
> all appear to be wrong.  ComputeAutoSize returns content-box sizes.

Oh, well, in that case, I may need to rework a large portion of the patch.  :(

> I'm also not sure why you're doing what you're doing in
> GetCaptionOrigin, GetInnerOrigin, and Reflow.  Didn't we decide to
> switch to the CSS 2.1 margin model for all sides?

I think those need to change because of the above comment.  But about the margin model, what do you think about what CSS2 says (regarding top/bottom captions behaving like block boxes adjacent to the inner frame)?

And if we do want to change the margin behavior, doing that involves changes to the current behavior, right?  In that case, what do you think about leaving that part to another bug, and let this bug keep the existing behavior?

Honestly, my personal opinion after working on this bug is that we should just remove support for top-outside/bottom-outside captions, as apparently we're the only ones supporting it, and it's replaced by the CSS2.1 top/bottom caption specs.

I've addressed the rest of the comments, but I won't ask for review now since I expect that the fixes to nsTableOuterFrame::ComputeAutoSize will change large portions of the patch.

	(no longer active) Assignee
	Comment 37 • 13 years ago

This patch should address all of the review comments that were left out of the previous revision.

	(no longer active) Assignee
	Comment 38 • 13 years ago

dbaron: ping?

	(no longer active) Assignee
	Comment 39 • 13 years ago

dbaron, could you please give me an ETA on the review here?  I'd really like this to land sooner than later...

	David Baron :dbaron:
	Comment 40 • 13 years ago

(In reply to comment #36)
> (In reply to comment #35)
> > nsBlockReflowState.cpp:
> > 
> > Given this:
> > >     // We pass in aReplacedWidth to make handling outer table frames
> > >     // work correctly.  For outer table frames, we need to subtract off
> > >     // the margin that's going to be at the edge of them, since we're
> > >     // dealing with margin that it's really the child's responsibility
> > >     // to place.
> > can you now remove the aReplacedWidth parameter entirely (and the code
> > in ComputeBlockAvailSpace that uses WidthToClearPastFloats in order to
> > pass it)?  I think that would simplify this and hopefully avoid the need
> > for the changes mentioned above and the changes you now have in this
> > file.
> 
> No, that would be wrong, since the comment itself is still valid, and doing
> that would break the positioning of tables in presence of adjacent floating
> frames (I did test it to make sure).

So the idea of WidthToClearPastFloats and ComputeReplacedBlockOffsetsForFloats is the following:  Replaced elements and elements that establish new block formatting contexts (basically, anything into which it doesn't make sense to propagate the regions of float exclusions) are displaced by floats much like wrapping text is displaced.  However, what is displaced is the *border* box of such elements, not the margin box.  So when floats are present we might need to:
 * move the element down lower where there are fewer floats (i.e., they take up less width)
 * inset the available width by the float displacement minus the element's margin, so that when it is laid out (with its margin), it's as though the element's border box was displaced by the floats
Therefore, WidthToClearPastFloats returns three separate pieces of information:  a border-box width, a left margin, and a right margin.

Tables are among the most important cases for this behavior.  Prior to the patch in this bug, the margin of a table is actually on the inner table frame, but this code displaces the outer frame instead, which requires extra complication.  I think the current structure of the way the contents of the two methods are separated is part of the horrible hack to get them to handle tables correctly.  I think after this patch we'll be able to substantially clean up how these methods are called.  But I'm not going to ask that you do that.


However, what I was asking was that you at least notice the following premises:

First, notice that your patch, by removing the special table handling in nsBlockFrame::WidthToClearPastFloats, creates the (new) invariant that the result of WidthToClearPastFloats has marginLeft and marginRight members that always match an nsCSSOffsetState.

Second, notice that the aReplacedWidth argument to ComputeReplacedBlockOffsetsForFloats is optional in some cases (note how nsBlockReflowState::ComputeBlockAvailSpace calls it) -- it's only required when the frame we're computing for is a table outer frame, since that's the only frame for which (prior to your patch) the invariant above might not be true.

Third, notice that your patch introduces a bug in that it starts ignoring margins in ComputeReplacedBlockOffsetsForFloats when the frame is not a table outer frame and the aReplacedWidth is not passed in.

and then based on these premises do the following:

First, based on the first and second premises above, *remove* the aReplacedWidth parameter to ComputeReplacedBlockOffsetsForFloats and make ComputeReplacedBlockOffsetsForFloats have the behavior that it had, prior to your patch, when *null* was passed for that parameter (i.e., always use the margins in the |nsCSSOffsetState os| that it constructs, which is fine based on the first premise above).

Second, remove the code in nsBlockReflowState::ComputeBlockAvailSpace that calls WidthToClearPastFloats solely to compute the |aReplacedWidth| parameter to ComputeReplacedBlockOffsetsForFloats.

Third, add a test that would fail given the bug you introduced (third premise above).  This is probably easiest to do using a display:block image, with a margin, wrapping around a float.  Your patch would introduce the bug that the margin box wraps around the float instead of the border box (i.e., that the margin shows up as a gap when it should not).  (It's not surprising that we don't have a test for this -- most of the Web compat cases for this behavior involve tables, and in the latest patch you break it for everything except tables.)

	David Baron :dbaron:
	Comment 41 • 13 years ago

An example of such a test would be to test that in this case, the image is indented 200px (compare, say, to the same without the float, but with a 200px margin and perhaps without display:block):

<!DOCTYPE html>
<style>
div { float: left; width: 200px; height: 200px }
img { display: block; margin-left: 100px; }
</style>
<div></div>
<img src="image that's reliably taller than a line of text">

... but when its margin is 300px it is indented 300px.

	David Baron :dbaron:
	Comment 42 • 13 years ago

(In reply to comment #36)
> > >-      if (isBlock && !IsSideCaption(frame, mStyleDisplay))
> > >+      // Exclude inline tables from the block margin calculations
> > >+      if (isBlock && !IsSideCaption(frame, mStyleDisplay) &&
> > >+          frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE)
> > 
> > Given the isBlock test this looks like a no-op change.
> 
> No, since mFrameType for inner table frames would be NS_CSS_FRAME_TYPE_BLOCK.

Why?  Not based on my reading of nsHTMLReflowState::InitFrameType.

	David Baron :dbaron:
	Comment 43 • 13 years ago

(In reply to comment #36)
> > nsHTMLReflowState.cpp:
> > 
> > >+  if (IsOutsideCaptionFrame(frame)) {
> > >+    mCBReflowState = parentReflowState->mCBReflowState;
> > >+  }
> > >   // Absolutely positioned frames should always be kids of the frames that
> > >   // determine their containing block, except for inner table frames.
> > >-  if (parentReflowState->frame == frame->GetContainingBlock() ||
> > >-      (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {
> > >+  else if (parentReflowState->frame == frame->GetContainingBlock() ||
> > >+           (NS_FRAME_GET_TYPE(mFrameType) == NS_CSS_FRAME_TYPE_ABSOLUTE)) {
> > 
> > Why are you doing this?  Isn't the plan to switch to the margin on the
> > table outer frame in all modes?  I don't see why outside captions should
> > be an exception here.
> 
> That would be the wrong thing to do, according to
> <http://www.w3.org/TR/2008/REC-CSS2-20080411/tables.html#q6>.  The outside
> captions should behave like regular block elements above or under the table,
> so they should get their CB from the outer table frame.

"very much as if" is not the same as "exactly as if"; in this case I'd prefer to apply the change that CSS 2.1 made to them as well.


> > Instead of testing aChildFrame->GetType(), test equality against
> > mInnerTableFrame.
> 
> ChildShrinkWrapWidth is not a member, that would lead to an ugly cast.  Do
> you still want me to do that?

It's ok either way, then.

> > Secondly, though, I don't see why you need this change to
> > ChildShrinkWrapWidth at all.  If the margins are now on the outer
> > table frame, the shrink wrapping behavior should work that way too.
> > Does that not work for some reason?
> 
> That would cause us to get the wrong result from nsTableFrame::ComputeSize.

Ah, I think what you really need to do here is reduce the aAvailableWidth.  Since the outer frame now has margins, the available width for its children is not the same as the available width for it.  Basically, the first thing in nsTableOuterFrame::ComputeAutoSize should be to initialize:
  nscoord kidAvailableWidth = aAvailableWidth - aMargin.width;
  NS_ASSERTION(aBorder == nsSize(0,0) && aPadding == nsSize(0,0), "...");

> > (Second, if we needed them, we
> > already have them in the calling function; recomputing them is
> > unnecessarily expensive.)
> 
> Do we?

The only caller of ChildShrinkWrapWidth is nsTableOuterFrame::ComputeAutoSize, which is passed its own margin in aMargin.

	David Baron :dbaron:
	Comment 44 • 13 years ago

Comment on attachment 542580 [details] [diff] [review]
Patch (v4)

Comments on the remainder of the patch not addressed in comments above (comment 40 through comment 43):

>Outer table frames act as CSS2.1 table wrapper boxes.  We use to lay them out without

Please wrap the wrapped part at less than 80 characters.

s/use to/used to/

>-    val->SetAppUnits(mInnerFrame->GetUsedMargin().Side(aSide));
>+    // For tables, GetUsedMargin always returns an empty margin, so we
>+    // should read the margin from the outer table frame instead.
>+    nscoord value = mInnerFrame->GetUsedMargin().Side(aSide);
>+    if (mInnerFrame != mOuterFrame) {
>+      NS_ASSERTION(value == 0, "Inner table frames don't have margins");
>+      value += mOuterFrame->GetUsedMargin().Side(aSide);
>+    }
>+    val->SetAppUnits(value);

Why not just simplify this to:
  val->SetAppUnits(mOuterFrame->GetUsedMargin().Side(aSide));
  NS_ASSERTION(mOuterFrame == mInnerFrame ||
               mInnerFrame->GetUsedMargin() == nsMargin(0, 0, 0, 0),
               "inner tables must have zero margins");

>     // Size the table and the caption independently.
>+    // Captions should be reflowed in the available width of the outer table frame.
>     OuterBeginReflowChild(aPresContext, mCaptionFrame, aOuterRS,
>-                          captionRSSpace, aOuterRS.ComputedWidth());
>+                          captionRSSpace, aOuterRS.availableWidth);

And, as I said above, I think we should leave this as it was and put
the margin outside the wrapper box for top-outside and bottom-outside
just like for the others.


I'm a lot happier with this version than the last one, but I'd like to look again (and hopefully faster).

	(no longer active) Assignee
	Comment 45 • 13 years ago

(In reply to David Baron [:dbaron] from comment #41)
> An example of such a test would be to test that in this case, the image is
> indented 200px (compare, say, to the same without the float, but with a
> 200px margin and perhaps without display:block):
> 
> <!DOCTYPE html>
> <style>
> div { float: left; width: 200px; height: 200px }
> img { display: block; margin-left: 100px; }
> </style>
> <div></div>
> <img src="image that's reliably taller than a line of text">
> 
> ... but when its margin is 300px it is indented 300px.

I added this test, but it passes both before and after my patch.

	(no longer active) Assignee
	Comment 46 • 13 years ago

(In reply to David Baron [:dbaron] from comment #42)
> (In reply to comment #36)
> > > >-      if (isBlock && !IsSideCaption(frame, mStyleDisplay))
> > > >+      // Exclude inline tables from the block margin calculations
> > > >+      if (isBlock && !IsSideCaption(frame, mStyleDisplay) &&
> > > >+          frame->GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_INLINE_TABLE)
> > > 
> > > Given the isBlock test this looks like a no-op change.
> > 
> > No, since mFrameType for inner table frames would be NS_CSS_FRAME_TYPE_BLOCK.
> 
> Why?  Not based on my reading of nsHTMLReflowState::InitFrameType.

Not any more.  :-)  Please see my mq patches <http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/>

	David Baron :dbaron:
	Comment 47 • 13 years ago

(In reply to David Baron [:dbaron] from comment #40)
> Third, notice that your patch introduces a bug in that it starts ignoring
> margins in ComputeReplacedBlockOffsetsForFloats when the frame is not a
> table outer frame and the aReplacedWidth is not passed in.

I think I may have been misreading the "!= nsGkAtoms::tableOuterFrame" in that function as an ==.  That would explain why I thought that, anyway, at least based on my ability to follow my own comments two months later.

	(no longer active) Assignee
	Comment 48 • 13 years ago

I think I've addressed all of the review comments in this version of the patch.  Let me know if I've missed something.

Note that even with this patch, there are two new test failures on the try server, both of which were added in http://hg.mozilla.org/mozilla-central/rev/2676a94cee76.  I still need to investigate those.

	(no longer active) Assignee
	Comment 50 • 13 years ago

This version of the patch fixes the code added in bug 642088 to stop collapsing caption and table margins.  Other than that, it's the same as v5.

	David Baron :dbaron:
	Comment 51 • 13 years ago

Comment on attachment 562145 [details] [diff] [review]
Patch (v5)

+    // Outer table frames inherit the margins from the table itself,
+    // and accounting them here breaks the offset calculations.

I think this is a little misleading since it makes it sounds like
you're doing something special for outer table frames.  Perhaps:

  // Since outer table frames inherit the margin from the table itself,
  // we don't need any special handling for tables here.

... or maybe even just no comment at all.


I'm puzzled as to why nsTableOuterFrame::ComputeAutoSize is still using
aAvailableWidth in some cases.  Shouldn't it use kidAvailableWidth
throughout?  The table's margins go outside the caption's margins.


You missed this bit of comment 44 (though you made the corresponding
changes to nsHTMLReflowState.cpp):
>>     // Size the table and the caption independently.
>>+    // Captions should be reflowed in the available width of the outer table frame.
>>     OuterBeginReflowChild(aPresContext, mCaptionFrame, aOuterRS,
>>-                          captionRSSpace, aOuterRS.ComputedWidth());
>>+                          captionRSSpace, aOuterRS.availableWidth);
>
>And, as I said above, I think we should leave this as it was and put
>the margin outside the wrapper box for top-outside and bottom-outside
>just like for the others.

r=dbaron with those things fixed

	David Baron :dbaron:
	Comment 52 • 13 years ago

Comment on attachment 562880 [details] [diff] [review]
Patch (v6)

r=dbaron with the things in comment 51 addressed

	(no longer active) Assignee
	Comment 53 • 13 years ago

With these patches, I _think_ that my work on bug 10209 might be ready to land.  I've pushed another try server job to see if there are any remaining test failures I need to take care of, but I'm crossing my fingers.  :-)

	Mozilla RelEng Bot
	Comment 54 • 13 years ago

Try run for 674011997a64 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=674011997a64
Results (out of 73 total builds):
    exception: 8
    success: 56
    failure: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-674011997a64

	(no longer active) Assignee
	Comment 55 • 13 years ago

With dbaron's comments addressed.

	Mozilla RelEng Bot
	Comment 56 • 13 years ago

Try run for 25d2329ddc2c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=25d2329ddc2c
Results (out of 234 total builds):
    exception: 1
    success: 219
    warnings: 13
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-25d2329ddc2c

	(no longer active) Assignee
	Comment 57 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/2d5d6e1c4418
https://hg.mozilla.org/mozilla-central/rev/978da009e89b
https://hg.mozilla.org/mozilla-central/rev/0d3d4cc835ea