Louis Lazaris Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.71 Safari/537.36

Steps to reproduce:

Use CSS columns property on an element with display: table-caption.


Actual results:

Columns did not take efffect.


Expected results:

The element should have been divided into the columns defined with the columns property. See demo:

http://jsbin.com/tikowiluka/1/edit?html,css,output

	Louis Lazaris Reporter
	Comment 1 • 10 years ago

All the other browsers, latest versions, display the columns correctly. It's possible their implementation is wrong, but I thought I'd submit to the minority browser in this case.

	Stefan Weiss [:sir_none]
	Comment 2 • 10 years ago

Hello,

you mix different CSS and HTML Versions i think.

if you just use:

p {
  border: solid 1px red;
  -moz-columns: 2 200px;
  -webkit-columns: 2 200px;
  columns: 2 200px;
}

it should do wat you want.
can you confirm from your site?

	Louis Lazaris Reporter
	Comment 3 • 10 years ago

(In reply to Stefan Weiss [:sir_none] from comment #2)
> Hello,
> 
> you mix different CSS and HTML Versions i think.
> 
> if you just use:
> 
> p {
>   border: solid 1px red;
>   -moz-columns: 2 200px;
>   -webkit-columns: 2 200px;
>   columns: 2 200px;
> }
> 
> it should do wat you want.
> can you confirm from your site?

Of course that works, but that's beside the point. The bug is happening when you combine table caption with columns. That's not 'mixing' different versions of anything, that's just CSS. Firefox doesn't apply the columns on an element set as a table caption, but all the other browsers do. I'm trying to figure out if the other browsers have the bug, or Firefox does. Someone at Mozilla should be able to establish if this is expected behavior or not, or maybe the spec allows leeway here.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 10 years ago

The problem is that we use a custom layout box for caption, not just a block (see nsTableCaptionFrame).  As a result, it doesn't support columns....

I wonder whether we could switch to just using a block here.  We'd need to:

1)  Replace the tableCaptionFrame frame type checks with display type checks or something.

2)  Get rid of nsTableCaptionFrame::ComputeAutoSize and replace it with ... something.

3)  Make GetParentStyleContext work correctly for the caption block.

4)  Replace nsTableCaptionFrame::AccessibleType with some equivalent.

We could perhaps keep the custom caption frame to handle #3 and #4 while still doing columnsets, but we'd want the styles for the caption to live on the columnset, not the block inside... and we'd still need to solve #1 and #2.

I guess the other option is to subclass columnset with something that looks to the world like a caption but does columny stuff inside.  That sounds pretty painful, though.

	Mats Palmgren (inactive) Assignee
	Comment 6 • 10 years ago

Subclassing columnset seems to work:
https://tbpl.mozilla.org/?tree=Try&rev=6737ba337ec0

	Louis Lazaris Reporter
	Comment 9 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #7)
> Created attachment 8535973 [details]
> Testcase

I don't think that is relevant to this bug. You're correct, it does work, as you can also see in this example:

http://jsbin.com/riyapa/1/edit?html,css,output

But the bug doesn't seem to occur when the table is produced in the HTML. The bug occurs when you take a regular non-table element (div, p, etc.) and convert it using "display: table-caption". That's when the columns do not take effect, as shown in the demo attached by Loic.

	Mats Palmgren (inactive) Assignee
	Comment 10 • 10 years ago

http://jsbin.com/riyapa/1/edit?html,css,output

That example is invalid since it doesn't use column layout at all due to the following error (that is shown in the console):
"Expected end of value but found '40%'.  Error in parsing value for '-moz-columns'.  Declaration dropped."

Percentage is invalid for column-width per the CSS Columns spec:
http://dev.w3.org/csswg/css-multicol/#cw

This bug is about implementing column support for <caption> (or arbitrary elements
styled as "display:table-caption") which currently is non-existent.  The attached
screenshot is the rendering of the attached Testcase using a modified build with
my fixes applied.

	Louis Lazaris Reporter
	Comment 11 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #10)
> http://jsbin.com/riyapa/1/edit?html,css,output
> 
> That example is invalid since it doesn't use column layout at all due to the
> following error (that is shown in the console):
> "Expected end of value but found '40%'.  Error in parsing value for
> '-moz-columns'.  Declaration dropped."
> 
> Percentage is invalid for column-width per the CSS Columns spec:
> http://dev.w3.org/csswg/css-multicol/#cw
> 
> This bug is about implementing column support for <caption> (or arbitrary
> elements
> styled as "display:table-caption") which currently is non-existent.  The
> attached
> screenshot is the rendering of the attached Testcase using a modified build
> with
> my fixes applied.

Sorry that was a mistype. But I'm not sure what you're saying... As a previous commenter pointed out, columns work fine on caption elements. But as the original demo points out, they don't work on elements defined as captions in the CSS. Isn't that what the bug is here?

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

> As a previous commenter pointed out, columns work fine on caption elements.

Not for <caption> elements inside a <table>, they don't.

> Isn't that what the bug is here?

The bug is about columns not working on display:table-caption elements.  That includes <caption> when it's a child of <table>.

	Louis Lazaris Reporter
	Comment 13 • 10 years ago

(In reply to Vacation Dec 15-31 from comment #12)
> > As a previous commenter pointed out, columns work fine on caption elements.
> 
> Not for <caption> elements inside a <table>, they don't.
> 
> > Isn't that what the bug is here?
> 
> The bug is about columns not working on display:table-caption elements. 
> That includes <caption> when it's a child of <table>.

Ah ok... Looks like I was nesting another element inside the caption, on which I applied the columns. So it does work in such a case. Ok, I think I get it now. Thanks.

	Mats Palmgren (inactive) Assignee
	Comment 18 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=7427bacba3fd
https://tbpl.mozilla.org/?tree=Try&rev=9d460cfd1f2f

	Mats Palmgren (inactive) Assignee
	Comment 19 • 10 years ago

Comment on attachment 8539600 [details] [diff] [review]
part 4, Frame construction bits to create the appropriate frame tree for table captions.

Note that this also makes IsPositioned() table captions be abs.pos.
containers.  It appears to have been intentionally disabled before
(FCDATA_SKIP_ABSPOS_PUSH), Boris do you know why?

Other UAs makes table captions abs.pos. containers when styled
as such (IE, Chrome, Safari), so I see no reason why we shouldn't.
I can't find any CSS spec text that disallows it.

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

Why have you taken this approach instead of using our regular frame types as suggested in comment #5?

It seems like this approach is less general. I don't really like the idea of having to have special code just for the (very rare) combination of table-caption+columns. It seems like this wouldn't scale when we mix in other CSS features with their own frame types. Will this work with overflow:auto table-captions with columns, for example?

	Mats Palmgren (inactive) Assignee
	Comment 21 • 10 years ago

Subclassing just seemed simpler, but I'll investigate the other solution a bit more...
It might win on being more generic as you suggest.

	Mats Palmgren (inactive) Assignee
	Comment 26 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e172b61aa222

	Mats Palmgren (inactive) Assignee
	Comment 27 • 10 years ago

The new patches also fixes bug 553977 (scrolling captions).

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

Comment on attachment 8540861 [details] [diff] [review]
part 4, Frame construction bits to create the appropriate frame tree for table captions.

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

Excellent! Looks like a net code reduction :-)

	Mats Palmgren (inactive) Assignee
	Comment 29 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2959e44dd6f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee05a5f6196
https://hg.mozilla.org/integration/mozilla-inbound/rev/b539dc005423
https://hg.mozilla.org/integration/mozilla-inbound/rev/79eb639db611
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67489a65e49

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/2959e44dd6f5
https://hg.mozilla.org/mozilla-central/rev/bee05a5f6196
https://hg.mozilla.org/mozilla-central/rev/b539dc005423
https://hg.mozilla.org/mozilla-central/rev/79eb639db611
https://hg.mozilla.org/mozilla-central/rev/b67489a65e49

	Jean-Yves Perrier [:teoli]
	Comment 31 • 10 years ago

Updated
https://developer.mozilla.org/en-US/docs/Web/CSS/columns
https://developer.mozilla.org/en-US/docs/Web/CSS/column-width
https://developer.mozilla.org/en-US/docs/Web/CSS/column-count
and
https://developer.mozilla.org/en-US/Firefox/Releases/37#CSS

	neil@parkwaycc.co.uk
	Comment 32 • 10 years ago

Comment on attachment 8540861 [details] [diff] [review]
part 4, Frame construction bits to create the appropriate frame tree for table captions.

>+    static const FrameConstructionData sNonScrollableBlockData[2][2] {
(Please would someone enlighten me as to this construct? I'm expecting an = before the {.)

	Mats Palmgren (inactive) Assignee
	Comment 33 • 9 years ago

It's a typo.  It seems the construct is accepted when compiling with -std=gnu++0x.
I've landed a fix in bug 1116236.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 9 years ago

> It appears to have been intentionally disabled before
> (FCDATA_SKIP_ABSPOS_PUSH), Boris do you know why?

Probably because I kept whatever the old (Karnaze?) behavior was when I switched things over to frame construction items.  I see no good reason to SKIP_ABSPOS_PUSH there, though we should add a testcase if there isn't one already.  I'm surprised we didn't have a bug about this already filed!

	Mats Palmgren (inactive) Assignee
	Comment 35 • 9 years ago

OK, good.  The tests that landed includes a few abs.pos. tests already.