695222 - Implement column-fill property of CSS3 spec

Status: RESOLVED DUPLICATE of bug 764567

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

Currently, the column-fill property, as specified in http://www.w3.org/TR/css3-multicol/#cf is not implemented by Gecko. In order to become fully compliant with this spec, we should implement the flow behavior as specified.

A reftest for the expected behavior has already been written as part of a patch to bug 684062.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b695222

Hopefully I'm on the right track here.

Comment 2

[Scott Johnson (:jwir3)]

Also, this is the prefixed form of this, but it'll be unprefixed when bug 698783 is finished.

Comment 3

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

Comment on attachment 577455 [details] [diff] [review]
b695222

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

Also, get dbaron's review on the style system changes.

::: dom/interfaces/css/nsIDOMCSS2Properties.idl
@@ +534,5 @@
>  
>             attribute DOMString        MozColumnGap;
>                                          // raises(DOMException) on setting
>  
> +           attribute DOMString        MozColumnFill;

Rev UUID on interface

::: layout/generic/nsColumnSetFrame.cpp
@@ +422,4 @@
>    // NOTE that the non-balancing behavior for non-auto computed height
>    // is not in the CSS3 columns draft as of 18 January 2001
> +  if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
> +      || aReflowState.ComputedHeight() == NS_INTRINSICSIZE) {

I think we should check computed max-height as well. If height is not auto OR max-height is not 'none' then we should consider the column-length to be constrained. The spec is unclear and I've asked Hakon to clarify.

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b695222 (v2)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Also, get dbaron's review on the style system changes.

Added dbaron as a reviewer.

> Rev UUID on interface

Done.

> I think we should check computed max-height as well. If height is not auto
> OR max-height is not 'none' then we should consider the column-length to be
> constrained. The spec is unclear and I've asked Hakon to clarify.

Ok, I added logic for this. I believe this is what you were asking for, but please let me know if I did this incorrectly.

Comment 5

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

Comment on attachment 578114 [details] [diff] [review]
b695222 (v2)

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

::: layout/generic/nsColumnSetFrame.cpp
@@ +422,5 @@
>    // NOTE that the non-balancing behavior for non-auto computed height
>    // is not in the CSS3 columns draft as of 18 January 2001
> +  if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
> +      || aReflowState.ComputedHeight() == NS_INTRINSICSIZE
> +      || aReflowState.mComputedMaxHeight != NS_UNCONSTRAINEDSIZE) {

I don't think this is right. If the max-height != NS_UNCONSTRAINEDSIZE, then the column length is constrained so we should respect COLUMN_FILL_AUTO.

Comment 6

[Scott Johnson (:jwir3)]

Attachment: TEST CASE

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)

> I don't think this is right. If the max-height != NS_UNCONSTRAINEDSIZE, then
> the column length is constrained so we should respect COLUMN_FILL_AUTO.

So the test case I've posted works with the conditional as I had originally added it. However, I do agree that the logic seems incorrect. If I make the conditional:

  if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
      || aReflowState.ComputedHeight() == NS_INTRINSICSIZE
      || aReflowState.mComputedMaxHeight == NS_UNCONSTRAINEDSIZE) {

Then, this results in 'with column-fill: auto and constrained height' being rendered incorrectly. It appears as the 'with column-fill: balance and constrained height' appears in the expected results of the test case.

I also have a question about whether the behavior is correct for 'with column-fill: balance and constrained height'. It seems as though the div containing the column box is large (obviously because it has a specified height), but the text is balanced correctly. I'm not sure how to resolve this discrepancy - the height of the column frame is specified explicitly, so it doesn't seem like it should be overridden. Thoughts on this?

Comment 7

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #6)
> Created attachment 578376 [details]
> TEST CASE
> 
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> 
> > I don't think this is right. If the max-height != NS_UNCONSTRAINEDSIZE, then
> > the column length is constrained so we should respect COLUMN_FILL_AUTO.
> 
> So the test case I've posted works with the conditional as I had originally
> added it. However, I do agree that the logic seems incorrect. If I make the
> conditional:
> 
>   if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
>       || aReflowState.ComputedHeight() == NS_INTRINSICSIZE
>       || aReflowState.mComputedMaxHeight == NS_UNCONSTRAINEDSIZE) {
> 
> Then, this results in 'with column-fill: auto and constrained height' being
> rendered incorrectly. It appears as the 'with column-fill: balance and
> constrained height' appears in the expected results of the test case.

I think I have the logic incorrect. I think it should be something like:
  if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
      || (aReflowState.ComputedHeight() == NS_INTRINSICSIZE
          && aReflowState.mComputedMaxHeight != NS_UNCONSTRAINEDSIZE))

Comment 8

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #7)

> I think I have the logic incorrect. I think it should be something like:
>   if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
>       || (aReflowState.ComputedHeight() == NS_INTRINSICSIZE
>           && aReflowState.mComputedMaxHeight != NS_UNCONSTRAINEDSIZE))

Oops, I meant:
  if (colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE
      || (aReflowState.ComputedHeight() == NS_INTRINSICSIZE
          && aReflowState.mComputedMaxHeight == NS_UNCONSTRAINEDSIZE))

Comment 9

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

Comment on attachment 578114 [details] [diff] [review]
b695222 (v2)

As far as the style system changes go:

You should make appropriate additions to layout/style/test/property_database.js and then get the mochitests in layout/style/ to pass.  I'd note that you're at the very least missing a piece of the code you need to add in nsRuleNode to handle Inherit/Initial, which you should probably just use SetDiscrete for.  You might also be missing other pieces, but the tests should help with that.

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b695222 (v3)

(In reply to David Baron [:dbaron] from comment #9)
> You should make appropriate additions to
> layout/style/test/property_database.js and then get the mochitests in
> layout/style/ to pass.  

Ok, I did this. They all appear to be passing for me now.

> I'd note that you're at the very least missing a
> piece of the code you need to add in nsRuleNode to handle Inherit/Initial,
> which you should probably just use SetDiscrete for.  You might also be
> missing other pieces, but the tests should help with that.

I replaced the chunk I had in nsRuleNode for this with SetDiscrete. as you advised, however, I'm unsure about the last 4 parameters that I'm passing in. If you could verify these are correct, then I think this should work as expected now.

Comment 11

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

Comment on attachment 579877 [details] [diff] [review]
b695222 (v3)

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

>+  // column-fill: enum
>+  SetDiscrete(*aRuleData->ValueForColumnFill(),
>+                column->mColumnFill, canStoreInRuleTree,
>+                SETDSC_ENUMERATED, parent->mColumnFill,
>+                NS_STYLE_COLUMN_FILL_BALANCE, NS_STYLE_COLUMN_FILL_AUTO,
>+                0, 0, 0);

The last four values only do anything if you use flags other than SETDSC_ENUMERATED, so you should just make the last 4 all 0.

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h

>   PRUint32     mColumnCount; // [reset] see nsStyleConsts.h
>   nsStyleCoord mColumnWidth; // [reset] coord, auto
>   nsStyleCoord mColumnGap;   // [reset] coord, normal
>+  PRUint8      mColumnFill;  // [reset] see nsStyleConsts.h
> 
>   nscolor      mColumnRuleColor;  // [reset]
>   PRUint8      mColumnRuleStyle;  // [reset]
>+

Probably best (For struct packing) to put mColumnFill after mColumnRuleStyle, so you have two PRUint8 values together.

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js

This file uses tabs.  Sorry.  You should match the rest.


r=dbaron with that

Comment 12

[Scott Johnson (:jwir3)]

Attachment: b695222 (v4) [r=dbaron]

Fixed the issues raised by dbaron in the last comment. Requesting re-review of the layout stuff.

Comment 13

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

Comment on attachment 579953 [details] [diff] [review]
b695222 (v4) [r=dbaron]

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

You need to add something to nsStyleColumn::CalcDifference to reflow when column-fill changes.

We'll need reftests covering that, and the other basic cases for column-fill (e.g. making sure it behaves correctly when height is constrained).

Comment 14

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

Sounds like Hakon is leaning towards removing the language about column-fill only applying in certain cases. So you can remove that from your patch.

Comment 15

[Scott Johnson (:jwir3)]

Attachment: b695222 (v5) [r=dbaron]

Made suggested changes to CalculateDifference and added unit tests.

Comment 16

[Scott Johnson (:jwir3)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b804e5f3f5

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/93b804e5f3f5

Comment 18

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en/CSS/column-fill
https://developer.mozilla.org/en/Firefox_12_for_developers

Comment 19

[Daniel Holbert [:dholbert]]

My flexbox code touches a chunk of code that this bug added (since column-* properties compute to their initial values on a flexbox), and I noticed that there was some extra indentation.  I figured I'd fix the indentation as an atomic followup-cset on this bug rather than as part of flexbox.  Pushed indentation-fix (as whitespace-only/DONTBUILD):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/5343959472c8

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5343959472c8

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Looks like this was backed out with https://hg.mozilla.org/releases/mozilla-beta/rev/fe494e9c9714, so the docs will need to be updated.

Comment 22

[Jean-Yves Perrier [:teoli]]

khuey: thx, I missed that point. Do you know if it may be backed out from Beta13 too?

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's possible, if the regressions are not fixed.  Scott could give a better assessment than me of how likely that is.

Comment 24

[Scott Johnson (:jwir3)]

I'm trying to get this regression fixed in 13, but I'm not sure whether it will get completed soon enough. it's probably a 50-50 chance it will be in 13.

Comment 25

[Jean-Yves Perrier [:teoli]]

The doc was up-to-date (fx 13), so I probably noticed the backout but didn't changed the target version here.

Scott: thx, could you change the keyword here to dev-doc-needed if it happens (or add dev-doc-needed on the backout bug) so that we track it in the doc.

(I'm a big fan of CSS3 Columns, I would like to have column-span and the breaking values for the MDN :-) — we use it for the index and may use it for some 2-column landing page but need to control the breaking between sections )

Comment 26

[Scott Johnson (:jwir3)]

(In reply to Jean-Yves Perrier [:teoli] from comment #25)
> The doc was up-to-date (fx 13), so I probably noticed the backout but didn't
> changed the target version here.
> 
> Scott: thx, could you change the keyword here to dev-doc-needed if it
> happens (or add dev-doc-needed on the backout bug) so that we track it in
> the doc.
> 

Sure thing.

> (I'm a big fan of CSS3 Columns, I would like to have column-span and the
> breaking values for the MDN :-) — we use it for the index and may use it for
> some 2-column landing page but need to control the breaking between sections
> )

Cool. Yeah, I'm working on it. :) Gotta get column-fill right, first, though. ;)

Comment 27

[Scott Johnson (:jwir3)]

This was backed out of Fx13.

Comment 28

[Eric Shepherd [:sheppy]]

Removing doc-needed; please place it on whatever bug covers re-landing this.

Comment 29

[j.j.]

I'm confused.
If this was backed out of Fx13, at least 
https://developer.mozilla.org/en/CSS/column-fill and
https://developer.mozilla.org/en/Firefox_13_for_developers
need to change, therefore the dev-doc-needed keyword, no?

And the status flags are confusing. Why is this fixed in Fx12 and 13 when it was backed out?

Comment 30

[Jean-Yves Perrier [:teoli]]

Sorry, my bad I should have reacted quickly.

I fixed the two pages and Fx 14 for developers.

Currently both our team and the PMM team have problems to track what is being backed out in the Aurora and Beta phase (as often the back-out happens in a separate bug that is not followed by any of the two teams, and has a cryptic title).

So when a back-out occurs on something that has or need to be documented, the dev-doc-needed must be added on the back-out bug (which must have a link to the original bug so that we can easily find it!), or at least on the original bug (like here), though the second case may lead to some confusion. 

That way we can update Firefox XY for developers that are now often done before the documentation per se.

Sorry for the confusion. (And as the back-out of Fx 13 has been handled in regard to the documentation I switch again the flag :-) ). 

:-)

Comment 31

[Daniel Veditz [:dveditz]]

The date of the beta backout, March 30, means Firefox 12 should still have the bug
http://hg.mozilla.org/releases/mozilla-beta/rev/fe494e9c9714

According to comment 27 it was also backed out of Firefox 13. Unless there have been additional backouts this should still be fixed on 14 and later. Adjusting the status flags to match that understanding, the opposite of how they were set. Maybe Scott was thinking of the regression bug 733614 when he was setting them.

Comment 32

[Scott Johnson (:jwir3)]

Backed this out on mozilla central (currently firefox 16) due to bug 733614:
https://hg.mozilla.org/mozilla-central/rev/aeaa00b5cab5
https://hg.mozilla.org/mozilla-central/rev/00244ceddd42

This will not be fixed until at least Firefox 16.

Comment 33

[Scott Johnson (:jwir3)]

I opened another bug to track remaining issues with column fill.