Closed Bug 1173689 Opened 4 years ago Closed 4 years ago

Multi-columns in orthogonal writing-mode overflow their container

Categories

(Core :: Layout: Columns, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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.
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.
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.
Attachment #8620851 - Attachment description: multiColumn.html → Testcase with column-count
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?
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 4 was in reference to the original example)
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.
Attached patch Patch v.0 (obsolete) — Splinter Review
What do you think of this approach? Is it too much of a hack?
Assignee: nobody → smontagu
Attachment #8623079 - Flags: feedback?(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...
Attachment #8623079 - Flags: feedback?(jfkthame) → feedback+
Attachment #8623079 - Attachment is obsolete: true
Attachment #8623506 - Flags: review?(roc)
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 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"?
Attachment #8623506 - Flags: review?(roc) → review-
Attached patch Patch v.2Splinter Review
(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)
Attachment #8624491 - Flags: review?(roc)
Attachment #8623506 - Attachment is obsolete: true
Comment on attachment 8624491 [details] [diff] [review]
Patch v.2

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

Yes, this makes a lot of sense!
Attachment #8624491 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/d6603b3fcea4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: Layout: Block and Inline → Layout: Columns
You need to log in before you can comment on or make changes to this bug.