Last Comment Bug 764567 - Fix and re-land patches to support multicolumn 'column-fill' property
: Fix and re-land patches to support multicolumn 'column-fill' property
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
http://www.metareads.com/articles/rea...
: 695222 (view as bug list)
Depends on: 986198 810726
Blocks: 733614 773823
  Show dependency treegraph
 
Reported: 2012-06-13 13:57 PDT by Scott Johnson (:jwir3)
Modified: 2014-03-20 18:40 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
b764567 (v1) (59.65 KB, patch)
2012-07-09 10:22 PDT, Scott Johnson (:jwir3)
jaywir3: review+
Details | Diff | Splinter Review
columnfill-nonbreak-div.html (TESTCASE) (1.07 KB, text/html)
2012-07-10 13:29 PDT, Scott Johnson (:jwir3)
no flags Details
opera-testcase-example.png (69.13 KB, image/png)
2012-07-10 13:31 PDT, Scott Johnson (:jwir3)
no flags Details
image test case examples multiple browsers (106.64 KB, image/png)
2012-07-12 08:53 PDT, Scott Johnson (:jwir3)
no flags Details

Description Scott Johnson (:jwir3) 2012-06-13 13:57:34 PDT
Since bug 695222 was backed out to fix regressions in bug 733614, these patches need to be re-landed. As part of this work, we should integrate the fix for bug 733614, which is currently incomplete.

(I created a new bug so that we can avoid the clutter of the other two which have multiple patches each).
Comment 1 Scott Johnson (:jwir3) 2012-06-13 13:58:42 PDT
*** Bug 695222 has been marked as a duplicate of this bug. ***
Comment 2 Scott Johnson (:jwir3) 2012-07-09 10:22:42 PDT
Created attachment 640269 [details] [diff] [review]
b764567 (v1)

Most of this patch has already been reviewed as part of bug 695222, but there are some parts that were originally implemented in bug 733614. (See bug 733614, comment 24 for reference on the previous implementation and to jog your memory about these changes). 

I believe I've made the changes necessary to complete the column-fill implementation, as well as fix regressions that were found in bug 733614 and issues you found during the initial review, roc. 

It's passed try server as well: https://tbpl.mozilla.org/?tree=Try&rev=eb89a6529b49
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-09 20:52:39 PDT
Comment on attachment 640269 [details] [diff] [review]
b764567 (v1)

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

::: layout/generic/nsColumnSetFrame.cpp
@@ +746,5 @@
> +
> +      if ((contentBottom > aReflowState.mComputedMaxHeight ||
> +          contentBottom > aReflowState.ComputedHeight()) &&
> +          aConfig.mBalanceColCount < PR_INT32_MAX &&
> +          !aColData.mShouldRevertToAuto) {

final mShouldRevertToAuto check here is not needed.

@@ +750,5 @@
> +          !aColData.mShouldRevertToAuto) {
> +        // We overflowed vertically, but have not exceeded the number
> +        // of columns. If we're balancing, then we should try reverting
> +        // to auto instead.
> +        aColData.mShouldRevertToAuto = true;

Why are we reverting to auto just because one column overflowed?

It seems to me that if one column has a very tall element in it that we can't break, it still makes sense to balance as much as we can instead of reverting to auto.

@@ +772,5 @@
> +                   contentBottom > aReflowState.ComputedHeight()) {
> +          aColData.mShouldRevertToAuto = true;
> +        } else {
> +          // This isn't a feasible case - it's a situation where we are
> +          // attempting to balance columns, but the column count is too high.

That's ambiguous. say "the number of columns required is too high".

@@ +1099,3 @@
>      }
>  
> + } while(colData.mShouldRevertToAuto);

space before (
Comment 4 Scott Johnson (:jwir3) 2012-07-10 09:21:04 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)

> final mShouldRevertToAuto check here is not needed.
Fixed.

> Why are we reverting to auto just because one column overflowed?
> 
> It seems to me that if one column has a very tall element in it that we
> can't break, it still makes sense to balance as much as we can instead of
> reverting to auto.

This is the situation we're running into with https://bug733614.bugzilla.mozilla.org/attachment.cgi?id=604079 (the testcase from bug 733614). It seems like what is happening is that all of the columns are overflowing vertically beyond their specified height, because one of the columns has too much content, and rather than add an additional column, we attempt to balance at a height that is incorrect.

I'm still looking into this. What would be a case where we have an element that's very tall and can't be broken? Would an image with height equal to or greater than the height of the column be a case like this?

> That's ambiguous. say "the number of columns required is too high".
Fixed.

> space before (
Fixed.
Comment 5 Scott Johnson (:jwir3) 2012-07-10 13:29:56 PDT
Created attachment 640753 [details]
columnfill-nonbreak-div.html (TESTCASE)

(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)

> > Why are we reverting to auto just because one column overflowed?
> > 
> > It seems to me that if one column has a very tall element in it that we
> > can't break, it still makes sense to balance as much as we can instead of
> > reverting to auto.

So, I've posted another test case. This one has an div set with display: inline-block inside of the first column. So, (I think) this shows the behavior you're talking about, roc. It looks like Opera does the same thing as we do in this case - that is, column-fill: auto and column-fill: balance are the same in this particular case. 

It seems to me this is our best option, because otherwise, I think we'd have to change ChooseColumnStrategy() to take the height into account. Right now, it doesn't take the height into account, so while we could add another column in the event that a particular column overflows vertically, this isn't taken into account by ChooseColumnStrategy, which sets mBalanceColCount initially. That means, we're still going to end up in the case where columnCount >= mBalanceColCount anyway, which means the logic will revert to the behavior of 'column-fill: auto'. 

Of course, it is possible I'm just missing something here. Thoughts?
Comment 6 Scott Johnson (:jwir3) 2012-07-10 13:31:18 PDT
Created attachment 640757 [details]
opera-testcase-example.png

Screenshot of opera with the testcase mentioned. Opera renders the same way regardless of whether column-fill: auto or column-fill: balance is specified.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-10 17:37:49 PDT
(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> > Why are we reverting to auto just because one column overflowed?
> > 
> > It seems to me that if one column has a very tall element in it that we
> > can't break, it still makes sense to balance as much as we can instead of
> > reverting to auto.
> 
> This is the situation we're running into with
> https://bug733614.bugzilla.mozilla.org/attachment.cgi?id=604079 (the
> testcase from bug 733614). It seems like what is happening is that all of
> the columns are overflowing vertically beyond their specified height,
> because one of the columns has too much content, and rather than add an
> additional column, we attempt to balance at a height that is incorrect.

That's not an example of what I was thinking of. With that testcase we should never have a vertically overflowing column. Maybe we need to add some code to ensure that the balance trial height is never greater than the computed height or max-height?

> I'm still looking into this. What would be a case where we have an element
> that's very tall and can't be broken? Would an image with height equal to or
> greater than the height of the column be a case like this?

Right.

(In reply to Scott Johnson (:jwir3) from comment #5)
> So, I've posted another test case. This one has an div set with display:
> inline-block inside of the first column. So, (I think) this shows the
> behavior you're talking about, roc. It looks like Opera does the same thing
> as we do in this case - that is, column-fill: auto and column-fill: balance
> are the same in this particular case.

In that testcase, the content doesn't fit in the available width and height so balancing must be disabled no matter what. A more interesting testcase would be to have the tall image followed by just two lines of text. Do those lines get balanced across the remaining columns or not? I say they should.
Comment 8 Scott Johnson (:jwir3) 2012-07-12 08:30:56 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> That's not an example of what I was thinking of. With that testcase we
> should never have a vertically overflowing column. Maybe we need to add some
> code to ensure that the balance trial height is never greater than the
> computed height or max-height?

Yes, you're probably right, and this will likely fix the case I was concerned about.

> A more interesting testcase would be to have the tall image followed
> by just two lines of text. Do those lines get balanced across the
> remaining columns or not? I say they should.

Yes, I think I agree with you, because Example 26 on http://dev.w3.org/csswg/css3-multicol/#overflow-inside-multicol-elements seems to indicate that if there is a break in the columns in the block direction (in this case, due to a page break/insufficient room on a page), then the two sets of columns are balanced individually. I would think the same thing should happen if we have an element that overflows vertically outside of the column, but have data remaining to be placed in additional column blocks. I'll construct a test case and see what other UAs do in this situation.
Comment 9 Scott Johnson (:jwir3) 2012-07-12 08:53:42 PDT
Created attachment 641498 [details]
image test case examples multiple browsers

This is an example of what is happening on different browsers when an image that is too large for the given column vertically (see http://people.mozilla.org/~sjohnson/b764567/columnfill-nonbreak-img.html for the test case itself) is added to a multicolumn element, along with a small amount of text. 

For some strange reason, webkit breaks the image and separates it between columns. This is a documented bug - https://bugs.webkit.org/show_bug.cgi?id=25633 with webkit, though.
Comment 10 Scott Johnson (:jwir3) 2012-07-13 15:00:58 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Maybe we need to add some
> code to ensure that the balance trial height is never greater than the
> computed height or max-height?

I've added code to do this, and it eliminates the need for the extra block where we abort balancing because one column is too large vertically. I'm just polishing up a couple of things, then I will post another patch for review.

> In that testcase, the content doesn't fit in the available width and height
> so balancing must be disabled no matter what. A more interesting testcase
> would be to have the tall image followed by just two lines of text. Do those
> lines get balanced across the remaining columns or not? I say they should.

I'm going to file another bug to track this issue, since it's related, but separate (at least in my mind), and can be tackled separately from column-fill itself. Also, as I previously mentioned, if we change this, we'll be the only UA (AFAICT) that would do it this way. That might be good, but it's also something that should probably be handled separately, in case the behavior is not in line with spec (or determined to be so at a later date), and thus needs to be backed out.
Comment 11 Scott Johnson (:jwir3) 2012-07-14 19:09:22 PDT
(In reply to Scott Johnson (:jwir3) from comment #10)
> I'm going to file another bug to track this issue, since it's related, but
> separate (at least in my mind), and can be tackled separately from
> column-fill itself. Also, as I previously mentioned, if we change this,
> we'll be the only UA (AFAICT) that would do it this way. That might be good,
> but it's also something that should probably be handled separately, in case
> the behavior is not in line with spec (or determined to be so at a later
> date), and thus needs to be backed out.

I've done this: bug 773823.
Comment 12 Scott Johnson (:jwir3) 2012-07-18 12:20:27 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from bug 773823 comment #6)
> Yes, I hadn't thought through the issues when i wrote that comment.

Hm, so do you agree then with the behavior of the patch? :D
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-18 19:56:42 PDT
Yes!
Comment 14 Scott Johnson (:jwir3) 2012-07-30 16:49:31 PDT
Comment on attachment 640269 [details] [diff] [review]
b764567 (v1)

Adding review+ based on comment 12 and comment 13.
Comment 15 Scott Johnson (:jwir3) 2012-07-31 09:22:32 PDT
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d30fa24371
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-07-31 19:18:45 PDT
https://hg.mozilla.org/mozilla-central/rev/34d30fa24371

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