1109571 - Multiple Columns not working on element with display: table-caption

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Louis Lazaris]

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

Comment 1

[Louis Lazaris]

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.

Comment 2

[Stefan Weiss [:sir_none]]

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?

Comment 3

[Louis Lazaris]

(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.

Comment 4

[Loic]

Attachment: jsbin.tikowiluka.1.html (reporter's testcase)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 8

[Mats Palmgren (inactive)]

Attachment: Screenshot

Comment 9

[Louis Lazaris]

(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.

Comment 10

[Mats Palmgren (inactive)]

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.

Comment 11

[Louis Lazaris]

(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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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>.

Comment 13

[Louis Lazaris]

(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.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 1, Move the nsTableCaptionFrame code into separate files

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 2, Implement nsTableCaptionColumnSetFrame, a subclass of nsColumnSetFrame, to support table captions with CSS Columns layout.

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 3, Replace nsGkAtoms::tableCaptionFrame with an IsFrameOfType bit.

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 4, Frame construction bits to create the appropriate frame tree for table captions.

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 5 - tests

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

Comment 19

[Mats Palmgren (inactive)]

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.

Comment 20

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

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?

Comment 21

[Mats Palmgren (inactive)]

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

Comment 22

[Mats Palmgren (inactive)]

Attachment: part 1, Remove nsTableCaptionFrame.

Comment 23

[Mats Palmgren (inactive)]

Attachment: part 2, Remove nsGkAtoms::tableCaptionFrame. Implement nsIFrame::IsTableCaption() and use that instead.

Comment 24

[Mats Palmgren (inactive)]

Attachment: part 3, Implement table caption specific code for ComputeAutoSize(), GetCorrectedParent() (for getting the style parent frame) and AccessibleType().

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 4, Frame construction bits to create the appropriate frame tree for table captions.

Comment 26

[Mats Palmgren (inactive)]

Attachment: part 5, tests

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

Comment 27

[Mats Palmgren (inactive)]

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

Comment 28

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

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 :-)

Comment 29

[Mats Palmgren (inactive)]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Jean-Yves Perrier [:teoli]]

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

Comment 32

[[e-mail]]

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 {.)

Comment 33

[Mats Palmgren (inactive)]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

> 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!

Comment 35

[Mats Palmgren (inactive)]

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