363248 - [reflow branch] Table caption regression

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Mats Palmgren (inactive)]

[reflow branch] Table caption regression.

STEPS TO REPRODUCE
1. load the attached testcase

Bug occurs in Firefox 2006120801 on Linux
Bug does not occur in Firefox 2006120701 on Linux

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

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

Well, that is what CSS2 says to do, and it was a good bit easier to change towards doing that than to doing what 2.1 says.  (Before we did neither, but a good bit closer to 2 except for this case.)

Assuming you're saying that the bug is the caption being in the middle of the page, that is.  You didn't actually say what the bug is.

Comment 3

[Mats Palmgren (inactive)]

I think the caption boxes should shrink-wrap its text and
be placed flush to the table edge specified by 'caption-side'.
The only one I consider correct is caption-side:right.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Same testcase with borders.

Comment 5

[Mats Palmgren (inactive)]

Hmm, actually reading CSS2 and CSS2.1 it seems you are right.
http://www.w3.org/TR/CSS2/tables.html#propdef-caption-side
http://www.w3.org/TR/CSS21/tables.html#propdef-caption-side

Since CSS 2.1 removed left/right, maybe we should keep the old rendering
for those two at least? (ie. the bug is only about caption-side:left)

Comment 6

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

Oh, yeah, left is definitely broken.  I wrote code to size to min-width; apparently it's not working.

Comment 7

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

Oh, er, it's placement that's not working.  That's the code in nsTableOuterFrame that I could never quite understand...

Comment 8

[Bernd]

David, I guess I can help with "That's the code in
nsTableOuterFrame that I could never quite understand...". Could you please define what should happen for the left and right case.

Comment 9

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

For the left and right cases the caption and the table should be next to each other.

Comment 10

[Mats Palmgren (inactive)]

Attachment: hack

The problem seems to be the caption right margin.  FWIW, this hack fixes
the bug, just as an illustration...

Comment 11

[Bernd]

Isn't the top and bottom case also wrong? Why does it span the whole containing block?

"The width of the anonymous box is the border-edge width of the table box inside it" 

Would make for me the box as large the table. 

"The caption boxes are block-level boxes that retain their own content, padding, margin, and border areas, and are rendered as normal blocks inside the anonymous box"

Would make the caption to fill the box entirely.

This understanding is BTW exactly what Opera9 is doing.

Or is that sentence from the example normative:

"In this example, the 'caption-side' property places captions below tables. The caption will be as wide as the parent of the table, and caption text will be left-justified."

Maybe I'm to stupid to read the spec right, but what I read is exactly what Opera does and i swear I did not touch their code.

Comment 12

[Gérard Talbot]

OS -> All

Comment 13

[Bernd]

David, I would love to work on this, but I need some guidance from you what you would like to have implemented. Thats first the questions from comment 11 and further how the margins should apply with the left and right scenario.
For instance if a table is centered via auto margins does a side caption influenc the position?

Comment 14

[Bernd]

Attachment: wip

this fixes the top caption for me
I am not sure that one needs to call Outerreflowchild again.

Comment 15

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

I don't have strong opinions here, and the spec has changed a good bit recently, so if you can implement what it now says, that's great.

Comment 16

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

That said, when the CSS WG drastically changed the behavior of top and bottom captions for CSS2.1, we agreed that css3 would have keywords for the old CSS2 behavior as well (since I think most of us agreed that it was better; just that Mozilla was the only one that tried to implement it (although not very well)).

So it might be worth implementing the good top/bottom caption behavior from CSS2 as -moz-* keywords rather than just removing it.

Comment 22

[Lasse Marøen]

Any chance of getting the top/bottom fix in Firefox 3? My site is affected by this bug :-(

Comment 23

[sayenko]

Attachment: Testcase #3 - Version 3 Beta 3 2008020514

Caption is wider then table. Was fine in version 2, broken in version 3.

Comment 24

[Mike Schroepfer]

DBaron/mats what do you want to do here?  This bug has been open for > 1 year and we are closing down for 1.9...

Comment 25

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

I've been looking at bug 386704 and this bug the past day or two.

Comment 26

[Damon Sicore (:damons)]

Assigning to dbaron per Comment #25.

Comment 27

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

Attachment: patch 1 of 4: refactor OuterReflowChild

This is patch 1 of 4 for this bug.  Patches 1, 2, and 4 fix the problem that is also described in bug 386704 and some of the duplicates.  (This bug is about multiple problems that should have been split into separate bugs initially.)

This patch is not intended to change behavior.  It splits OuterReflowChild into two functions so that I can (in patch 2) call them at separate times and change the order of when we construct the reflow states for the inner and caption frames without changing the order of when we reflow them.


Bernd -- you're welcome to look at all of these if you want, and I'll update them to reflect any comments you have, but if roc's ok with reviewing them, I'm not planning on waiting for your comments if you're unable to look soon.

Comment 28

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

Attachment: patch 2 of 4: fix top/bottom caption sizing, and split old behavior into top-outside/bottom-outside

split old top/bottom sizing behavior into top-outside/bottom-outside (as mentioned in CSS 2.1; not sure whether I need -moz- on them but I'm inclined to leave it off since the names are in a CSS 2.1 CR and the definitions in CSS 2.0), and implement the backwards-compatible caption sizing behavior

Comment 29

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

Attachment: patch 3 of 4: fix margin expansion for side captions

This is much like Mats's hack in attachment 248061 [details] [diff] [review], and is pretty much independent of the other three patches.  I added some comments to code that confused me a little while I was poking around.

Comment 30

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

Attachment: patch 4 of 4: fix horizontal positions of top/bottom captions

This probably should have been part of patch 2, but I have it separate, and it might be easier to review that way.  This fixes things so that the widths produced by patch 2 don't produce incorrect horizontal positions (although I'm pretty sure it doesn't work well with the combination of RTL and explicit widths -- although we're significantly better on RTL cases than Firefox 2).

Comment 31

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

Attachment: testcases I was looking at (LTR)

These should really all (and more, as noted by the "to do" list at the bottom) be turned into reftests, but I'll probably just add some minimal reftests for the bugs being tested.

Comment 32

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

Attachment: testcases I was looking at (RTL)

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 305670 [details] [diff] [review]
patch 2 of 4: fix top/bottom caption sizing, and split old behavior into top-outside/bottom-outside

+  case NS_STYLE_CAPTION_SIDE_BOTTOM:
+  case NS_STYLE_CAPTION_SIDE_BOTTOM_OUTSIDE: {
     if (NS_AUTOMARGIN == aCaptionMargin.left) {
       aCaptionMargin.left = CalcAutoMargin(aCaptionMargin.left, aCaptionMargin.right,
                                            aContainBlockSize.width, aCaptionSize.width);

I don't really get how auto margins work for top and bottom captions. The margin values we compute here would center the caption in the containing block. Does that mean that the caption can be centered in the containing block, somewhere not under its table, while its width is constrained to the table width? That would seem a bit odd.

Assuming you have a reasonable explanation of that (which I assume strongly!), this looks good.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 305671 [details] [diff] [review]
patch 3 of 4: fix margin expansion for side captions

+      PRUint8 captionSide;
+      if (isBlock &&
+          // don't do this for side captions
+          (mStyleDisplay->mDisplay != NS_STYLE_DISPLAY_TABLE_CAPTION ||
+           ((captionSide = frame->GetStyleTableBorder()->mCaptionSide) !=
+              NS_STYLE_CAPTION_SIDE_LEFT &&
+            captionSide != NS_STYLE_CAPTION_SIDE_RIGHT)))

This would be much clearer using a helper function IsSideCaption(nsStyleDisplay*, nsIFrame*)

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 305672 [details] [diff] [review]
patch 4 of 4: fix horizontal positions of top/bottom captions

looks good except I still don't understand how we get the right horizontal caption margins for non-outside top/bottom captions.

Comment 36

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

(In reply to comment #33)
> I don't really get how auto margins work for top and bottom captions. The
> margin values we compute here would center the caption in the containing block.
> Does that mean that the caption can be centered in the containing block,
> somewhere not under its table, while its width is constrained to the table
> width? That would seem a bit odd.
> 
> Assuming you have a reasonable explanation of that (which I assume strongly!),
> this looks good.

I don't either.  I don't think that code makes any sense, but I didn't want to attack margins in these patches any more than I had to.  I'd like to be able to look into it at some point in the future, though...

(In reply to comment #35)
> looks good except I still don't understand how we get the right horizontal
> caption margins for non-outside top/bottom captions.

What in particular?  They're supposed to be inside the table margins (just like the available space is).  But I haven't tested much relating to margins.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Here's the testcase I'm thinking of. With your patches, will the caption's margins be computed to center it in the containing block, and then we add that computed left margin to the inner table's left margin?

Even if we do, I'd much rather land these patches than worry about margins on the caption.

Comment 38

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

No, that case is fine, since patch 2 above changes nsTableOuterFrame::Reflow so that it passes the narrow available width to the reflow state constructor.  nsContainerFrame::ComputeAutoSize and nsHTMLReflowState::CalculateBlockSideMargins both use that available width for computing width and margins, so in that case the auto margins end up computing to zero and something slightly negative (since we hit the overconstraint path), so everything is fine.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK great.

Comment 40

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

Above patches checked in to trunk, 2008-02-26 18:01/02/02/03 -0800.

Also had to undo two known-fail annotations, one for mozilla/content/base/test/test_bug414190.html and one for mozilla/layout/reftests/bugs/134706-6b.html, plus fix the reference rendering for mozilla/layout/reftests/bugs/386014-1-ref.html which depended on the wide top/bottom caption behavior.

Still want to check in reftests and look into margins, but marking fixed.

Comment 41

[Gérard Talbot]

Table caption does not maintain same width as the table when one or many table <col> or <colgroup> is/are collapsed. Firefox 2.0.0.12 did maintain same width.

Steps: Tests 5 and 7 in
attachment 147933 [details]
now fail wrt caption width in rv:1.9b4pre build 2008022801

---------

I'd like to understand why the table caption does not take as much horizontal space as it can when caption-side is set to left (or right):

Steps:
load
http://www.gtalbot.org/DHTMLSection/DynamicTableFormatting.html
and then in the Caption side select, choose Left