1173689 - Multi-columns in orthogonal writing-mode overflow their container

Status: RESOLVED FIXED

(Core :: Layout: Columns, defect)

Description

[Simon Montagu :smontagu]

Attachment: Testcase with column-count

The testcase shows a problem with multi-column layout in orthogonal blocks, which is a use case likely to be common for real-world sites with vertical text.

In horizontal writing modes, the wrapper expands in its block-size direction to fit the content. In vertical writing-modes, the content overflows.

Comment 1

[Jonathan Kew [:jfkthame]]

It's not clear to me what you are suggesting should happen here. The testcase specifies a max-block-size on the test element, and therefore it's not expected to expand in its block-size direction. If you remove that constraint, then it will indeed overflow the width of the enclosing wrapper.

Comment 2

[Simon Montagu :smontagu]

I expect that since the max-block-size forces the test element to expand (by creating overflow columns) in its inline-size direction, the wrapper should expand in its block-size direction to fit.

Comment 3

[Simon Montagu :smontagu]

Attachment: Testcase with column width

Maybe this is a better example. Is it not reasonable that in both horizontal and vertical writing modes the wrapper should expand to fit the content?

Comment 4

[Jonathan Kew [:jfkthame]]

Oh, I see what you mean. Sorry.... not enough coffee, I guess!

How do other browsers deal with this? Does the spec address it?

The trouble with expanding the block-size of the wrapper is that this would change the inline-size of the test element, thereby changing its (automatic) column width and forcing a reflow... and a potential feedback loop, probably.

Comment 5

[Jonathan Kew [:jfkthame]]

(comment 4 was in reference to the original example)

Comment 6

[Simon Montagu :smontagu]

My starting point for all this is the part in the spec about auto-multi-col for orthogonal flows: http://dev.w3.org/csswg/css-writing-modes-3/#auto-multicol

There has been some discussion about removing or deferring this section (https://lists.w3.org/Archives/Public/www-style/2015May/0283.html), and I also have my reservations about making multi-col happen automagically, but I was experimenting with setting properties manually to get the same effect.

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch v.0

What do you think of this approach? Is it too much of a hack?

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8623079 [details] [diff] [review]
Patch v.0

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

It seems logical enough, if it works as it looks like it would. I'd add a comment at the ChooseColumnStrategy call to explain what's being done. Maybe get :roc to review? I think he knows the column-set code best, and may be more aware of any traps lurking in there for the unwary...

Comment 9

[Simon Montagu :smontagu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e7c69fe8a48

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch v.1 with added and improved comments

Comment 11

[Simon Montagu :smontagu]

Attachment: News site with the patch

Just for fun, here's a screenshot of a Japanese news site with the patch and with vertical writing-mode and column-width set on the article body via userContent.css. To my eyes it looks quite similar to the formatting in the print edition.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8623506 [details] [diff] [review]
Patch v.1 with added and improved comments

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

::: layout/generic/nsColumnSetFrame.cpp
@@ +788,5 @@
>        aReflowState.ApplyMinMaxBSize(contentSize.BSize(wm),
>                                      aConfig.mConsumedBSize);
>    }
> +  if (aReflowState.ComputedISize() != NS_INTRINSICSIZE &&
> +      aReflowState.AvailableISize() != NS_INTRINSICSIZE) {

Ignoring the computed ISize seems like a problem. E.g. in vertical writing if someone sets "height:100px" that's what they should get on the element.

@@ +1059,5 @@
> +  // happens in orthogonal flows where the inline direction is the
> +  // container's block direction).
> +  ReflowConfig config =
> +    ChooseColumnStrategy(aReflowState,
> +                         aReflowState.AvailableISize() == NS_UNCONSTRAINEDSIZE);

Again, I don't see why we should override a computed ISize here.

::: layout/generic/nsHTMLReflowState.cpp
@@ +449,5 @@
> +    // container's block direction
> +    if (type == nsGkAtoms::columnSetFrame) {
> +      AvailableISize() = NS_UNCONSTRAINEDSIZE;
> +    } else {
> +      AvailableBSize() = NS_UNCONSTRAINEDSIZE;

Maybe we should change the computed value here too, if the style was "auto"?

Comment 13

[Simon Montagu :smontagu]

Attachment: Patch v.2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> ::: layout/generic/nsHTMLReflowState.cpp
> @@ +449,5 @@
> > +    // container's block direction
> > +    if (type == nsGkAtoms::columnSetFrame) {
> > +      AvailableISize() = NS_UNCONSTRAINEDSIZE;
> > +    } else {
> > +      AvailableBSize() = NS_UNCONSTRAINEDSIZE;
> 
> Maybe we should change the computed value here too, if the style was "auto"?

So I think we can go a step further and *only* change the computed value, if the style is auto. Then the first hunk in the original patch is unnecessary, and the second also changes to test computed size, so your question there doesn't arise. Does that make sense?

(FTR I experimented with making the corresponding change to block size for non-columns, but that causes a lot of regressions)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8624491 [details] [diff] [review]
Patch v.2

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

Yes, this makes a lot of sense!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6603b3fcea4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6603b3fcea4