Multi-columns in orthogonal writing-mode overflow their container

RESOLVED FIXED in Firefox 41

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8620851 [details]
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.
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.
(Assignee)

Comment 2

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

Updated

3 years ago
Attachment #8620851 - Attachment description: multiColumn.html → Testcase with column-count
(Assignee)

Comment 3

3 years ago
Created attachment 8620896 [details]
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?
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)
(Assignee)

Comment 6

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

Comment 7

3 years ago
Created attachment 8623079 [details] [diff] [review]
Patch v.0

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

Comment 9

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e7c69fe8a48
(Assignee)

Comment 10

3 years ago
Created attachment 8623506 [details] [diff] [review]
Patch v.1 with added and improved comments
Attachment #8623079 - Attachment is obsolete: true
Attachment #8623506 - Flags: review?(roc)
(Assignee)

Comment 11

3 years ago
Created attachment 8623532 [details]
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 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-
(Assignee)

Comment 13

3 years ago
Created attachment 8624491 [details] [diff] [review]
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)
Attachment #8624491 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6603b3fcea4
https://hg.mozilla.org/mozilla-central/rev/d6603b3fcea4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.