1674302 - [css-flexbox] Wrong Size/MinSize/MaxSize style data is used for table flex items

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect, P3)

Description

[Mats Palmgren (inactive)]

For example:
https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/layout/generic/nsFlexContainerFrame.cpp#2160-2170
Size/MinSize/MaxSize is always auto on the table-wrapper (these properties are not inherited and shouldn't be), so when checking "is this style auto or not?" we should use the StylePosition() on the (inner) table frame instead.

Comment 1

[Mats Palmgren (inactive)]

Attachment: WIP

Fwiw, here's a hack I did to see if it would solve some of the remaining errors in testing/web-platform/tests/css/css-flexbox/table-item-flex-percentage-width.html, but to my surprise it didn't. I think we need to do something like this though, but there are more changes needed too to fix that test.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I investigate this a bit and found two more things we need to fix.

1. In nsIFrame::ComputeSize, if this is an nsTableFrame, IsFlexItem() doesn't treat it as a flex item, and we might want to do something like isGridItem like https://searchfox.org/mozilla-central/rev/919f7400e2e12bc963ee2ae8400ef961972c3059/layout/generic/nsIFrame.cpp#6118-6119,6126-6128 for the purpose of size computation.

2. When flex container overrides flex item's computed main size during the flex algorithm or at the end of item's final reflow , it uses the special AutoFlexItemMainSizeOverride class or calls ReflowInput::{SetComputedISize,SetComputedBSize} after establishing ReflowInput. It's ok except for <table> flex items because overriding the size nsTableWrapperFrame doesn't affect nsTableFrame.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

With bug 1686603, I think its easier to solve the issue 2 in comment 2, because we'll cache the desired flex item's main size and cross size when creating table wrapper's ReflowInput. At the time when we create a ReflowInput for the inner table frame, we'll know table wrapper's computed isize/bsize are been overridden, then we can do some the necessary computation to subtract table caption, border padding, etc, and pass it to table frame.

I have a WIP that seems to fix this bug, but I haven't sort out the the details of subtracting table caption, border padding yet.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 1 - Add FLEX_LOG to print main/cross size overrides.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 2 - Use correct nsStylePosition to access style sizes for table frames.

The style sizes like width, height, min-width, min-height,
max-width, and max-height are not inherited from inner table frame
to the table wrapper frame, so we need to use inner table's style for
these style size properties when calculating a table frame's various
sizes.

Note we don't really need to access inner table's box-sizing because
even if we do, table wrapper frame's border and padding are all zero, it
doesn't affect the size overrides provided by flex container. However,
in Part 4 we'll need to adjust the size overrides when propagating the
size override to inner table frame.

This is a preparation patch, but it can potentially break existing
behavior, but combining this patch with later parts won't.

Depends on D103434

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 3 - Expand nsTableWrapperFrame::InitChildReflowInput() at its only caller.

The purpose of InitChildReflowInput() is to provide customized border,
padding, and containing block size for the inner table frame, not for
the caption frame. So the caption frame's ReflowInput doesn't need to be
initialized in a separate step.

This patch doesn't change the behavior, and is also a preparation for
Part 4 where we want to carry the style size override from table
wrapper's ReflowInput when constructing inner table frame's ReflowInput.

Depends on D103435

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 3 - Fix flex base size resolution and main/cross size override for table flex items.

The size override adjustment in
nsTableWrapperFrame::OuterBeginReflowChild() prevents us from breaking
layout/reftests/flexbox/flexbox-table-flex-items-2.html.

This patch didn't subtract the area occupied by table caption from the
size overrides. We can probably fix this in bug 799725 when implementing
stretching for table flex items.

Depends on D103436

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 2 - Use StyleSizeOverrides to replace ComputeSizeFlag::UseAutoISize.

This patch does change the behavior. UseAutoISize flag is buggy when
used on table flex items because it never propagates to inner table
frame. Luckily, this can be fixed by replacing the flag with
StyleSizeOverrides emplacing an 'auto' mStyleISize.

Depends on D103437

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 6 - Don't let table flex item's shrink below its min-content inline-size.

Similar logic is applied in nsTableFrame::ComputeSize() and
nsLayoutUtils::IntrinsicForAxis().

Depends on D103438

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 4 - Fix reftests and adjust test expectations.

With this patch series, firefox now renders
flexbox-table-flex-items-1.html the same as Google Chrome. Hence the
modification to flexbox-table-flex-items-1-ref.html.

Depends on D103439

Comment 11

[Daniel Holbert [:dholbert]]

As discussed in the patch stack (particularly https://phabricator.services.mozilla.com/D103435 ), I'm uneasy about just directly using the table's min-width and min-height as if they were the min-width and min-height of the table-wrapper, because it means we don't end up setting aside space for the caption, which can then mean we have needlessly-overlapping content.

I suspect in comment 0, mats was just suggesting that we defer to the table-inner-frame for the purpose of answering certain targeted questions like "is this property auto" in some specific cases where it makes sense to do so; but not that we should be directly using the inner frame's exact specified property min-size/max-size values for sizing the outer frame (as Part 2 of the patch-stack here kinda does).

Am I understanding that correctly, mats?

(I'm also not entirely sure which specific targeted questions it makes sense to defer to the table-inner-frame, offhand -- the link in comment 0 is about min-width:auto / min-height:auto. I actually think it feels right for that to always be auto on the table-wrapper frame, even in cases where the author has specified some min-width/min-height value on the table, since tables don't support being smaller than their content-size anyway; and if the specified value is larger, then we'll still pick that up & honor it when we measure content to resolve the auto size on the table wrapper.)

Comment 12

[Mats Palmgren (inactive)]

Am I understanding that correctly, mats?

Yup, that was my intent when filing this bug. However, if there's a better way to solve the problem I'm all for it.

Comment 13

[Phabricator Automation]

Comment on attachment 9199975 [details]
Bug 1674302 Part 1 - Add FLEX_LOG to print main/cross size overrides.

Revision D103434 was moved to bug 1690713. Setting attachment 9199975 [details] to obsolete.

Comment 14

[Phabricator Automation]

Comment on attachment 9199977 [details]
Bug 1674302 Part 3 - Expand nsTableWrapperFrame::InitChildReflowInput() at its only caller.

Revision D103436 was moved to bug 1690715. Setting attachment 9199977 [details] to obsolete.

Comment 15

[Daniel Holbert [:dholbert]]

Skimming over the WPT tests that were fixed (based on .ini-file-removals) in the original patch stack here:

1. https://wpt.fyi/results/css/css-flexbox/table-as-item-narrow-content.html
2. https://wpt.fyi/results/css/css-flexbox/table-as-item-specified-width.html
3. https://wpt.fyi/results/css/css-flexbox/table-item-flex-percentage-width.html
4. https://wpt.fyi/results/css/css-flexbox/table-with-percent-intrinsic-width.html

I think all of the tests require that we at least fix one particular thing: we need to make sure the table-inner will respect the width that the table-wrapper has available for it, during the final flex-item reflow. So e.g. if the flex algorithm decides that the table-wrapper is 400px wide and gives the table-wrapper frame that width, then we should behave as if the table had width:calc(400px-captionWidth) (if the caption is on the side).

I think this sort of change would make us pass the first test (table-as-item-narrow-content.html). From looking at a frame dump, it seems we actually get the correct size for the table-wrapper frame (it ends up 100px wide as-expected, because the flex properties inherit through). But, the inner table frame is still too small -- it shrinkwraps its own contents (per its width:auto) rather than filling the table-wrapper frame, so the flex container's final imposed size doesn't end up making a visible difference here, and the test fails.

For the second test, there's one additional thing we need to fix -- I think the table-wrapper's implicit min-width:auto is resolving to be something that's unexpectedly-large, because the table-wrapper naturally asks its child (the table inner frame) for its min-content contribution, and the child returns its specified width, which is 500px. Really, we want that specified width to be informing the flex-basis:auto, not establishing the content-based minimum size. We definitely need some sort of targeted hack to prevent this from happening; and maybe that's what the original Part 5 patch is aiming to fix?

For the third and fourth tests, I haven't looked as closely, so I won't comment on them at this point.

Comment 16

[Daniel Holbert [:dholbert]]

Also: before investing too much time in accounting for inline-axis contributions of captions, we might want to first unsupport the inline-axis caption-side values, as you've suggested we do in bug 1688695. I imagine that'll make the arithmetic easier here in that axis, and it'd make it less-error-prone to directly share inline-axis sizing data back and forth between the table wrapper & inner table.

If we end up with kinda-gross special-case code to subtract out the caption (like my width:calc(400px-captionWidth) expression in previous comment), it'll be nice for that special-case code to only have to consider the block axis.

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 11:

I actually think it feels right for that to always be auto on the table-wrapper frame, even in cases where the author has specified some min-width/min-height value on the table, since tables don't support being smaller than their content-size anyway; and if the specified value is larger, then we'll still pick that up & honor it when we measure content to resolve the auto size on the table wrapper.

I feel this make sense to me. We probably shouldn't look into inner table's property except when absolutely necessary. I'll explore your idea.

Re comment 15:

Thank you for taking a look into these testcases, and giving the outline of the possible fix. You're right that original Part 5 (remove ComputeSizeFlag::UseAutoISize) is trying to prevent inner <table>'s specified size from influencing the content-based minimum size.

Re comment 16:

Yeah, its painful to fix this bug if we want to consider all the non-standard caption-side value, but bug 1688695 probably won't be fix soon. I think we should at least make the standard caption-side correct without worrying too much about breaking those non-standard ones.

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1674302 Part 1 - Change a FLEX_LOG to dump address of flex item's frame rather than flex item itself.

All the other FLEX_LOGs dump the address of flex item's frame.

Depends on D104643

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db348f5c471b
Part 1 - Change a FLEX_LOG to dump address of flex item's frame rather than flex item itself. r=emilio
https://hg.mozilla.org/integration/autoland/rev/23aff0f21a1d
Part 2 - Use StyleSizeOverrides to replace ComputeSizeFlag::UseAutoISize. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a6a313238f3a
Part 3 - Fix flex base size resolution and main/cross size override for table flex items. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d1b7430e5ebb
Part 4 - Fix reftests and adjust test expectations. r=dholbert

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27673 for changes under testing/web-platform/tests

Comment 21

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/db348f5c471b
https://hg.mozilla.org/mozilla-central/rev/23aff0f21a1d
https://hg.mozilla.org/mozilla-central/rev/a6a313238f3a
https://hg.mozilla.org/mozilla-central/rev/d1b7430e5ebb

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot