The default bug view has changed. See this FAQ.

Implement column-fill property of CSS3 spec

RESOLVED DUPLICATE of bug 764567

Status

()

Core
Layout
RESOLVED DUPLICATE of bug 764567
6 years ago
5 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 wontfix, firefox13 wontfix, firefox14 wontfix, firefox15 wontfix, firefox16 affected)

Details

(Whiteboard: [qa+], URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Blocks: 698783
(Assignee)

Comment 1

5 years ago
Created attachment 577455 [details] [diff] [review]
b695222

Hopefully I'm on the right track here.
Attachment #577455 - Flags: review?(roc)
(Assignee)

Comment 2

5 years ago
Also, this is the prefixed form of this, but it'll be unprefixed when bug 698783 is finished.
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.
(Assignee)

Comment 4

5 years ago
Created attachment 578114 [details] [diff] [review]
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.
Attachment #577455 - Attachment is obsolete: true
Attachment #577455 - Flags: review?(roc)
Attachment #578114 - Flags: review?(roc)
Attachment #578114 - Flags: review?(dbaron)
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.
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

5 years ago
(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 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.
Attachment #578114 - Flags: review?(dbaron) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 579877 [details] [diff] [review]
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.
Attachment #578114 - Attachment is obsolete: true
Attachment #578114 - Flags: review?(roc)
Attachment #579877 - Flags: review?(dbaron)
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
Attachment #579877 - Flags: review?(dbaron) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 579953 [details] [diff] [review]
b695222 (v4) [r=dbaron]

Fixed the issues raised by dbaron in the last comment. Requesting re-review of the layout stuff.
Attachment #579877 - Attachment is obsolete: true
Attachment #579953 - Flags: review?(roc)
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).
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.
(Assignee)

Comment 15

5 years ago
Created attachment 582956 [details] [diff] [review]
b695222 (v5) [r=dbaron]

Made suggested changes to CalculateDifference and added unit tests.
Attachment #579953 - Attachment is obsolete: true
Attachment #579953 - Flags: review?(roc)
Attachment #582956 - Flags: review?(roc)
Attachment #582956 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Blocks: 569193
(Assignee)

Updated

5 years ago
Blocks: 459597
(Assignee)

Comment 16

5 years ago
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b804e5f3f5
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/93b804e5f3f5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: dev-doc-needed
https://developer.mozilla.org/en/CSS/column-fill
https://developer.mozilla.org/en/Firefox_12_for_developers
Keywords: dev-doc-needed → dev-doc-complete
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
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/5343959472c8
Depends on: 722888

Updated

5 years ago
Depends on: 731516

Updated

5 years ago
Depends on: 733614

Updated

5 years ago
Depends on: 733808
Depends on: 747688
Looks like this was backed out with https://hg.mozilla.org/releases/mozilla-beta/rev/fe494e9c9714, so the docs will need to be updated.
Keywords: dev-doc-complete → dev-doc-needed
Target Milestone: mozilla12 → mozilla13
khuey: thx, I missed that point. Do you know if it may be backed out from Beta13 too?
That's possible, if the regressions are not fixed.  Scott could give a better assessment than me of how likely that is.
(Assignee)

Comment 24

5 years ago
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.
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 )
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 26

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

Comment 27

5 years ago
This was backed out of Fx13.
status-firefox12: --- → fixed
status-firefox13: --- → fixed
status-firefox14: --- → affected
status-firefox15: --- → affected
Keywords: dev-doc-complete → dev-doc-needed
Whiteboard: [qa+]
Removing doc-needed; please place it on whatever bug covers re-landing this.
Keywords: dev-doc-needed

Comment 29

5 years ago
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?
Keywords: dev-doc-needed
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 :-) ). 

:-)
Keywords: dev-doc-needed → dev-doc-complete
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.
status-firefox12: fixed → affected
status-firefox13: fixed → affected
status-firefox14: affected → wontfix
status-firefox15: affected → fixed
Target Milestone: mozilla13 → mozilla14
(Assignee)

Comment 32

5 years ago
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.
Status: RESOLVED → REOPENED
status-firefox12: affected → wontfix
status-firefox13: affected → wontfix
status-firefox15: fixed → wontfix
status-firefox16: --- → affected
Resolution: FIXED → ---
(Assignee)

Comment 33

5 years ago
I opened another bug to track remaining issues with column fill.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 764567
You need to log in before you can comment on or make changes to this bug.