New issue
Advanced search Search tips

Issue 857185 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Tables with colspan and %widths compute min-content size > max-content

Project Member Reported by cbiesin...@chromium.org, Jun 27 2018

Issue description

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_table.cc?q=layout_table&sq=package:chromium&g=0&l=1009

Caption size gets included in the min-content size but not max-content. That means min-content can be larger than max-content.

Testcase:
Add an "html { width: fit-content; }" style to:
external/wpt/css/css-flexbox/flexbox_stf-table-caption.html
And run with --enable-blink-features=LayoutNG

That will trigger a DCHECK.
 
Note, on Mac that test fails even without the CSS change because there we always compute min/max content sizes for the document
Owner: dgro...@chromium.org
Status: Assigned (was: Untriaged)
note to self: include in max-content and see if tests break. also just check css-tables-3 max-width section that fantasai linked to the other day
Cc: mstensho@chromium.org
Adding Morten since he had some comments on this in slack (tldr: add the caption's min-content to the table's max-content, not the caption's max-content)
Even CSS2 defines this. See https://www.w3.org/TR/CSS22/tables.html#auto-table-layout
Status: Started (was: Assigned)
From what I can tell https://www.w3.org/TR/CSS22/tables.html#auto-table-layout doesn't discuss a table's min-content and max-content calculation.

https://drafts.csswg.org/css-tables-3/#layout-principles says:
The max-content width of a table is the width required to fit all of its columns max-content widths and its undistributable spaces.

I suspect the caption omission is related to the ongoing discussion around "table box", "table wrapper box", and "table grid box" (https://github.com/w3c/csswg-drafts/issues/471, https://github.com/w3c/csswg-drafts/issues/2501)

In any case, updating the table's max preferred width with caption's min preferred width in our table implementation doesn't regress any tests, so maybe that's just what we do.

Though I'm not yet sure why we shouldn't make the table's max preferred width include the captions' _max_ preferred width. Maybe because we don't want to make a table wider just to prevent a caption from wrapping. We already have interop issues with Firefox here.
Oh, maybe CSS2 implies to not use captions' max preferred width
in table's max preferred width just via omission. Bottom of https://www.w3.org/TR/CSS21/tables.html#auto-table-layout :

If the 'table' or 'inline-table' element has 'width: auto', the used width is the greater of the table's containing block width, CAPMIN, and MIN. However, if either CAPMIN or the maximum width required by the columns plus cell spacing or borders (MAX) is less than that of the containing block, use max(MAX, CAPMIN).


Indeed, a few layout tests fail when incorporating CAPMAX. fast/table/colgroup-preceded-by-caption.html renders the same on Edge, FF, and current Blink, wrapping a caption whose max-content is wider than the table.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dff5931455e0302289a3cc9027fd3f98cfe22fca

commit dff5931455e0302289a3cc9027fd3f98cfe22fca
Author: David Grogan <dgrogan@chromium.org>
Date: Thu Jul 12 16:34:30 2018

[css-tables] Make table's max-content honor captions' min-content

Table's min-content already honored it.

We'd apparently never been bitten before by tables occasionally having
min-content > max-content. A new DCHECK in LayoutNG exposed it.

Bug: 857185
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I49009a4988fbf38c1bf745a4f028e4e8b050804b
Reviewed-on: https://chromium-review.googlesource.com/1121251
Commit-Queue: David Grogan <dgrogan@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574595}
[modify] https://crrev.com/dff5931455e0302289a3cc9027fd3f98cfe22fca/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/dff5931455e0302289a3cc9027fd3f98cfe22fca/third_party/WebKit/LayoutTests/fast/table/caption-min-greater-than-max-crash.html
[add] https://crrev.com/dff5931455e0302289a3cc9027fd3f98cfe22fca/third_party/WebKit/LayoutTests/fast/table/spans-min-greater-than-max-crash.html
[modify] https://crrev.com/dff5931455e0302289a3cc9027fd3f98cfe22fca/third_party/blink/renderer/core/layout/layout_table.cc
[modify] https://crrev.com/dff5931455e0302289a3cc9027fd3f98cfe22fca/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc

Owner: ----
Status: Available (was: Started)
Summary: Tables with colspan and %widths compute min-content size > max-content (was: Tables can compute a min-content size that's larger than max-content)
See spans-min-greater-than-max-crash.html for an example
 Issue 891663  has been merged into this issue.

Sign in to add a comment