Fix and re-land patches to support multicolumn 'column-fill' property

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
7 months ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

(Depends on: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

5 years ago
Assignee: nobody → sjohnson
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

5 years ago
Duplicate of this bug: 695222
(Assignee)

Updated

5 years ago
Blocks: 733614
(Assignee)

Updated

5 years ago
(Assignee)

Comment 2

5 years ago
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
Attachment #640269 - Flags: review?(roc)
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 (
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

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

Comment 8

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

Comment 9

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

Comment 10

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

Updated

5 years ago
Blocks: 773823
(Assignee)

Comment 11

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

Comment 12

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

Comment 14

5 years ago
Comment on attachment 640269 [details] [diff] [review]
b764567 (v1)

Adding review+ based on comment 12 and comment 13.
Attachment #640269 - Flags: review?(roc) → review+
(Assignee)

Comment 15

5 years ago
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d30fa24371
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/34d30fa24371
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 810726

Updated

3 years ago
Depends on: 986198

Comment 17

7 months ago
Firefox 52.0a1 buildID=20161013030204 passes the 8 'column-fill' tests in

http://test.csswg.org/suites/css-multicol-1_dev/nightly-unstable/html4/chapter-7.htm

Note that

http://test.csswg.org/suites/css-multicol-1_dev/nightly-unstable/html4/multicol-fill-auto.htm

is linked to an incorrect reference file. In that multicol-fill-auto.htm

<link rel="match" href="reference/multicol-fill-ref.htm">

should be instead

<link rel="match" href="reference/multicol-fill-auto-002-ref.htm">

I'm going to fix this shortly.

Comment 18

7 months ago
(...) 

> I'm going to fix this shortly.

I must contact the author first... as the reference file is also not compact, not streamlined...

- - - - - - -

I have updated this column-fill test:

http://www.gtalbot.org/BrowserBugsSection/CSS3Multi-Columns/column-fill-balance-0xx.html

The difference of rendering for 'column-fill: auto' between, on one hand, Firefox 52 and, on the other hand, Opera 12.16 (Presto engine), Chrome 54 and Chrome 55 is that Opera and Chrome implement 'widows: 2' on column boxes while Firefox does not.
You need to log in before you can comment on or make changes to this bug.