Closed Bug 1674302 Opened 1 year ago Closed 8 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox87 --- fixed

People

(Reporter: mats, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

WIP
10.49 KB, patch
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

Attached patch WIPSplinter Review

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.

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.

See Also: → 1456224

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.

Depends on: 1686603

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

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

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

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

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

Depends on D103438

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

Assignee: nobody → aethanyc
Status: NEW → ASSIGNED

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.)

Flags: needinfo?(mats)

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.

Flags: needinfo?(mats)

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.

Attachment #9199975 - Attachment is obsolete: true

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.

Attachment #9199977 - Attachment is obsolete: true

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.

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.

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.

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

Depends on D104643

Attachment #9199979 - Attachment description: Bug 1674302 Part 5 - Use StyleSizeOverrides to replace ComputeSizeFlag::UseAutoISize. → Bug 1674302 Part 2 - Use StyleSizeOverrides to replace ComputeSizeFlag::UseAutoISize.
Attachment #9199978 - Attachment description: Bug 1674302 Part 4 - Carry the StyleSizeOverrides from table wrapper frame to inner table frame. → Bug 1674302 Part 3 - Fix flex base size resolution and main/cross size override for table flex items.
Attachment #9199981 - Attachment description: Bug 1674302 Part 7 - Fix reftests and adjust test expectations. → Bug 1674302 Part 4 - Fix reftests and adjust test expectations.
Depends on: 1691875
Blocks: 799725
Attachment #9199980 - Attachment is obsolete: true
Attachment #9199976 - Attachment is obsolete: true
Pushed by aethanyc@gmail.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27673 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
Blocks: 1693409
Duplicate of this bug: 1692638
Blocks: 1694298
Duplicate of this bug: 1456224
Duplicate of this bug: 1454330
Blocks: 1692116
Regressions: 1698772
Regressions: 1701447
Regressed by: 1700879
No longer regressed by: 1700879
Regressions: 1700879
Regressions: 1705314
You need to log in before you can comment on or make changes to this bug.