Bug 191390 - "min-content" & "max-content" keywords should behave as initial value in block axis (but WebKit improperly treats them as the content-size)
Summary: "min-content" & "max-content" keywords should behave as initial value in bloc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 12
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks:
 
Reported: 2018-11-07 11:53 PST by Daniel Holbert
Modified: 2021-02-20 14:42 PST (History)
13 users (show)

See Also:


Attachments
testcase 1 (591 bytes, text/html)
2018-11-07 11:53 PST, Daniel Holbert
no flags Details
Patch (3.89 KB, patch)
2021-02-19 01:37 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.83 KB, patch)
2021-02-19 06:18 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2021-02-20 01:38 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Holbert 2018-11-07 11:53:27 PST
Created attachment 354122 [details]
testcase 1

I'm testing using Safari 12 on Mojave.


What steps will reproduce the problem?
(1) Load attached testcase, or visit https://jsfiddle.net/s9bw6j4c/

What is the expected result?
 No red should be visible. (i.e. the black-bordered areas should all be 0-height)

What happens instead?
In the first two chunks, there is a large rectangle visible (i.e. the black bordered area expands *beyond* its specified "height:0px" to encompass its child's height)


WebKit is improperly honoring "min-height: min-content"/"max-content" here -- if you remove that line, the rendering switches to the correct rendering, as shown in the third chunk of this testcase.

The css-sizing-3 spec says that min-content and max-content only resolve to the content size in the inline axis, and "otherwise behaves as the property’s initial value".  So in this case, it should behave as if it weren't specified, but that's not what happens in WebKit.

Spec link:
https://drafts.csswg.org/css-sizing-3/#valdef-width-min-content

Firefox 64 beta & 65 Nightly (which support these prefixed keywords in the block dimension) gives the expected behavior.

(Edge and earlier Firefox versions give the expected behavior, too, but only because they don't support these keywords -- not in the block dimension at least, for older Firefox versions.)

Note: I ran into a version of this causing a compatibility issue on Facebook.com -- see https://github.com/webcompat/web-bugs/issues/21006 for details. (That case has to do with the "min-height:auto" special case on flex items with overflow:hidden, where it resolves to 0.   Facebook is naiively trying to opt out of that special case by specifying "min-height:max-content", but per spec, that should behave like the initial value and so should still resolve to 0. But it doesn't in WebKit.)

Chrome has the same bug -- I filed https://bugs.chromium.org/p/chromium/issues/detail?id=902863 on this for Chrome.
Comment 1 Michael Puckett 2020-03-02 15:40:00 PST
This is still present in Safari 13.

The Chrome issue has been fixed.

This is a cross-browser inconsistency for me during development. Animating max-height from max-content to 0 greatly simplifies transitions, but this is not possible in Safari.
Comment 2 Rob Buis 2021-02-19 01:37:15 PST
Created attachment 420939 [details]
Patch
Comment 3 Rob Buis 2021-02-19 06:18:00 PST
Created attachment 420960 [details]
Patch
Comment 4 Darin Adler 2021-02-19 12:03:51 PST
Comment on attachment 420960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420960&action=review

> LayoutTests/ChangeLog:11
> +        Remove test that makes now incorrect assumptions.
> +
> +        * fast/css-intrinsic-dimensions/height-expected.html: Removed.
> +        * fast/css-intrinsic-dimensions/height.html: Removed.

It’s great to remove tests based on incorrect assumptions. This doesn’t explain why it’s OK to have reduced test *coverage*, though. Presumably the reason is either that WPT has thorough testing of this that was already redundant with this test, or that we have plenty of coverage elsewhere (but then I’d expect us to be updating expected results for the progression).
Comment 5 Rob Buis 2021-02-20 01:38:22 PST
Created attachment 421090 [details]
Patch
Comment 6 EWS 2021-02-20 14:40:59 PST
Committed r273206: <https://commits.webkit.org/r273206>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421090 [details].
Comment 7 Radar WebKit Bug Importer 2021-02-20 14:42:15 PST
<rdar://problem/74559442>