Bug 214370

Summary: [css-grid] Vertical alignment is wrong with box-sizing: border-box
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, clopez, darin, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, obrufau, pdr, rego, simon.fraser, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
URL: https://codepen.io/argyleink/pen/pogZxaZ
See Also: https://github.com/web-platform-tests/wpt/pull/24748
Attachments:
Description Flags
Testcase to reproduce the issue
none
testcase with both box-sizing values
none
Patch
none
Patch none

Description Simon Fraser (smfr) 2020-07-15 12:15:57 PDT
https://codepen.io/argyleink/pen/pogZxaZ

Looks different in WebKit vs other browsers.
Comment 1 Simon Fraser (smfr) 2020-07-15 12:16:34 PDT
Maybe a grid issue?
Comment 2 Manuel Rego Casasnovas 2020-07-15 13:29:16 PDT
Created attachment 404385 [details]
Testcase to reproduce the issue
Comment 3 Manuel Rego Casasnovas 2020-07-15 13:30:06 PDT
(In reply to Simon Fraser (smfr) from comment #1)
> Maybe a grid issue?

Yes, it looks like a bug related to vertical alignment and box-sizing: border-box.

Please Oriol, could you take a look?
Comment 4 Radar WebKit Bug Importer 2020-07-22 12:16:12 PDT
<rdar://problem/65948925>
Comment 5 Oriol Brufau 2020-07-24 09:34:13 PDT
Created attachment 405157 [details]
testcase with both box-sizing values

Actually it also happens with 'box-sizing: content-box'. See new testcase.

The problem seems that RenderBox::availableLogicalHeight calls constrainLogicalHeightByMinMax instead of constrainContentBoxLogicalHeightByMinMax.
Comment 6 Oriol Brufau 2020-07-24 16:37:47 PDT
Created attachment 405201 [details]
Patch
Comment 7 EWS Watchlist 2020-07-24 16:38:44 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 8 Oriol Brufau 2020-07-24 17:25:36 PDT
It seems Chromium did the same change in https://chromium.googlesource.com/chromium/src/+/acc04ddbdc31a7c5bda78f88719df43367630fed%5E%21/#F2
Comment 9 Manuel Rego Casasnovas 2020-07-27 01:22:30 PDT
Comment on attachment 405201 [details]
Patch

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

It looks like we could have a test without grid too (checking the Chromium change).

BTW, is this a new test? If that's the case you need to export it to WPT and land it there, before merging it in WebKit.

> Source/WebCore/ChangeLog:9
> +        the available grid space for in the block axis. But there was a bug when

Nit: You might want to reword this sentence "the available grid space for in the block axis" sounds weird to me.
Comment 10 Oriol Brufau 2020-07-27 03:40:30 PDT
(In reply to Manuel Rego Casasnovas from comment #9)
> BTW, is this a new test? If that's the case you need to export it to WPT and
> land it there, before merging it in WebKit.

Yes, a new test, this is the export: https://github.com/web-platform-tests/wpt/pull/24748
Comment 11 Oriol Brufau 2020-07-28 14:09:19 PDT
Created attachment 405412 [details]
Patch
Comment 12 Oriol Brufau 2020-07-28 14:11:18 PDT
Comment on attachment 405201 [details]
Patch

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

I have imported the Chromium test.

>> Source/WebCore/ChangeLog:9
>> +        the available grid space for in the block axis. But there was a bug when
> 
> Nit: You might want to reword this sentence "the available grid space for in the block axis" sounds weird to me.

Done.
Comment 13 EWS 2020-07-28 16:41:10 PDT
Committed r265020: <https://trac.webkit.org/changeset/265020>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405412 [details].