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

VERIFIED FIXED

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: Loic, Assigned: jwir3)

Tracking

(Depends on: 1 bug, {dev-doc-complete, regression})

12 Branch
dev-doc-complete, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12+ fixed, firefox13+ verified, firefox14+ verified, firefox15+ verified, firefox16+ verified)

Details

(Whiteboard: [qa+])

Attachments

(11 attachments, 3 obsolete attachments)

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
(Reporter)

Description

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

Updated

5 years ago
Component: Untriaged → Layout
Product: Firefox → Core
Hardware: x86_64 → All
(Reporter)

Updated

5 years ago
Attachment #603521 - Attachment mime type: text/plain → text/html

Comment 1

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

Comment 2

5 years ago
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

5 years ago
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 4

5 years ago
Created attachment 604079 [details]
Exsample XXIII of http://www.w3.org/TR/css3-multicol/#pagination-and-overflow-outside-multicol

Comment 5

5 years ago
Created attachment 604083 [details]
screenshot for Example XXIII (comparison with Chrome, Opera, Firefox11Beta and Aurora12.0a1)

Updated

5 years ago
Duplicate of this bug: 733691
Yes, I think it is too.
Assignee: nobody → sjohnson
(Assignee)

Updated

5 years ago
Priority: -- → P3

Updated

5 years ago
QA Contact: untriaged → layout
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?

Updated

5 years ago
tracking-firefox12: ? → +
tracking-firefox13: ? → +
tracking-firefox14: ? → +

Comment 8

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

Updated

5 years ago
Priority: P3 → P1
(Assignee)

Comment 9

5 years ago
Modified priority to reflect that it's tracking for FF12.
(Assignee)

Comment 10

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

Comment 11

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

Comment 14

5 years ago
Backed out of FF12:
https://hg.mozilla.org/releases/mozilla-beta/rev/fe494e9c9714
(Assignee)

Updated

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

Comment 18

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

Comment 19

5 years ago
Created attachment 615830 [details]
screenshot of chrome 18.0 with overflow from attached columnfill testcase
(Assignee)

Comment 20

5 years ago
WRT comment 18, obviously our rendering is incorrect currently, but I'm curious as to what the behavior SHOULD be.
(Assignee)

Comment 21

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

Comment 22

5 years ago
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!
Attachment #617650 - Flags: review?(roc)
Backed out of FF13: https://hg.mozilla.org/releases/mozilla-beta/rev/e60ca2e387a8
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.

Updated

5 years ago
status-firefox13: --- → fixed
Backed out of FF14: http://hg.mozilla.org/releases/mozilla-beta/rev/564eb0ab7580
status-firefox14: --- → fixed

Comment 26

5 years ago
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?
status-firefox13: fixed → affected
status-firefox14: fixed → affected
(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?
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox15: --- → +
tracking-firefox16: --- → +
(Assignee)

Comment 29

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

Comment 30

5 years ago
Created attachment 632424 [details] [diff] [review]
b695222-backout-fx16: Backout patch for central

Backout patch for mozilla-central
Attachment #610589 - Attachment is obsolete: true
Attachment #632424 - Flags: review?(dbaron)
(Assignee)

Comment 31

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

Comment 35

5 years ago
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
Attachment #632732 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

5 years ago
Backed out on central:
https://hg.mozilla.org/mozilla-central/rev/aeaa00b5cab5
(Assignee)

Comment 37

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

Comment 38

5 years ago
Created attachment 632768 [details] [diff] [review]
b695222-backout-fx13: Backout patch for release

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+
(Assignee)

Comment 42

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

Comment 43

5 years ago
Created attachment 632793 [details] [diff] [review]
b695222-backout-fx14: Backout patch for beta

[Approval Request Comment]
(See above)
Attachment #632793 - Flags: review?(dbaron)
Attachment #632793 - Flags: approval-mozilla-beta?
(Assignee)

Comment 44

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

Comment 45

5 years ago
Created attachment 632799 [details] [diff] [review]
b695222-backout-fx14: Backout patch for beta

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

Comment 46

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

Comment 48

5 years ago
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+

Updated

5 years ago
Attachment #632732 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #632799 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #632801 - Flags: review+
(Assignee)

Comment 49

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

Updated

5 years ago
Depends on: 764567
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 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?
(Assignee)

Comment 51

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

Comment 53

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

Comment 54

5 years ago
This had to be re-backed out on beta and release due to crashtest oranges from bug 724978:

https://hg.mozilla.org/releases/mozilla-release/rev/ada112dc8c7a
and
https://hg.mozilla.org/releases/mozilla-beta/rev/a672276a745e

and then it was re-applied:
https://hg.mozilla.org/releases/mozilla-release/rev/99a198ee453c
and
https://hg.mozilla.org/releases/mozilla-beta/rev/4a4307e626c3
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?
(Assignee)

Comment 57

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

Comment 59

5 years ago
(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. :)
status-firefox13: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
status-firefox16: affected → fixed
(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.
status-firefox13: fixed → verified
status-firefox14: fixed → verified
Verified fixed on FF 15b1 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.4.
status-firefox15: fixed → verified
Verified fixed on FF 16b5 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.4.
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified

Updated

5 years ago
Depends on: 819600
Depends on: 1250844
Attachment #617650 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.