Last Comment Bug 733614 - CSS 'height' property on a multi-column block is not respected
: CSS 'height' property on a multi-column block is not respected
Status: VERIFIED FIXED
[qa+]
: dev-doc-complete, regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 12 Branch
: All All
: P1 normal with 1 vote (vote)
: ---
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
: 733691 (view as bug list)
Depends on: 1250844 764567 819600
Blocks: 695222
  Show dependency treegraph
 
Reported: 2012-03-06 17:01 PST by Loic
Modified: 2016-03-05 01:12 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
verified
+
verified
+
verified
+
verified


Attachments
testcase.html (10.75 KB, text/html)
2012-03-06 17:01 PST, Loic
no flags Details
Exsample XXIII of http://www.w3.org/TR/css3-multicol/#pagination-and-overflow-outside-multicol (1.92 KB, text/html)
2012-03-08 08:33 PST, Alice0775 White
no flags Details
screenshot for Example XXIII (comparison with Chrome, Opera, Firefox11Beta and Aurora12.0a1) (183.99 KB, image/png)
2012-03-08 08:46 PST, Alice0775 White
no flags Details
b695222-backout: Backout bug 695222 on mozilla-beta (38.70 KB, patch)
2012-03-29 10:35 PDT, Scott Johnson (:jwir3)
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
columnfill-regression testcase - max-width and max-height specified. (4.67 KB, text/html)
2012-04-17 12:42 PDT, Scott Johnson (:jwir3)
no flags Details
screenshot of chrome 18.0 with overflow from attached columnfill testcase (199.17 KB, image/png)
2012-04-17 12:43 PDT, Scott Johnson (:jwir3)
no flags Details
Chart of multicol overflow support in 3 browsers (16.12 KB, text/html)
2012-04-17 13:27 PDT, Scott Johnson (:jwir3)
no flags Details
b733614 (22.94 KB, patch)
2012-04-23 14:44 PDT, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
b695222-backout-fx16: Backout patch for central (40.70 KB, patch)
2012-06-12 14:55 PDT, Scott Johnson (:jwir3)
dbaron: review+
Details | Diff | Splinter Review
b695222-backout-fx15: Backout patch for aurora (43.77 KB, patch)
2012-06-13 09:21 PDT, Scott Johnson (:jwir3)
dbaron: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
b695222-backout-fx13: Backout patch for release (43.79 KB, patch)
2012-06-13 10:57 PDT, Scott Johnson (:jwir3)
dbaron: review+
Details | Diff | Splinter Review
b695222-backout-fx14: Backout patch for beta (43.79 KB, patch)
2012-06-13 11:41 PDT, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
b695222-backout-fx14: Backout patch for beta (42.89 KB, patch)
2012-06-13 11:57 PDT, Scott Johnson (:jwir3)
jaywir3: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
b695222-backout-fx13: Backout patch for release (43.79 KB, patch)
2012-06-13 12:02 PDT, Scott Johnson (:jwir3)
jaywir3: review+
dbaron: review+
akeybl: approval‑mozilla‑release+
Details | Diff | Splinter Review

Description Loic 2012-03-06 17:01:41 PST
Created attachment 603521 [details]
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
Comment 1 Alice0775 White 2012-03-06 20:03:52 PST
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
Comment 2 Loic 2012-03-07 06:19:27 PST
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.
Comment 3 Alice0775 White 2012-03-07 11:07:21 PST
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.
Comment 5 Alice0775 White 2012-03-08 08:46:00 PST
Created attachment 604083 [details]
screenshot for Example XXIII (comparison with Chrome, Opera, Firefox11Beta and Aurora12.0a1)
Comment 6 Phoenix 2012-03-09 02:46:31 PST
*** Bug 733691 has been marked as a duplicate of this bug. ***
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 19:05:36 PDT
Yes, I think it is too.
Comment 8 Alex Keybl [:akeybl] 2012-03-28 16:15:58 PDT
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.
Comment 9 Scott Johnson (:jwir3) 2012-03-28 17:05:57 PDT
Modified priority to reflect that it's tracking for FF12.
Comment 10 Scott Johnson (:jwir3) 2012-03-28 23:10:07 PDT
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?
Comment 11 Scott Johnson (:jwir3) 2012-03-29 10:35:37 PDT
Created attachment 610589 [details] [diff] [review]
b695222-backout: Backout bug 695222 on mozilla-beta

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
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-29 14:24:33 PDT
(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 13 Alex Keybl [:akeybl] 2012-03-29 15:27:44 PDT
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.
Comment 14 Scott Johnson (:jwir3) 2012-03-30 11:34:45 PDT
Backed out of FF12:
https://hg.mozilla.org/releases/mozilla-beta/rev/fe494e9c9714
Comment 15 Jean-Yves Perrier [:teoli] 2012-03-30 11:53:30 PDT
I'm adding dev-doc-needed as we already documented this and have to correct it now column-fill has been (temporary) backed out.
Comment 16 Jean-Yves Perrier [:teoli] 2012-03-30 12:16:24 PDT
So I've removed the mention from
https://developer.mozilla.org/index.php?title=en/Firefox_12_for_developers
and added it on
https://developer.mozilla.org/index.php?title=en/Firefox_13_for_developers
and updated the compatibility table (12->13) on
https://developer.mozilla.org/en/CSS/column-fill#section_5
Comment 17 Jean-Yves Perrier [:teoli] 2012-03-30 12:22:18 PDT
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.
Comment 18 Scott Johnson (:jwir3) 2012-04-17 12:42:26 PDT
Created attachment 615829 [details]
columnfill-regression testcase - max-width and max-height specified.

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.
Comment 19 Scott Johnson (:jwir3) 2012-04-17 12:43:28 PDT
Created attachment 615830 [details]
screenshot of chrome 18.0 with overflow from attached columnfill testcase
Comment 20 Scott Johnson (:jwir3) 2012-04-17 12:44:27 PDT
WRT comment 18, obviously our rendering is incorrect currently, but I'm curious as to what the behavior SHOULD be.
Comment 21 Scott Johnson (:jwir3) 2012-04-17 13:27:39 PDT
Created attachment 615848 [details]
Chart of multicol overflow support in 3 browsers

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.
Comment 22 Scott Johnson (:jwir3) 2012-04-23 14:44:05 PDT
Created attachment 617650 [details] [diff] [review]
b733614

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!
Comment 23 Alex Keybl [:akeybl] 2012-04-24 17:32:46 PDT
Backed out of FF13: https://hg.mozilla.org/releases/mozilla-beta/rev/e60ca2e387a8
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-24 22:38:38 PDT
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.
Comment 25 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-04 14:09:51 PDT
Backed out of FF14: http://hg.mozilla.org/releases/mozilla-beta/rev/564eb0ab7580
Comment 26 Marek 2012-06-10 10:58:54 PDT
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).
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-10 12:05:34 PDT
(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?
Comment 28 Alex Keybl [:akeybl] 2012-06-10 17:05:20 PDT
(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?
Comment 29 Scott Johnson (:jwir3) 2012-06-12 11:18:35 PDT
(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.
Comment 30 Scott Johnson (:jwir3) 2012-06-12 14:55:22 PDT
Created attachment 632424 [details] [diff] [review]
b695222-backout-fx16: Backout patch for central

Backout patch for mozilla-central
Comment 31 Scott Johnson (:jwir3) 2012-06-12 14:58:12 PDT
The others are still compiling so I can verify them. As soon as they are done, I'll post them here.
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-12 17:09:41 PDT
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
Comment 33 Alex Keybl [:akeybl] 2012-06-13 06:32:43 PDT
(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?
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-13 06:43:01 PDT
(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.
Comment 35 Scott Johnson (:jwir3) 2012-06-13 09:21:03 PDT
Created attachment 632732 [details] [diff] [review]
b695222-backout-fx15: Backout patch for aurora

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
Comment 36 Scott Johnson (:jwir3) 2012-06-13 09:23:08 PDT
Backed out on central:
https://hg.mozilla.org/mozilla-central/rev/aeaa00b5cab5
Comment 37 Scott Johnson (:jwir3) 2012-06-13 10:18:39 PDT
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
Comment 38 Scott Johnson (:jwir3) 2012-06-13 10:57:19 PDT
Created attachment 632768 [details] [diff] [review]
b695222-backout-fx13: Backout patch for release

Backout patch for release branch.

[Approval Request Comment]
(See above)
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-13 11:24:15 PDT
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 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-13 11:28:48 PDT
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
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-13 11:30:17 PDT
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.
Comment 42 Scott Johnson (:jwir3) 2012-06-13 11:36:48 PDT
(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.
Comment 43 Scott Johnson (:jwir3) 2012-06-13 11:41:37 PDT
Created attachment 632793 [details] [diff] [review]
b695222-backout-fx14: Backout patch for beta

[Approval Request Comment]
(See above)
Comment 44 Scott Johnson (:jwir3) 2012-06-13 11:43:50 PDT
(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).
Comment 45 Scott Johnson (:jwir3) 2012-06-13 11:57:05 PDT
Created attachment 632799 [details] [diff] [review]
b695222-backout-fx14: Backout patch for beta

Forgot to remove the extra line changed.
Comment 46 Scott Johnson (:jwir3) 2012-06-13 12:02:38 PDT
Created attachment 632801 [details] [diff] [review]
b695222-backout-fx13: Backout patch for release

Posting patch with dbaron's requested changes.

[Approval Request Comment]
(See above)
Comment 47 Alex Keybl [:akeybl] 2012-06-13 13:14:35 PDT
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).
Comment 48 Scott Johnson (:jwir3) 2012-06-13 13:23:35 PDT
Comment on attachment 632799 [details] [diff] [review]
b695222-backout-fx14: Backout patch for beta

Same patch as the central one.
Comment 49 Scott Johnson (:jwir3) 2012-06-13 14:00:00 PDT
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/65f752c3498f

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/235f5dbfe5aa

Pushed to release:
https://hg.mozilla.org/releases/mozilla-release/rev/fa25fc204958

Followup fixes will be done as part of re-landing bug 764567.
Comment 50 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-13 14:49:46 PDT
Can someone please clarify the verification for the backout? Since this is a backout, the testcase should still reproduce this bug, correct?
Comment 51 Scott Johnson (:jwir3) 2012-06-13 15:04:58 PDT
(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.
Comment 52 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-13 15:16:41 PDT
So, just to reiterate, the testcase attached to this bug should no longer reproduce, and the testcase in bug 695222 should still reproduce?
Comment 53 Scott Johnson (:jwir3) 2012-06-13 15:20:58 PDT
(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).
Comment 55 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-14 15:48:53 PDT
Backout verified for Firefox 13.0.1 as per comment 53.
Comment 56 Jean-Yves Perrier [:teoli] 2012-06-18 05:50:07 PDT
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?
Comment 57 Scott Johnson (:jwir3) 2012-06-18 08:59:39 PDT
(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.
Comment 58 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 10:28:47 PDT
So if this has now been backed out of all affected branches, can we update the status flags to 'fixed'?
Comment 59 Scott Johnson (:jwir3) 2012-06-18 10:40:13 PDT
(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. :)
Comment 60 Paul Silaghi, QA [:pauly] 2012-07-03 23:05:59 PDT
(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.
Comment 61 Paul Silaghi, QA [:pauly] 2012-07-19 07:13:58 PDT
Verified fixed on FF 15b1 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.4.
Comment 62 Paul Silaghi, QA [:pauly] 2012-10-02 06:13:19 PDT
Verified fixed on FF 16b5 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.4.

Note You need to log in before you can comment on or make changes to this bug.