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

RESOLVED FIXED in Future

Status

()

Core
Layout: Tables
P3
normal
RESOLVED FIXED
20 years ago
16 years ago

People

(Reporter: Christine Hoffman, Assigned: Bernd)

Tracking

({css2, testcase})

Trunk
Future
css2, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [awd:tbl], URL)

Attachments

(14 attachments, 9 obsolete attachments)

829 bytes, text/html
Details
623 bytes, text/html
Details
558 bytes, text/html
Details
18.11 KB, image/jpeg
Details
1.05 KB, text/html
Details
6.57 KB, image/png
Details
14.47 KB, patch
Details | Diff | Splinter Review
1015 bytes, text/html
Details
2.36 KB, text/html
Details
748 bytes, text/html
Details
1.98 KB, text/html
Details
60.33 KB, patch
Bernd
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
1.43 KB, text/html
Details
1.43 KB, text/html
Details
(Reporter)

Description

20 years ago
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.

Updated

20 years ago
Status: NEW → ASSIGNED

Comment 1

20 years ago
We may not be supporting this for a while.

Updated

20 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 20 years ago
Resolution: --- → LATER
Target Milestone: M15

Comment 2

20 years ago
This has been cut as a 1.0 feature.

Comment 3

20 years ago
when was the cut decided and who decided it?

Comment 4

20 years ago
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

20 years ago
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

20 years ago
ekrock, agreed?  Feature coming out?
Yes, it's officially out for 1.0.
(Reporter)

Updated

20 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 8

20 years ago
Verified LATER.
Summary: Caption-align-left and caption-align-right do not work → {css2} Caption-align-left and caption-align-right do not work ('caption-side')
*** Bug 10216 has been marked as a duplicate of this bug. ***
Summary: {css2} Caption-align-left and caption-align-right do not work ('caption-side') → {html40}{css2} Caption-align-left and caption-align-right do not work ('caption-side')
Blocks: 7954
Summary: {html40}{css2} Caption-align-left and caption-align-right do not work ('caption-side') → {css2} Caption-align-left and caption-align-right do not work ('caption-side')
Keywords: css2
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...
Reopening and moving to Future...
Status: VERIFIED → REOPENED
Priority: P2 → P3
Hardware: PC → All
Resolution: LATER → ---
Summary: {css2} Caption-align-left and caption-align-right do not work ('caption-side') → Caption-align-left and caption-align-right do not work ('caption-side')
Target Milestone: M15 → Future

Comment 12

18 years ago
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.
Keywords: testcase

Updated

18 years ago
Status: REOPENED → ASSIGNED
*** Bug 49988 has been marked as a duplicate of this bug. ***
*** Bug 49989 has been marked as a duplicate of this bug. ***

Comment 15

18 years ago
Adding Keyword 4xp as IE5 handles both testcases correctly.
Keywords: 4xp
Removed 4xp keyword. 4xp means "Nav4 did this and we need to be compatible." 
Click the Keywords link to see the definitions.
Keywords: 4xp

Comment 17

18 years ago
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???
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.
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.
(Reporter)

Comment 20

18 years ago
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
(Reporter)

Comment 21

18 years ago
Created attachment 14620 [details]
testcase to replace originals

Comment 22

18 years ago
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!

Updated

18 years ago
Keywords: mozilla0.9
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
*** Bug 63966 has been marked as a duplicate of this bug. ***

Comment 25

18 years ago
CC:ing sorry for the spam
(Assignee)

Comment 26

18 years ago
Created attachment 22369 [details]
caption side right not working
(Assignee)

Comment 27

18 years ago
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

18 years ago
QA contact update
QA Contact: chrisd → amar
(Assignee)

Comment 29

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

Updated

17 years ago
Target Milestone: Future → mozilla1.1

Updated

17 years ago
Whiteboard: [awd:tbl][TESTCASE]

Updated

17 years ago
Target Milestone: mozilla1.1 → Future

Comment 30

17 years ago
removing myself from the cc list

Comment 31

17 years ago
Created attachment 76175 [details]
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.
*** Bug 147775 has been marked as a duplicate of this bug. ***

Comment 33

16 years ago
Created attachment 87332 [details] [diff] [review]
patch 0.1

Comment 34

16 years ago
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

16 years ago
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 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.
Attachment #87332 - Flags: needs-work+

Updated

16 years ago
Attachment #87332 - Attachment is obsolete: true

Comment 37

16 years ago
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.

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

16 years ago
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?
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.
(Assignee)

Comment 41

16 years ago
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. 
(Assignee)

Comment 42

16 years ago
it looks to me like the w3c will drop caption-side:left and right from the css
2.1 spec.
... but only for lack of implementation.  It doesn't make this bug any less valid.
(Assignee)

Comment 44

16 years ago
I have a running prototype, taking the bug.
Assignee: karnaze → bernd.mielke
Status: ASSIGNED → NEW
(Assignee)

Comment 45

16 years ago
Created attachment 98261 [details]
implementation proposal

Comment 46

16 years ago
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.

Updated

16 years ago
Blocks: 166758
(Assignee)

Comment 47

16 years ago
Created attachment 100906 [details]
the css2 testcase
(Assignee)

Comment 48

16 years ago
Created attachment 100908 [details]
rendering proposal
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.
(Assignee)

Comment 50

16 years ago
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.
(Assignee)

Comment 51

16 years ago
Created attachment 101557 [details] [diff] [review]
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
(Assignee)

Updated

16 years ago
Depends on: 172608

Updated

16 years ago
Whiteboard: [awd:tbl][TESTCASE] → [awd:tbl]
(Assignee)

Comment 52

16 years ago
Created attachment 102349 [details] [diff] [review]
patch v 0.2
(Assignee)

Comment 53

16 years ago
Created attachment 102715 [details] [diff] [review]
patch

The patch passed the regression tests. Its open for discussion even r/sr.
Attachment #101557 - Attachment is obsolete: true
Attachment #102349 - Attachment is obsolete: true

Comment 54

16 years ago
Created attachment 103062 [details] [diff] [review]
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.
(Assignee)

Comment 55

16 years ago
Created attachment 103466 [details] [diff] [review]
patch revisited
Attachment #102715 - Attachment is obsolete: true
Attachment #103062 - Attachment is obsolete: true
(Assignee)

Comment 56

16 years ago
Created attachment 103467 [details] [diff] [review]
changes after the previous patch

Updated

16 years ago
Attachment #103467 - Attachment is patch: true
(Assignee)

Comment 57

16 years ago
Comment on attachment 103466 [details] [diff] [review]
patch revisited

there is an error
Attachment #103466 - Attachment is obsolete: true
(Assignee)

Comment 58

16 years ago
Created attachment 104103 [details] [diff] [review]
patch revised

Comment 59

16 years ago
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

16 years ago
Created attachment 104232 [details]
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.
(Assignee)

Comment 61

16 years ago
Created attachment 104270 [details]
testcase proving that % heights are honoured
(Assignee)

Comment 62

16 years ago
Created attachment 104271 [details] [diff] [review]
patch fixing fantsai's testcase
Attachment #104103 - Attachment is obsolete: true
(Assignee)

Comment 63

16 years ago
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

16 years ago
Created attachment 104284 [details]
long centered caption
(Assignee)

Comment 65

16 years ago
Created attachment 104307 [details] [diff] [review]
patch 

the patch fixes attachment 104284 [details]
(Assignee)

Updated

16 years ago
Attachment #104271 - Attachment is obsolete: true

Comment 66

16 years ago
Created attachment 104325 [details]
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

16 years ago
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

16 years ago
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;
+      }
+    }
Attachment #104307 - Flags: review+
(Assignee)

Comment 69

16 years ago
Created attachment 104633 [details] [diff] [review]
patch with reviewers changes
Attachment #104307 - Attachment is obsolete: true
(Assignee)

Comment 70

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

taking over the r=karnaze
Attachment #104633 - Flags: review+

Comment 71

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

sr=kin@netscape.com

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
Attachment #104633 - Flags: superreview+
(Assignee)

Comment 72

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

Please open new bugs if you find bugs within this implementation.
Status: NEW → RESOLVED
Last Resolved: 20 years ago16 years ago
Resolution: --- → FIXED
Created attachment 105557 [details]
new test case showing that layout stays correct of the caption-side is dynamically changed

great work Bernd !
Created attachment 105558 [details]
better mimetype, sorry for the spam
Attachment #105557 - Attachment is obsolete: true

Comment 75

16 years ago
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!
Attachment #105557 - Attachment is obsolete: false
Attachment #105557 - Attachment is patch: false
Attachment #105557 - Attachment mime type: text/plain → text/html

Comment 76

16 years ago
 Verified on trunk Build: 2002112108. All the testcases works fine for " caption
align".
You need to log in before you can comment on or make changes to this bug.