Fix shrink-to-fit when children's writing-mode is orthogonal

Fix shrink-to-fit when children's writing-mode is orthogonal

This patch fixes the shrink-to-fit algorithm when descendants of the
element have orthogonal writing-mode.

Strictly speaking, as the [spec] defines, min-content width of the element
must be determined by logicalHeight of children. This would require either
"layout during preferred width calculation" or "multiple layout passes."

In most cases, however, descendants are not mixed. Since orthogonal
descendants need re-layout only when the parent's logicalHeight changes,
this patch updates logicalWidth after children are laid out but does not
re-layout descendants.

Table cells with orthogonal descendants require additional work and the
fix is not included in this CL. 001q-001x are tests for table cells.

css3/flexbox/flex-flow-margins-auto-size.html that currently expects to
fail passes with this patch: http://wkb.ug/70769

[spec]: http://dev.w3.org/csswg/css-writing-modes-3/#orthogonal-flows

BUG=410320, 359604
TEST=css3/flexbox/flex-flow-margins-auto-size.html
TEST=imported/csswg-test/css-writing-modes-3/orthogonal-parent-shrink-to-fit-001.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201370

[kojii, 2015-05-03 17:28:47]

kojii@chromium.org changed reviewers:
+ eae@chromium.org

[kojii, 2015-05-03 17:28:48]

PTAL!

[eae, 2015-05-04 00:06:59]

https://codereview.chromium.org/1121173002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1121173002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:803:
const_cast<LayoutObject*>(this)->layoutIfNeeded();
I'd rather try to avoid calling layout during preferred with calculation. Any
way it it could be moved to one of the layout* methods?

[kojii, 2015-05-04 06:11:12]

On 2015/05/04 00:06:59, eae wrote:

Hm, right. I suppose you understand this is unusual anyway, in genenral we
determine width based on min/max content (preferred width) to determine the
height. In this specific case we need child's height to determine preferred
width.

The other way is to go without content height as a tentative value at this
point, then when child's height was determined, it invalidates parent's width if
orthogonal. That can avoid child's layout*(), but it might require to calculate
parent's width twice, and each caller must be aware that preferred width may
change later in the lifecycle.

I'll see if it's possible and simplifies the patch.

[kojii, 2015-05-04 20:42:25]

Not for landing but could you PTAL? I think this patch gives the idea of another
approach.

The hard part of this approach is that it needs to run parts of layout twice.
It's not only slow, but it isn't easy to make sure all states are reset in the
2nd loop. I actually haven't really make it sure in this patch yet.

While I understand your concern, I mildly prefer the original approach. Here's
why.

IIUC, layout must be avoided during preferred width calculation because children
layout could change if parent's preferred width changes, and thus it can cause
multiple layouts. In normal flow, layout goes down to descendants by calculating
preferred width, determines leaf's height, and then the height goes up to
ancestors. So calling layout breaks the dependency.

What makes orthogonal flows tricky is that, because parent's width is child's
height, preferred width change does not affect child layout at all. And to
calculate parent's min-content, we need child's height, so the dependency is
reversed. I think in this specific case, calling layout for child does not harm.

The original approach not only smaller and faster, it looks to cause less bugs,
no?

[kojii, 2015-05-04 20:54:26]

Oh, the diff is too hard to read due to changed indentations. I switched to
"goto" just to make it easier to read.

[kojii, 2015-05-13 15:56:08]

kojii@chromium.org changed reviewers:
+ cbiesinger@chromium.org, esprehn@chromium.org, ojan@chromium.org

[kojii, 2015-05-13 15:56:10]

+ojan, esprehn, cbiesinger

PS3 layouts children first in LayoutBlock::minMaxPreferredLogicalWidthOfChild
(LayoutBlock.cpp).

PS7 is another approach; do layout with a tentative preferred width
(LayoutBlock.cpp), but re-run part of the layout if child's preferred width was
tentative after children were layout (LayoutBlockFlow.cpp).

[esprehn, 2015-05-21 01:52:54]

https://codereview.chromium.org/1121173002/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1121173002/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:445: goto
recalcLogicalWidthAfterLayoutChildren;
I think you can just return false here instead, the code above you is a loop.

[kojii, 2015-05-23 00:22:47]

PTAL. Returning false works good for LayoutBlockFlow. For table, I added a loop
counter to prevent infinite loop.

Does this look good direction?

https://codereview.chromium.org/1121173002/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1121173002/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.cpp:445: goto
recalcLogicalWidthAfterLayoutChildren;
On 2015/05/21 01:52:54, esprehn wrote:
> I think you can just return false here instead, the code above you is a loop.

Done.

[ojan, 2015-05-23 00:56:22]

This doesn't work if you have a shrink-wrapped container around your test cases
(e.g. make class="test" set width:-webkit-fit-content) because that container
will layout using the incorrect preferred widths of the children.

It's true that the current behavior is undesirable, but only in the case of
shrink-wrapped orthogonal writing modes. Purely horizontal, purely vertical or
definite size orthogonal writing modes all work fine today. Is there real
content you've encountered that wants to do this or is it just test cases?

[kojii, 2015-05-25 03:55:28]

On 2015/05/23 00:56:22, ojan wrote:
> This doesn't work if you have a shrink-wrapped container around your test
cases
> (e.g. make class="test" set width:-webkit-fit-content) because that container
> will layout using the incorrect preferred widths of the children.

You're right, the flag needs to be propagated to ancestors. Fixed and tests
added.

> It's true that the current behavior is undesirable, but only in the case of
> shrink-wrapped orthogonal writing modes. Purely horizontal, purely vertical or
> definite size orthogonal writing modes all work fine today. Is there real
> content you've encountered that wants to do this or is it just test cases?

Two bug reports were filed so far:
1. A real site from http://crbug.com/410320
   http://iagenweb.org/calhoun/records/census/ia/1885/butler/3.php
2. A minimized sample from an EPUB content in http://crbug.com/359604

Another example is that Excel can do this per cell, so exported tables from
Excel can contain this pattern.

Definitely there are real contents and needs. It's not often seen, rather rarer,
but since there is not a good workaround, I can't find ways to go other than
this. I wish to come up with any wild ideas that authors could work around, but
without luck.

[kojii, 2015-06-03 18:43:53]

PTAL.

I did:
* Cleanup of comments and removed #ifdef for testing.
* Made LayoutTable::layout() similar to what LayoutBlockFlow::layoutBlock()
does.
* Removed the test, since it's upstreamed to CSS WG, and we started to import
them. All tests I had, with nested cases, are here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
More nested cases (such as table cell having a div that has an orthogonal child)
are added to CSS WG, soon to be imported too.
* Filed a bug for Gecko. No actions yet.
https://bugzilla.mozilla.org/show_bug.cgi?id=1166120

Does this CL still concern you? Do we want to wait until Gecko decides what
they'd like to do?

[ojan, 2015-06-10 02:56:22]

Sorry, I've been trying to make the time to do this review and I really just
haven't been able to get to it. I still have concerns that this patch adds
complexity that isn't worth it. Doing layout in loops is bad and we should try
to do it less. Also, it looks to me like you're relying on the loop in
LayoutBlockFlow::layoutBlock, but that won't work for flex and grid since they
don't loop their layout at the moment. If you add that, then this patch will
have more obvious added complexity.

[kojii, 2015-06-10 18:14:13]

On 2015/06/10 at 02:56:22, ojan wrote:
> Sorry, I've been trying to make the time to do this review and I really just
haven't been able to get to it. I still have concerns that this patch adds
complexity that isn't worth it. Doing layout in loops is bad and we should try
to do it less.

Thank you Ojan for taking time on this. I understand you're super busy person
and this issue must be very trivial from your point of view, appreciate your
attention on this.

I've been thinking how I could simplify this further, but one thing I wonder is,
since the complexity is always compared with the value as you said, I think what
really we do not agree with may be the value rather than the complexity. Allow
me to explain my view on the value.

I agree that this is not a feature that will be used very widely. But whenever
non-CJK authors want to use vertical flow, the most use cases I've seen is a
small chunk of text, usually only up to single sentence or so in a few lines.
That kind of small text usually is layout with shrink-to-fit.

I think you remember the table cell example, that's one. I saw some toolbars UI
docked to left or right and set in vertical flow, this requires shrink-to-fit
too. Sometimes in magazines-like layout, you should see headings are layout in
vertical flow, float to left or right. This is also shrink-to-fit. Actually,
it's hard to find a case where non-CJK users would use vertical flow without
shrink-to-fit.

Well, if you say "vertical flow for non-CJK users" itself isn't worth to care,
that's hard to object though ;(, but I hope you at least agree that the target
users include non-CJK and the base number isn't ignorable although the rate of
use is low.

IE has implemented this and passed the tests, and I believe Gecko is willing to
do so too. I can try to sync with dbaron and Jonathan (the Gecko dev working on
writing-mode) if that helps you to reconsider the value.

> Also, it looks to me like you're relying on the loop in
LayoutBlockFlow::layoutBlock, but that won't work for flex and grid since they
don't loop their layout at the moment. If you add that, then this patch will
have more obvious added complexity.

The flexbox test (css3/flexbox/flex-flow-margins-auto-size-expected.txt in this
diff) turns green with this patch, so I don't think we need more changes. I
don't think we have tests for Grid, nor I follow Grid well enough to know it
supports shrink-to-fit with mixed flows. I checked CSS WG test suites but they
don't seem to have one either. I know little-to-zero on Grid, but I can study to
write a test if that helps to mitigate your concerns.

[ojan, 2015-06-10 18:33:32]

You hit the nail on the head. My inclination is that vertical flow for non-CJK
content isn't worth adding this level of complexity for.

Since IE has implemented this, maybe we should reach out to them and see what
solution they have? Maybe there's some clever solution that we haven't
considered or maybe they believe we just need to live with the added complexity.

To give a bit more context, the repeated layout loops are a real problem for the
long-term health of the code. The patch you have is probably fine because it's
guarded by dirty bits, but it opens the door for future patches accidentally
depending on the loop. The RenderBlockFlow loop that this patch depends on was
added very reluctantly and only because we had backcompat issues that we didn't
have a choice in. I'd be more comfortable with an explicit two passes, but I'm
not sure if that could be done correctly. Even then, I wouldn't love the amount
of complexity we're adding for such a niche use-case.

[kojii, 2015-06-10 18:46:04]

On 2015/06/10 at 18:33:32, ojan wrote:
> You hit the nail on the head. My inclination is that vertical flow for non-CJK
content isn't worth adding this level of complexity for.

Thank you for your prompt reply again. Yeah, that was what I was afraid to hear,
but also wondering it might be so.

> Since IE has implemented this, maybe we should reach out to them and see what
solution they have? Maybe there's some clever solution that we haven't
considered or maybe they believe we just need to live with the added complexity.

Thank you for giving an action to make the progress. I'll try to reach them and
get back to you.

> To give a bit more context, the repeated layout loops are a real problem for
the long-term health of the code. The patch you have is probably fine because
it's guarded by dirty bits, but it opens the door for future patches
accidentally depending on the loop. The RenderBlockFlow loop that this patch
depends on was added very reluctantly and only because we had backcompat issues
that we didn't have a choice in. I'd be more comfortable with an explicit two
passes, but I'm not sure if that could be done correctly. Even then, I wouldn't
love the amount of complexity we're adding for such a niche use-case.

I'm quite new and know little about problems you had before, I am never taking
your words lightly at all. Tried hard to think other ways to solve what's needed
for the scenario, but still without luck.

Note that you might have noticed LayoutTable.cpp has an explicit limit to two
passes. LayoutBlockFlow is harder as you said because there are other reasons to
loop, but I can try that if that mitigates the risks.

But before that, I'll see what IE folks could give us. Thank you again.

[kojii, 2015-07-14 17:43:26]

ojan@, PTAL. This gets much simpler, no longer require either "layout in
preferred width" nor "multi-pass layout", so I hope your concerns are clear now.

I could not get response from IE, but smontagu, an engineer at Mozilla came up
with a good compromise to assume that, when writing-mode is orthogonal and
shrink-to-fit, there's only one child. This assumption matches to use cases for
floats and table cells and makes sense to me.

With this assumption, the child can layout without parent's available width (it
needs parent's available height, which is ICB if indefinite) and can calculate
its height. When parent's width is zero, the height simply overflows. After
that, we can change parent's logical width to match to it, but since parent's
width does not affect the layout of the child, we don't have to re-layout the
child.

There is a request for multiple children in inline-block case
<http://crbug.com/359604>, but that request can be worked out with different
solution, I can talk to the reporter. I think we can leave it for now and see
how much real world authors would care.

Note that I removed table cell support from this CL (and thus 8 tests still
fail.) Simon did similar fix to Gecko, but I haven't succeeded to change the
column width yet, so I'd like to split that to a separate CL and ask your
opinion on this one.

Your review is greatly appreciated in advance.

[kojii, 2015-08-19 15:10:28]

eae@, PTAL.

[eae, 2015-08-27 18:24:46]

Much cleaner, thank you for taking all the feedback into consideration and
working with Mozilla to simplify the handling of this.

The current patch strikes a much better balance between complexity and
functionality and is explicit in a way that previous attempts where not.

LGTM

[kojii, 2015-08-28 01:06:20]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-08-28 01:06:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121173002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1121173002/400001

[commit-bot: I haz the power, 2015-08-28 02:11:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-28 02:11:13]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kojii, 2015-08-28 03:38:25]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-08-28 03:38:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121173002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1121173002/400001

[commit-bot: I haz the power, 2015-08-28 04:53:12]

Committed patchset #21 (id:400001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201370