3166 - Caption-align-left and caption-align-right do not work ('caption-side')

Status: RESOLVED FIXED

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

Description

[Christine Hoffman]

Using 2/11 build on Win 95, Win NT, Win 98, Kinux, Mac8.5.

Open URLs above.

Expected results:
(1) http://slip/projects/marvin/html/tables_caption_align_left.html:
Caption should align to the left at the top of the table.

(2) http://slip/projects/marvin/html/tables_caption_align_right.html:
Caption should align to the right at the top of the table.

Actual results:
(1) and (2): Captions do not align to left or right.

Comment 1

[karnaze (gone)]

We may not be supporting this for a while.

Comment 2

[karnaze (gone)]

This has been cut as a 1.0 feature.

Comment 3

[rubydoo123]

when was the cut decided and who decided it?

Comment 4

[karnaze (gone)]

It was decided because (1) there is not time before 5/18 to complete it and (2)
IE doesn't really support it yet either.

Comment 5

[rubydoo123]

so does that mean it will go in at a later milestone? Or did
Saito/Krock/Leger/Hofmann agree to the de-commitment?

Comment 6

[leger]

ekrock, agreed?  Feature coming out?

Comment 7

[ekrock's old account (dead)]

Yes, it's officially out for 1.0.

Comment 8

[Christine Hoffman]

Verified LATER.

Comment 9

[Hixie (not reading bugmail)]

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

Comment 10

[Hixie (not reading bugmail)]

Migrating from {css2} to css2 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...

Comment 11

[Hixie (not reading bugmail)]

Reopening and moving to Future...

Comment 12

[Bernd Mielke]

The following URL is also visible outside netscape:

http:
//lxr.mozilla.org/seamonkey/source/layout/html/tests/table/marvin/tables_caption_align_left.html

adding testcase keyword, as marvin cases seem to be valid tests that should not
appear in the bugathon query.

Comment 13

[Hixie (not reading bugmail)]

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

Comment 14

[Hixie (not reading bugmail)]

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

Comment 15

[Bernd Mielke]

Adding Keyword 4xp as IE5 handles both testcases correctly.

Comment 16

[ekrock's old account (dead)]

Removed 4xp keyword. 4xp means "Nav4 did this and we need to be compatible." 
Click the Keywords link to see the definitions.

Comment 17

[Bernd Mielke]

from keywords.cgi: Product Parity bugs. A bug is a Product Parity bug if it 
occurs on Mozilla builds, but does not occur using the latest release of 
Netscape Communicator 4.x or *competitor products*. 

The bug does not occure at a competitor product. I hope they are still 
competitors???

Comment 18

[ekrock's old account (dead)]

I'm not sure when "competitor products" was added there or why. What I *think* 
they mean by competitor products in this context is situations like: suppose 
feature X worked in both Nav4 and IE4. Then we'd want it to work in Mozilla. But 
this doesn't mean that in the extremely rare cases that IE5.5 implements a 
standards feature that Mozilla currently doesn't (and such cases are outnumbered 
a thousand to one by cases where we support the standard and they don't--just 
think about DOM1/2, etc.) that such bugs should be marked 4xp--because Nav4x 
didn't actually support it.

Hope that clarifies things. Best is to think of 4xp as a synonym for 
"compatibility with Nav4.x." If the keywords definition is causing confusion, it 
needs to be clarified.

Comment 19

[Hixie (not reading bugmail)]

ekrock: I disagree. I understood the 4xp keyword to mean exactly what the 
definition says -- we have a bug that a released browser does not. If this is
not what was intended, then we have several hundred bugs to recheck.

Don't forget Mozilla is almost a complete rewrite from the Nav 4.x series. From
a mozilla.org point of view, Nav 4.x is just as relevant as WinIE, MacIE, Opera,
Konqueror, et al. Why would mozilla.org worry about being better than the Nav 4
series in particular?

IMHO 4xp should be reinstated on this bug.

Comment 20

[Christine Hoffman]

Attaching a testcase (original ones have a broken link). Testing with Win 98, 
Nav and Netscape 6 do not render 'caption align="left"' or 'caption 
align="right"'. IE renders them differently that the spec indicates (they left 
or right align the caption at the top). See spec at: 
http://www.w3.org/TR/html4/struct/tables.html#h-11.2.2

Comment 21

[Christine Hoffman]

Attachment: testcase to replace originals

Comment 22

[HJ]

Bug 63966 is a duplicate of this bug. I have made a clear and CSS/HTML 4.01
validated testcase. Also a screenshot from Opera 5.0 is provided.

It really looks like you are waiting for february 16 2001 to come first!

Comment 23

[Doron Rosenberg (IBM)]

as our goal is to create a browser which is as standard-compliant as possible,
nominating for 0.9. doubt this would be a nsbeta1 candidate

Comment 24

[Doron Rosenberg (IBM)]

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

Comment 25

[HJ]

CC:ing sorry for the spam

Comment 26

[Bernd]

Attachment: caption side right not working

Comment 27

[Bernd]

The dupe bug 10216 was dealing also with the caption-side property. Not
implementing the caption-side property makes it impossible to create a table
accordingly to CSS2 sect. 17.4.01.

Comment 28

[Amarendra Hanumanula]

QA contact update

Comment 29

[Bernd]

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

Comment 30

[rubydoo123]

removing myself from the cc list

Comment 31

[José Jeria]

Attachment: Caption does not stay centered

This testcase is weird, cause if you remove the line changing the rules its
stayed centered. Removing only the other line instead also makes the caption
stay in the correct position.

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

Comment 33

[Jerry]

Attachment: patch 0.1

Comment 34

[Jerry]

I think this bug is caused by uncorrectly use parsent's nsHTMLReflowState.

when nsTableOuterFrame reflow its children--nsTableCaptionFrame, it pass its 
nsHTMLReflowState to it, but nsTableCaptionFrame has its only style.

So my patch is just calculate captionframes's align value, set 
captionReflowState's mStyleText according to it.

I an new to reflow, any suggestion is welcome.

Comment 35

[Håkan Waara]

Comment on attachment 87332 [details] [diff] [review]
patch 0.1

>+        if (alignValue.Equals(NS_LITERAL_STRING("left"))) styleText->mTextAlign = NS_STYLE_TEXT_ALIGN_LEFT;
>+        if (alignValue.Equals(NS_LITERAL_STRING("right"))) styleText->mTextAlign = NS_STYLE_TEXT_ALIGN_RIGHT;
>+        captionReflowState.mStyleText = styleText;

Please change this to be:

if (alignValue...)
  styleText->...
else if (alignValue)
  styleText->...

Comment 36

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

Comment on attachment 87332 [details] [diff] [review]
patch 0.1

This patch is completely incorrect, for the following reasons:

 1. Caption alignment means that the caption should be on a different side of
the table, not that its contents should be text-aligned.  See
http://www.w3.org/TR/html4/struct/tables.html#h-11.2.2
 2. The attribute mapping of the alignment attributes is already done
correctly, in MapAttributesIntoRule in nsTableCaptionElement.cpp, but they're
mapped into the caption-side property (as they should be), not the text-align
property.
 3. The nsStyleText struct that you're trying to modify (and on which you're
casting away const) is immutable -- it may be shared with other elements, and
you don't want to modify the data for those elements.

Comment 37

[Jerry]

I know my patch is not correct, so mark it as obsolete.

but to dbaron's comment,I have some question about it.
1.so what do you think of this testcase should be?
<TABLE border="1" width="200" ><CAPTION align="right">A test table 
</CAPTION><TR><TD>123</TABLE></HTML>

Using IE,  the text "A test table" will be placed on the top of table,right 
aligned.
but with mozilla, the text "A test table" will be placed on the top center of 
table.

2.So how to understand the meaning of 
"left: The caption is at the left of the table." in html4 spec.
before my understanding is that the text of caption will be left aligned.

Comment 38

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

No, it means that the entire caption will be to the left of the table.  IE is
probably incorrect.  In other words, attachment 22369 [details] should look like:


The table's    Data   Data
caption.       Data   Data

(and the 'caption-side: right' there is equivalent to the align="right"
attribute on a caption element).

Comment 39

[Jerry]

i got one idea about it.
since mozilla, opera do not support caption-align, and IE implenments this in
its own way.
can mozilla actions just like IE in quirk mode?

Comment 40

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

We should consider quirks in quirks mode if the quirk is needed to fix the
display of real pages on the web.  (My typical definition of real pages is those
where the bugs are reported by someone other than the author of the page.)  Is
this feature actually used on the web?  I doubt it.

Comment 41

[Bernd]

I go with dbaron's oppinion. We have the groundwork to get this working as
specified in the spec already in nsTableOuterFrame.cpp. And once I have my
buglist shrinked I will certainly go to implement this correctly. So please
don't hork that with a quirk. 

Comment 42

[Bernd]

it looks to me like the w3c will drop caption-side:left and right from the css
2.1 spec.

Comment 43

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

... but only for lack of implementation.  It doesn't make this bug any less valid.

Comment 44

[Bernd]

I have a running prototype, taking the bug.

Comment 45

[Bernd]

Attachment: implementation proposal

Comment 46

[Jerry]

bernd, in your implementation proposal img,
I know its (x,y, w),
but don't know its height, 

maybe its h can be the same as Table body,
but it will be ugly if it's a big table.

maybe its h can be the first row height?
I dont know.

Comment 47

[Bernd]

Attachment: the css2 testcase

Comment 48

[Bernd]

Attachment: rendering proposal

Comment 49

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

The fun part is what to do for 'auto' widths on right and left captions. :-)

The optimal solution, would, I think, be to use the minimum width that doesn't
make the caption taller than the table, unless the caption wouldn't fit
horizontally in such a width.  That's quite hard, though, and I'm not sure what
the implementable alternative should be.

Comment 50

[Bernd]

David, I do my mozilla work only for fun (TM). 

The issue that I have, which worries me, is that, at
http://www.people.fas.harvard.edu/~dbaron/css/test/sec1704 the last cases imply
a different margin handling then attachment 98261 [details]. 
While I believe in my proposal, I can't see how in your margin handling one
could center a table and keep the caption attached directly to the table.
Second, wouldn't, if even if you can do that, the caption shift the center of
gravity of the whole construct to the left or right. Just imagine a windmil
testcase where the table is centered and via DOM the caption is circled around
the table. In my proposal the table would keep its position horizontally and
only move vertically depending whether the caption is top or not. Any insight
from you is very wellcome, I would you anyway ask you for sr once the thing is
working correctly.

Comment 51

[Bernd]

Attachment: patch v 0.1 WIP

This patch is a snapshot of my current tree, the following things need still to
be done: 
 DOM manipulations
 regression tests

Comment 52

[Bernd]

Attachment: patch v 0.2

Comment 53

[Bernd]

Attachment: patch

The patch passed the regression tests. Its open for discussion even r/sr.

Comment 54

[karnaze (gone)]

Attachment: previous patch with changes made while reviewing

Bernd, you've done a great job with this. The patch I'm attaching corrects some
cosmetics, reduces the amount of code in a few places, and add "XXX BERND"
comments when I had a question or thought there might be a problem. I didn't
run this patch	through the regression tests, but I don't think the changes
I've made are serious enough to cause things to break. After you attach a new
patch, I would like to review it.

Comment 55

[Bernd]

Attachment: patch revisited

Comment 56

[Bernd]

Attachment: changes after the previous patch

Comment 57

[Bernd]

Comment on attachment 103466 [details] [diff] [review]
patch revisited

there is an error

Comment 58

[Bernd]

Attachment: patch revised

Comment 59

[fantasai]

I just ran through some tests with the patch, and I must say that overall I'm
very impressed, Bernd. However -

I think you should use the caption's top/bottom margins in the vertical position
calculation. e.g.
  caption {
    caption-side: left;
    vertical-align: top;
    margin-top: 5px;
  }

The top border edge of the caption should be 5px below the top of the table.

If the caption has "vertical-align: center", I think it should be 2.5px
off-center. (The margin would change its "center of gravity", as you say. If it
had "margin: 5px", the caption would be once again exactly centered.) Of course,
that's open to interpretation.

I would like some future version of CSS to allow percentage values for
vertical-align so that captions may be aligned vertically in a manner similar to
background images.


Any particular reason why centered captions longer than the table aren't centered?

Percentage heights aren't recognized.

Percentage widths give strange results. I'll attach a testcase.

Comment 60

[fantasai]

Attachment: caption % width testcase

I think percent width on a side-aligned caption should be relative to the
table's margin (i.e. available space) rather than the containing block. You can
get the latter effect by using auto width and a % margin on the table.

Comment 61

[Bernd]

Attachment: testcase proving that % heights are honoured

Comment 62

[Bernd]

Attachment: patch fixing fantsai's testcase

Comment 63

[Bernd]

I left the vertical margins intentionaly out now. I see them conflicting with
the vertical alignment and will implement that later. This patch does exactly
what is written in the CSS2.0 spec for vertical alignment. If you look at your
proposal it looks more like a proposal for w3c-style then something one should
implement now. We should go for clarification once this patch is in and there is
something to show to the w3c.

>Any particular reason why centered captions longer than the table aren't >centered?

could you provide a testcase, I don't see that.

>Percentage heights aren't recognized.

This is not the case, see the attached testcase


>I think percent width on a side-aligned caption should be relative to the
>table's margin (i.e. available space) rather than the containing block.
 
As I explained on IRC I disagree the CSS2 specs states clearly: The percentage
is calculated with respect to the width of the generated box's containing block.
And we do so for the top and bottom already. I would not change that, because I
think it is wrong.

Comment 64

[fantasai]

Attachment: long centered caption

Comment 65

[Bernd]

Attachment: patch

the patch fixes attachment 104284 [details]

Comment 66

[fantasai]

Attachment: negative margin + width testcase

The caption disappears when width <= negative margin

The disappearing text in the fifth test happens when width is given in pixels,
but not when it's given in em.

Comment 67

[fantasai]

I built with the latest patch. The centered caption testcase and the one I just
attached both work fine. However, the "caption % width testcase" has problems
with the right margin: It's still as wide as it was before the caption was
added, so the canvas is too wide.

>>Percentage heights aren't recognized.
>This is not the case, see the attached testcase

What is the containing block for a table caption that is neither above nor below
the table? Is it the anonymous table+caption box, or that box's containing block?


I was just reading through the CSS2 section on table captions, and it seems that
putting the caption in the side margins doesn't quite match the wording. (See
2nd paragraph under 17.4 and the caption-in-margin example.) I really like what
you've done here, and comment 50 has a very compelling argument for this model.
Still, I thought I should bring this up...

Comment 68

[karnaze (gone)]

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

r=karnaze

Bernd, I assume that you have a bunch of test cases to check in as regression
tests. Here are a few additional comments.


The indentation is 2px too much
+    switch (captionSide) {
+	 case NS_SIDE_LEFT:

Chage
+  if(mCaptionFrame) {

to

+  if (mCaptionFrame) {

Change
  *  There are for possible scenarios 
to
  *  There are four possible scenarios 

Change (2 places)
  default: // all others are treated as top for now
to
  default: // top

The following occurs in 2 places, can you make it a function. Also, could you
check that there isn't other similar code sections that are duplicated. 

+    if (NS_SIDE_LEFT == captionSide || NS_SIDE_RIGHT == captionSide) {
+      if (mCaptionFrame) {
+	 nsMargin capMarginIgnore;
+	 nsMargin capMarginNoAuto;
+	 nsMargin captionPaddingIgnore;
+	 GetMarginPadding(aPresContext, aOuterRS, mCaptionFrame,
aOuterRS.availableWidth, capMarginIgnore, 
+			  capMarginNoAuto, captionPaddingIgnore);
+	 PRBool isPctWidth;
+	 IsAutoWidth( *mCaptionFrame,&isPctWidth);
+	 if (isPctWidth && (eReflowReason_Initial != aOuterRS.reason)) {
+	   nsRect priorCaptionRect;
+	   mCaptionFrame->GetRect(priorCaptionRect);
+	   capMin = priorCaptionRect.width;
+	 }
+	 capMin += capMarginNoAuto.left + capMarginNoAuto.right;
+      }
+    }

Comment 69

[Bernd]

Attachment: patch with reviewers changes

Comment 70

[Bernd]

Comment on attachment 104633 [details] [diff] [review]
patch with reviewers changes

taking over the r=karnaze

Comment 71

[kinmoz]

Comment on attachment 104633 [details] [diff] [review]
patch with reviewers changes

sr=[e-mail]

Just some minor formatting nits ...


==== Removing the following will leave 3 blank lines between the 2 left over
functions, remove 2 of them to match the spacing between functions used in the
file.


-// tables change 0 width into auto, trees override this and do nothing
-NS_IMETHODIMP
-nsTableOuterFrame::AdjustZeroWidth()
-{
-  /* It does not appear that this function is needed any longer, since
-   the content node handles the deprecated width="0" attribute case. -- dwh
-  */
-  return NS_OK;
-}
+


==== I see comments like this throughout the diff, where the comment is in the
middle of the function arg list? Are these comments necessary since the code
that sets the NoAuto vars looks self explanatory?


+				     // aMargin, but with auto margins set to 0


+					 // aInnerMargin, but with auto margins
set to 0


==== Removing GetChildAvailWidth() leaves 2 blank lines above MoveFrameTo(),
remove one of the lines.



==== Are we keeping this comment for a reason? Or can it be removed?



+      nsHTMLReflowMetrics captionMet((eReflowReason_StyleChange !=
reflowReason) ? nsnull : &maxElementSize);
+      //nsHTMLReflowMetrics captionMet(nsnull);  // don't ask for MES, it
hasn't changed

Comment 72

[Bernd]

fix checked in, btek went from 1070 to 1069ms :-)

Please open new bugs if you find bugs within this implementation.

Comment 73

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: new test case showing that layout stays correct of the caption-side is dynamically changed

great work Bernd !

Comment 74

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: better mimetype, sorry for the spam

Comment 75

[timeless]

Comment on attachment 105557 [details]
new test case showing that layout stays correct of the caption-side is dynamically changed

you don't need to attach a new file just to change the mimetype. next time
click edit in the attachments area, uncheck [x] patch and enter a new mimetype!

Comment 76

[Amarendra Hanumanula]

 Verified on trunk Build: 2002112108. All the testcases works fine for " caption
align".