Closed Bug 733614 Opened 12 years ago Closed 12 years ago

CSS 'height' property on a multi-column block is not respected

Categories

(Core :: Layout, defect, P1)

12 Branch
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox12 + fixed
firefox13 + verified
firefox14 + verified
firefox15 + verified
firefox16 + verified

People

(Reporter: epinal99-bugzilla2, Assigned: jwir3)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, regression, Whiteboard: [qa+])

Attachments

(11 files, 3 obsolete files)

10.75 KB, text/html
Details
1.92 KB, text/html
Details
183.99 KB, image/png
Details
4.67 KB, text/html
Details
199.17 KB, image/png
Details
16.12 KB, text/html
Details
22.94 KB, patch
Details | Diff | Splinter Review
40.70 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
43.77 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
42.89 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
43.79 KB, patch
jwir3
: review+
dbaron
: review+
Details | Diff | Splinter Review
Attached file testcase.html
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120306 Firefox/13.0a1
Build ID: 20120306031101

Steps to reproduce:

Open the testcase (joined) in FF13 and in FF10. In FF13, the property 'height' on a CSS multi-column block is not respected and FF13 is not able to build 250px-width & 600px-height columns from the left to the right.

Ref.: https://developer.mozilla.org/en/CSS3_Columns
Component: Untriaged → Layout
Product: Firefox → Core
Hardware: x86_64 → All
Attachment #603521 - Attachment mime type: text/plain → text/html
Regression window,
Works:
http://hg.mozilla.org/mozilla-central/rev/9f29daaecbcc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20111226 Firefox/12.0a1 ID:20111226031002
Fails:
http://hg.mozilla.org/mozilla-central/rev/6f4f2e53694b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20111227 Firefox/12.0a1 ID:20111227031015
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9f29daaecbcc&tochange=6f4f2e53694b

Suspected: Bug 695222
Blocks: 695222
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
Version: 13 Branch → 12 Branch
After reading the specs, I'm not sure if it's a real regression:
http://www.w3.org/TR/css3-multicol/#pseudo-algorithm

Pseudo-algo steps:
(15)  if (column-width != auto) and (column-count = auto) then
(16)    N := max(1, floor((available-width + column-gap) / (column-width + column-gap)));
(17)    W := ((available-width + column-gap) / N) - column-gap;
(18)  exit;

In my testcase:
N = max(1, floor(900/250)) = 3
W = 900/3 = 300

And that's this rendering we see in FF13: 3 columns with 300px as width, 600px as height between top and the horizontal scrollbar.
http://www.w3.org/TR/css3-multicol/#pagination-and-overflow-outside-multicol clearly specified as follows.

"a declaration that constrains the column height (e.g., using ‘height’ or ‘max-height’). In this case, additional column boxes are created in the inline direction"
 
So I think this is a regression.
Yes, I think it is too.
Assignee: nobody → sjohnson
Priority: -- → P3
QA Contact: untriaged → layout
This is now tracking for FF12 since it may cause web regressions. I'm concerned with the P3 priority attached to this bug. Please make the case for untracking for release, or prioritize/fix this bug.
Priority: P3 → P1
Modified priority to reflect that it's tracking for FF12.
I think our best bet is probably to back bug 695222 out of Ff12 until I can prepare a fix for this. How should I go about requesting approval for this?
roc, do you agree that bug 695222 should be backed out of FF12 (beta)? Otherwise, I think we'll have a strange state where FF12 supports column-fill in a way that breaks overflow for columns, but then it would be fixed again in FF13. This inconsistency should be corrected, and I'm not confident that trying to fix this, get review, get approval, etc... all within the next 2 weeks is a great idea.

[Approval Request Comment]
Regression caused by (bug #): 695222
User impact if declined: regression will cause a strange state where overflow is semi-supported in a strange way in FF12 only
Testing completed (on m-c, etc.): n/a - this is a backout patch
Risk to taking this patch (and alternatives if risky): very low, because bug 695222 was a feature originally added starting in FF12
String changes made by this patch: none
Attachment #610589 - Flags: approval-mozilla-beta?
(In reply to Scott Johnson (:jwir3) from comment #11)
> roc, do you agree that bug 695222 should be backed out of FF12 (beta)?

Yes.
Comment on attachment 610589 [details] [diff] [review]
b695222-backout: Backout bug 695222 on mozilla-beta

[Triage Comment]
Low risk backout for a regression that may cause web incompatibilities. Approved for Beta 12.
Attachment #610589 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm adding dev-doc-needed as we already documented this and have to correct it now column-fill has been (temporary) backed out.
Keywords: dev-doc-needed
Oh yes, if column-fill is backed out of Fx 13 later, could you flip (or set if another bug) the dev-doc-complete but to dev-doc-needed so we don't miss it.
In a situation where max-width and max-height are specified, and there is overflow from columns, should this overflow ALWAYS result in additional columns? Attached is a test case with these specified, where the columns overflow, but it seems to be improperly rendered in webkit as well.
WRT comment 18, obviously our rendering is incorrect currently, but I'm curious as to what the behavior SHOULD be.
I've made a chart of what we're missing compared to what Opera and Chrome (webkit) are missing. I'm working on a patch right now to fix this. I think I can proceed by forcing it to balance in certain special cases (where the height is restricted), as it did prior to bug 695222 landing.
Attached patch b733614Splinter Review
This seems to fix the overflow problems with bug 695222, but I'm not sure I took it in the right direction. Basically, I added a loop in Reflow() that will run a maximum of 2 times. It runs reflow as requested (with column-fill: auto or balance), but then if there is overflow (determined after FinishAndStoreOverflow() is called, and balance was requested), it loops back to the beginning, so that it can re-reflow with column-fill: auto.

The 'auto' column fill for overflow columns seems to be what webkit and opera do for overflow columns, although I can't seem to see that it's specifically noted in the spec. It doesn't seem correct to balance a bunch of overflow columns, to me, though.

At any rate, I'd appreciate an initial review to determine if this is the right approach, and if not, perhaps some guidance as to what direction to proceed in. 

Thanks, Roc!
Attachment #617650 - Flags: review?(roc)
Comment on attachment 617650 [details] [diff] [review]
b733614

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

::: layout/generic/nsColumnSetFrame.cpp
@@ +359,5 @@
>    nscoord colHeight = GetAvailableContentHeight(aReflowState);
>    if (aReflowState.ComputedHeight() != NS_INTRINSICSIZE) {
>      colHeight = aReflowState.ComputedHeight();
> +  } else if (aReflowState.mComputedMaxHeight != NS_INTRINSICSIZE) {
> +    colHeight = aReflowState.mComputedMaxHeight;

This change looks good.

@@ +444,5 @@
>        numColumns = 1;
>      }
>  
>      colHeight = NS_MIN(mLastBalanceHeight,
> +                       colHeight);

This change looks good.

@@ +966,5 @@
> +  // overflows, then we want to revert to column-fill: auto. Unfortunately,
> +  // we can't really tell whether overflow will happen until after
> +  // FinishAndStoreOverflow() is called, so we need to try to reflow with
> +  // balancing first, and then if we have overflow, reflow again with balancing
> +  // disabled.

This isn't quite the right approach. The overflow we care about here is not the overflow of FinishAndStoreOverflow. For example, the very first column might contain a relatively positioned element that extends outside the column set: in that case, there will be scrollable or visual overflow, but that shouldn't affect our column filling behavior.

All that matters is whether all the content can be placed in the allocated number of columns. I.e., do we hit the case
      if (columnCount >= aConfig.mBalanceColCount) {
        // No more columns allowed here. Stop.
If we do, then we might need to use the auto-fill fallback that your patch introduces. Otherwise we don't.

Note that if columnCount >= aConfig.mBalanceColCount and it's because we hit the 'height' or 'max-height' limit we should add overflow columns, but if it's because we hit the available-height limit we should NOT add overflow columns but keep taking the "Move any of our leftover columns to our overflow list" path, so that the extra columns will appear on the next page.
Are you aware that the issue is still not fixed in Firefox 14 (beta channel)? I have a web site which relies on the now broken behavior, eg.: http://www.metareads.com/articles/read/dan-maharry-ddd-southwest-session-notes-3-defensive-programming-101/ef5efdf4-eb31-48d4-a88d-605d81edd651 (on Chrome it works fine).
(In reply to Alex Keybl [:akeybl] from comment #23)
> Backed out of FF13:
> https://hg.mozilla.org/releases/mozilla-beta/rev/e60ca2e387a8

(In reply to Lukas Blakk [:lsblakk] from comment #25)
> Backed out of FF14:
> http://hg.mozilla.org/releases/mozilla-beta/rev/564eb0ab7580

These alleged backouts were both indentation changes, and look nothing like the backout that happened on Firefox 12:

https://hg.mozilla.org/releases/mozilla-beta/rev/fe494e9c9714


So it looks like we shipped this in Firefox 13.  Could somebody confirm?
(In reply to David Baron [:dbaron] from comment #27)
> So it looks like we shipped this in Firefox 13.  Could somebody confirm?

From the testcase and checkin, this would appear to be the case.

Scott - can you prepare backout patches for all branches, including mozilla-release and mozilla-central?
(In reply to Alex Keybl [:akeybl] from comment #28)
> (In reply to David Baron [:dbaron] from comment #27)
> > So it looks like we shipped this in Firefox 13.  Could somebody confirm?
> 
> From the testcase and checkin, this would appear to be the case.

Unfortunately, yes, this is the case.

> Scott - can you prepare backout patches for all branches, including
> mozilla-release and mozilla-central?

I'm doing so right now.
Backout patch for mozilla-central
Attachment #610589 - Attachment is obsolete: true
Attachment #632424 - Flags: review?(dbaron)
The others are still compiling so I can verify them. As soon as they are done, I'll post them here.
Whiteboard: [qa+]
Comment on attachment 632424 [details] [diff] [review]
b695222-backout-fx16: Backout patch for central

>diff --git a/dom/interfaces/css/nsIDOMCSS2Properties.idl b/dom/interfaces/css/nsIDOMCSS2Properties.idl
>--- a/dom/interfaces/css/nsIDOMCSS2Properties.idl
>+++ b/dom/interfaces/css/nsIDOMCSS2Properties.idl

This is going to need a new UUID.

>diff --git a/layout/generic/nsColumnSetFrame.cpp b/layout/generic/nsColumnSetFrame.cpp
>--- a/layout/generic/nsColumnSetFrame.cpp
>+++ b/layout/generic/nsColumnSetFrame.cpp
>@@ -326,32 +326,16 @@ nsColumnSetFrame::ChooseColumnStrategy(c
>   nscoord colHeight = GetAvailableContentHeight(aReflowState);
>   if (aReflowState.ComputedHeight() != NS_INTRINSICSIZE) {
>     colHeight = aReflowState.ComputedHeight();
>   }
> 
>   nscoord colGap = GetColumnGap(this, colStyle);
>   PRInt32 numColumns = colStyle->mColumnCount;
> 
>-  bool isBalancing = colStyle->mColumnFill == NS_STYLE_COLUMN_FILL_BALANCE;
>-  if (isBalancing) {
>-    const PRUint32 MAX_NESTED_COLUMN_BALANCING = 2;
>-    PRUint32 cnt = 1;
>-    for (const nsHTMLReflowState* rs = aReflowState.parentReflowState;
>-         rs && cnt < MAX_NESTED_COLUMN_BALANCING;
>-         rs = rs->parentReflowState) {
>-      if (rs->mFlags.mIsColumnBalancing) {
>-        ++cnt;
>-      }
>-    }
>-    if (cnt == MAX_NESTED_COLUMN_BALANCING) {
>-      numColumns = 1;
>-    }
>-  }
>-

You should leave this, but remove the isBalancing variable (and make it always act like isBalancing is true).  Otherwise you'll be reverting bug 725376.

>@@ -635,17 +615,16 @@ nsColumnSetFrame::ReflowChildren(nsHTMLR
>       if (reflowNext)
>         child->AddStateBits(NS_FRAME_IS_DIRTY);
> 
>       nsHTMLReflowState kidReflowState(PresContext(), aReflowState, child,
>                                        availSize, availSize.width,
>                                        aReflowState.ComputedHeight());
>       kidReflowState.mFlags.mIsTopOfPage = true;
>       kidReflowState.mFlags.mTableIsSplittable = false;
>-      kidReflowState.mFlags.mIsColumnBalancing = aConfig.mBalanceColCount < PR_INT32_MAX;
>           
> #ifdef DEBUG_roc
>       printf("*** Reflowing child #%d %p: availHeight=%d\n",
>              columnCount, (void*)child,availSize.height);
> #endif

Likewise, leave this piece.  (Part of the same patch.)

>-== columnfill-overflow-style.html columnfill-overflow-style-ref.html

You should also remove these test files.

And you should also remove the line added to ua.css for bug 715203.

r=dbaron with that
Attachment #632424 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #32)
> Comment on attachment 632424 [details] [diff] [review]
> b695222-backout-fx16: Backout patch for central
> 
> >diff --git a/dom/interfaces/css/nsIDOMCSS2Properties.idl b/dom/interfaces/css/nsIDOMCSS2Properties.idl
> >--- a/dom/interfaces/css/nsIDOMCSS2Properties.idl
> >+++ b/dom/interfaces/css/nsIDOMCSS2Properties.idl
> 
> This is going to need a new UUID.

We're pursuing a backout for mozilla-release this morning - this UUID change sounds like it could break add-on/web compatibility. Is that true?
(In reply to Alex Keybl [:akeybl] from comment #33)
> (In reply to David Baron [:dbaron] from comment #32)
> > Comment on attachment 632424 [details] [diff] [review]
> > b695222-backout-fx16: Backout patch for central
> > 
> > >diff --git a/dom/interfaces/css/nsIDOMCSS2Properties.idl b/dom/interfaces/css/nsIDOMCSS2Properties.idl
> > >--- a/dom/interfaces/css/nsIDOMCSS2Properties.idl
> > >+++ b/dom/interfaces/css/nsIDOMCSS2Properties.idl
> > 
> > This is going to need a new UUID.
> 
> We're pursuing a backout for mozilla-release this morning - this UUID change
> sounds like it could break add-on/web compatibility. Is that true?

Nothing to do with Web compatibility, and I think breaking addons is unlikely since this interface is very unlikely to be used by C++ code (although it's possible).  Its UUID changes pretty much every release, and there's an alternative way to do what it does that doesn't have that problem.
Same patch, but tailored for aurora branch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 695222
User impact if declined: Functionality of column-fill will regress balancing of columns in other cases.
Testing completed (on m-c, etc.): m-c, earlier releases (fx 12)
Risk to taking this patch (and alternatives if risky): Very low risk - just backing out a patch that was previously applied.
String or UUID changes made by this patch: UUID change made to nsIDOMCSS2Properties
Attachment #632732 - Flags: approval-mozilla-aurora?
Also added the following cset which reverts a whitespace change I made accidentally in the last patch:
https://hg.mozilla.org/mozilla-central/rev/00244ceddd42
Backout patch for release branch.

[Approval Request Comment]
(See above)
Attachment #632768 - Flags: approval-mozilla-release?
You need to be more careful with the UUIDs; if the interface is different, you want different UUIDs, so while you're correct to use the same UUID on mozilla-central and mozilla-aurora (since the interface is currently the same on both), you need a different UUID for mozilla-release and mozilla-beta (which are the same as each other, but different from mozilla-aurora and mozilla-central).
Comment on attachment 632732 [details] [diff] [review]
b695222-backout-fx15: Backout patch for aurora

>@@ -636,18 +628,18 @@ nsColumnSetFrame::ReflowChildren(nsHTMLR
>         child->AddStateBits(NS_FRAME_IS_DIRTY);
> 
>       nsHTMLReflowState kidReflowState(PresContext(), aReflowState, child,
>                                        availSize, availSize.width,
>                                        aReflowState.ComputedHeight());
>       kidReflowState.mFlags.mIsTopOfPage = true;
>       kidReflowState.mFlags.mTableIsSplittable = false;
>       kidReflowState.mFlags.mIsColumnBalancing = aConfig.mBalanceColCount < PR_INT32_MAX;
>-          
>-#ifdef DEBUG_roc
>+
>+      #ifdef DEBUG_roc
>       printf("*** Reflowing child #%d %p: availHeight=%d\n",
>              columnCount, (void*)child,availSize.height);
> #endif
> 
>       // Note if the column's next in flow is not being changed by this incremental reflow.
>       // This may allow the current column to avoid trying to pull lines from the next column.
>       if (child->GetNextSibling() &&
>           !(GetStateBits() & NS_FRAME_IS_DIRTY) &&

Undo this change (as you did in the followup on mozilla-central), and r=dbaron
Attachment #632732 - Flags: review+
Comment on attachment 632768 [details] [diff] [review]
b695222-backout-fx13: Backout patch for release

>-[builtinclass, scriptable, uuid(76732e62-da09-4aef-850a-78b9f6d5c8cf)]
>+[builtinclass, scriptable, uuid(35b2c7f0-b56c-11e1-afa6-0800200c9a66)]

Need a different UUID here, as I said above.

>@@ -673,17 +665,17 @@ nsColumnSetFrame::ReflowChildren(nsHTMLR
>         child->AddStateBits(NS_FRAME_IS_DIRTY);
> 
>       nsHTMLReflowState kidReflowState(PresContext(), aReflowState, child,
>                                        availSize, availSize.width,
>                                        aReflowState.ComputedHeight());
>       kidReflowState.mFlags.mIsTopOfPage = true;
>       kidReflowState.mFlags.mTableIsSplittable = false;
>       kidReflowState.mFlags.mIsColumnBalancing = aConfig.mBalanceColCount < PR_INT32_MAX;
>-          
>+
> #ifdef DEBUG_roc
>       printf("*** Reflowing child #%d %p: availHeight=%d\n",
>              columnCount, (void*)child,availSize.height);
> #endif
> 
>       // Note if the column's next in flow is not being changed by this incremental reflow.
>       // This may allow the current column to avoid trying to pull lines from the next column.
>       if (child->GetNextSibling() &&

And drop this change.


r=dbaron with that.

I expect this patch will apply as-is to mozilla-beta as well.
Attachment #632768 - Flags: review+
(In reply to David Baron [:dbaron] from comment #39)
> You need to be more careful with the UUIDs; if the interface is different,
> you want different UUIDs, so while you're correct to use the same UUID on
> mozilla-central and mozilla-aurora (since the interface is currently the
> same on both), you need a different UUID for mozilla-release and
> mozilla-beta (which are the same as each other, but different from
> mozilla-aurora and mozilla-central).

Oh.. the UUID needs to be different on each branch? Ok, I didn't know that, I'll change them.
[Approval Request Comment]
(See above)
Attachment #632793 - Flags: review?(dbaron)
Attachment #632793 - Flags: approval-mozilla-beta?
(In reply to Scott Johnson (:jwir3) from comment #42)
> (In reply to David Baron [:dbaron] from comment #39)
> > You need to be more careful with the UUIDs; if the interface is different,
> > you want different UUIDs, so while you're correct to use the same UUID on
> > mozilla-central and mozilla-aurora (since the interface is currently the
> > same on both), you need a different UUID for mozilla-release and
> > mozilla-beta (which are the same as each other, but different from
> > mozilla-aurora and mozilla-central).
> 
> Oh.. the UUID needs to be different on each branch? Ok, I didn't know that,
> I'll change them.

Or, rather, what I meant was that the UUIDs need to be the same for aurora and nightly and for beta and release (but those two UUIDs should be different from each other, and what they were at before).
Forgot to remove the extra line changed.
Attachment #632793 - Attachment is obsolete: true
Attachment #632793 - Flags: review?(dbaron)
Attachment #632793 - Flags: approval-mozilla-beta?
Attachment #632799 - Flags: review?(dbaron)
Attachment #632799 - Flags: approval-mozilla-beta?
Posting patch with dbaron's requested changes.

[Approval Request Comment]
(See above)
Attachment #632768 - Attachment is obsolete: true
Attachment #632768 - Flags: approval-mozilla-release?
Attachment #632801 - Flags: review+
Attachment #632801 - Flags: approval-mozilla-release?
Comment on attachment 632801 [details] [diff] [review]
b695222-backout-fx13: Backout patch for release

[Triage Comment]
Approved for release - low risk correctness fix that returns us to our previous behavior (no moz-column-fill).
Attachment #632801 - Flags: approval-mozilla-release? → approval-mozilla-release+
Comment on attachment 632799 [details] [diff] [review]
b695222-backout-fx14: Backout patch for beta

Same patch as the central one.
Attachment #632799 - Flags: review?(dbaron) → review+
Attachment #632732 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #632799 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 764567
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Can someone please clarify the verification for the backout? Since this is a backout, the testcase should still reproduce this bug, correct?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #50)
> Can someone please clarify the verification for the backout? Since this is a
> backout, the testcase should still reproduce this bug, correct?

Not quite. This bug is a regression caused by bug 695222. So, by backing out bug 695222, we fixed this bug. The testcase for bug 695222 should still be unfixed, but because of clutter within that bug, I opened another one.
So, just to reiterate, the testcase attached to this bug should no longer reproduce, and the testcase in bug 695222 should still reproduce?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #52)
> So, just to reiterate, the testcase attached to this bug should no longer
> reproduce, and the testcase in bug 695222 should still reproduce?

Basically, yes, this is correct. But, since bug 695222 was an added feature, we simply don't support the 'column-fill' or '-moz-column-fill' CSS property for the time being. (There isn't really a test case for bug 695222 is what I'm trying to say).
Backout verified for Firefox 13.0.1 as per comment 53.
Just to be sure to understand this correctly to update the doc: we don't support (-moz-)column-fill on any Fx version yet (except on Fx 13.0.0, but this is anecdotical as removed in 13.0.1)

Am I right?
(In reply to Jean-Yves Perrier [:teoli] from comment #56)
> Just to be sure to understand this correctly to update the doc: we don't
> support (-moz-)column-fill on any Fx version yet (except on Fx 13.0.0, but
> this is anecdotical as removed in 13.0.1)
> 
> Am I right?

This is correct.
So if this has now been backed out of all affected branches, can we update the status flags to 'fixed'?
(In reply to Lukas Blakk [:lsblakk] from comment #58)
> So if this has now been backed out of all affected branches, can we update
> the status flags to 'fixed'?

Yep. And done. :)
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #55)
> Backout verified for Firefox 13.0.1 as per comment 53.

Able to see the issue on Nightly 2012-03-15 with the test cases from comment 0 and comment 26.
Verified fixed on FF 14b10 on Win 7, Ubuntu 12.04 and Mac OS X 10.6.
Verified fixed on FF 15b1 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.4.
Verified fixed on FF 16b5 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.4.
Status: RESOLVED → VERIFIED
Depends on: 819600
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: