Bug 191462 - [css-grid] Percentage height on replaced item with scrollbar
Summary: [css-grid] Percentage height on replaced item with scrollbar
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2018-11-09 03:54 PST by Javier Fernandez
Modified: 2020-11-11 22:27 PST (History)
13 users (show)

See Also:

Test case to reproduce the issue (352 bytes, text/html)
2018-11-09 03:54 PST, Javier Fernandez
no flags Details
Patch (3.27 KB, patch)
2020-11-11 02:51 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2020-11-11 05:53 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2020-11-11 06:46 PST, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2018-11-09 03:54:00 PST
Created attachment 354325 [details]
Test case to reproduce the issue

If you have a replaced item that has scrollbar and a 100% height,
the height is wrongly computed as it doesn't get subtracted the scrollbar.

In the attached example you shouldn't see any overflow
(so you cannot actually scroll).
Comment 1 zsun 2020-11-11 02:51:14 PST
Created attachment 413803 [details]
Comment 2 Manuel Rego Casasnovas 2020-11-11 04:09:27 PST
Comment on attachment 413803 [details]

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

Thanks for the patch, it looks good, but you need to fix some stuff before landing.

> Source/WebCore/ChangeLog:9
> +        https://github.com/web-platform-tests/wpt/commit/538eedafb15733c6113b44998170a6bbdae7518b

It's good to refer to the original patch in the ChangeLog, but we also should explain and describe the change in this file. How the changes are fixing the issue.

Apart from that, as we're now passing the test css/css-grid/grid-items/percentage-size-replaced-subitems-001.html, we should remove the line for that test in TestExpectations. So the test is actually run by the bots.

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

This line should be moved above, just after the URL bug line with an empty line in the middle.

> Source/WebCore/rendering/RenderBox.cpp:3202
> +                    stretchedHeight = block->overridingLogicalHeight() - block->borderAndPaddingLogicalHeight() - block->scrollbarLogicalHeight();

I guess you can use block->overridingContentLogicalHeight() directly here.
Comment 3 zsun 2020-11-11 05:53:52 PST
Created attachment 413813 [details]
Comment 4 zsun 2020-11-11 06:46:00 PST
Created attachment 413817 [details]
Comment 5 Manuel Rego Casasnovas 2020-11-11 08:38:49 PST
Comment on attachment 413817 [details]

Comment 6 EWS 2020-11-11 22:26:32 PST
Committed r269717: <https://trac.webkit.org/changeset/269717>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413817 [details].
Comment 7 Radar WebKit Bug Importer 2020-11-11 22:27:16 PST