[css-grid] Resolution of percentage padding/margin is inconsistent for orthogonal writing-mode |
|||||
Issue descriptionChrome Version: 65.0.3311.3 (Official Build) dev (64-bit) OS: Linux What steps will reproduce the problem? (1) load the attached testcase (2) (3) What is the expected result? grids: grid-template-rows:18px; height:18px; child padding-bottom:5px grid-template-rows:38px; height:38px; child padding-bottom:5px blocks: height:18px; child padding-bottom:5px height:38px; child padding-bottom:5px What happens instead? grids: grid-template-rows:18px; height:18px; child padding-bottom:5px grid-template-rows:48px; height:48px; child padding-bottom:5px blocks: height:18px; child padding-bottom:5px height:38px; child padding-bottom:5px ============ I'm removing back-computing of percentages from Gecko and I get the expected result shown above. The percentage basis for the 10% bottom padding is now (after the recent spec change) the CB inline-size, which in this case is the 50px column size. This is a definite size so I tend to think it should be used as a percentage basis when calculating the min/max-content size for the item when determining the (auto) row size. It works when the item has the same writing-mode (grid-template-rows:18px), and also when sizing a float with an orthogonal child, so the exception seems to be orthogonal writing-mode grid items. This seems inconsistent to me. I don't really understand how you got a row size of 48px either. It seems to indicate that you resolved 10% against a 150px measure at some point, resulting in a 15px bottom padding. I suspect that you used the container's inline-size, which is 150px (due to grid-template-columns: 50px 100px), but that's the wrong percentage basis for grid items, which should use the size of their grid area.
,
Feb 5 2018
Thanks for the bug report. There are some clear issues with percentage paddings/margins and orthogonal items: http://jsbin.com/kegiyov/1/edit?html,css,output Also in the attached example, if you later modify from the inspector the grid-template-columns, the issue seems to be gone, so we have some differences between layouts there.
,
Mar 15 2018
I've been investigating this and I've realized that the bug is not related to grid, it's a generic bug with vertical writing-modes and percentage margins. The problem is that the margins are not always resolved against the width and we get weird results. Check the attached example. There's a wrapper element with a fixed height of 100px. Inside it there's an element with a fixed width of 500px. That element has a vertical element inside (orthogonal) with some content and a margin-top of 10%. The margin-top should be resolved against the width of the parent, so 10% of 500px is 50px. The margin-top is computed correctly. However, the problem comes when computing the available height of the orthogonal element. We should use the 100px from the wrapper ans subtract the margin-top, so the height of the orthogonal element should be 50px. But we're subtracting only 10px (10% of 100px), and setting a 90px height which is wrong. Regarding other browsers is quite amazing that Edge has the very same issue. Firefox doesn't consider the 100px height from the wrapper so the orthogonal element is as tall as it needs due to its contents (so that's a different bug). Also if you check padding the result is quite strange, but it seems a different issue. The padding problem is gone if you remove the "display: inline-block", the margin one is still present on that case.
,
Mar 15 2018
If you're curious about the code, the method that calculates the available size for the orthogonal is: LayoutBox::FillAvailableMeasure() https://chromium.googlesource.com/chromium/src/+/9a36904547f59189e6fd096c6cbc74037251c34a/third_party/WebKit/Source/core/layout/LayoutBox.cpp#2813 As you can see there we're resolving the margins against the available_logical_width which is 100px in this case.
,
Mar 19 2018
I've a simpler example that works right in Firefox and not in the rest of browsers (Chromium, WebKit and Edge). I've a proposed patch to fix the percentage margins issue at: https://chromium-review.googlesource.com/c/chromium/src/+/968522
,
Mar 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78822c6d1c1170be5387fd457444c6c7a969160c commit 78822c6d1c1170be5387fd457444c6c7a969160c Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Mon Mar 19 15:48:27 2018 Fix sizing of orthogonal elements with percentage margins LayoutBox::FillAvailableMeasure() was not considering the case of orthogonal elements when computing the margins. The margins ended up being properly calculated but the size of the orthogonal elements was wrong, as they considered to have more or less space than the available one. The method is modified in order to use the containing block inline size in order to resolve the percentages: https://www.w3.org/TR/css-writing-modes-3/#dimension-mapping BUG=808758 TEST=external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-00*.html Change-Id: Ib8c81dcd14589b3fefe806de3f8f75c000b1cac9 Reviewed-on: https://chromium-review.googlesource.com/968522 Commit-Queue: Koji Ishii <kojii@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#544047} [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-001-ref.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-001.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-002-ref.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-002.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-003-ref.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-003.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-004.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-005-ref.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-005.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-006-ref.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-006.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-007-ref.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-007.html [add] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes/sizing-orthogonal-percentage-margin-008.html [modify] https://crrev.com/78822c6d1c1170be5387fd457444c6c7a969160c/third_party/WebKit/Source/core/layout/LayoutBox.cpp
,
Mar 19 2018
,
Mar 20 2018
So I've reported issue #823683 for the generic problem with paddings and orthogonal items. Apart from that, there's an issue specific to CSS Grid Layout. Check the new attached example, the percentage padding is resolved to 10px as epxected (10% of the column width 100px), however the height of the item is 60px like if it was resolving the padding against the grid container width (10% of 500px). We need to find what's the point where the grid area size is ignored.
,
Mar 20 2018
After some extra investigation I've found out that the bug (or a similar one) is also present with regular items (no orthogonal). Check the new attached example, it's just a regular item (no orthogonal) with a "padding-left: 50%". That padding-left is resolved against the 500px width of the grid container, while resolving the content based minimum tracks. This causes that the column is much wider than needed. The final result is a 250px track and a padding-left of 125px. Note that I'm using "overflow: scroll" in the example, to just have 1 layout. If you remove it the result is still wrong, but different as in the 2nd layout, the override is set (to 250px) and it uses it to resolve the percentage padding. In Firefox and Edge this works fine and the column is 100px width and the item has a padding-left of 50px. The problem is that when the grid area size hasn't been set yet, we should just resolve the percentage padding as 0px. Then the issue with orthogonal elements is somehow related. First we run the algorithm for columns, we know that there's a fixed column of 100px, but we don't set the overrides yet. Then we process the rows, when you're resolving the percentage padding of the orthogonal item it uses the grid container width, as the grid area hasn't been set yet. However the grid area has already been calculated at that stage, so we might need to set it early or something to fix the problem.
,
Mar 21 2018
The issue described in the last comment is also present if you use percentage margins (for example change padding-left by margin-left in the attachment). To fix it we could simply modify LayoutBox::ContainingBlockLogicalWidthForContent() so it returns 0 for grid layout items, if the override is not set. Anyway that won't be enough to solve the orthogonal items problem.
,
Apr 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d commit ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Mon Apr 02 06:07:19 2018 [css-grid] Fix resolution of percentage paddings and margins of grid items We were not resolving properly percentage paddings and margins for tracks that have something like minmax(auto, 100px). The reason was that while computing the minimum size of a grid item, the percentages were resolved against the inline size of the grid container. But for grid items we shouldn't never use the grid container size, but the grid area size, as that's their containing block. The patch modifies ContainingBlockLogicalWidthForContent() and ContainingBlockLogicalHeightForContent() in LayoutBox, so for grid items we return 0 if the area size hasn't been set yet. We never want to use the grid container's sizes in these cases. BUG=808758 TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-* TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-* Change-Id: Ib142e51aee1fe623d38688469b179f01f82eb07b Reviewed-on: https://chromium-review.googlesource.com/980756 Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#547417} [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-001.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-002.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-lr-001.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-lr-002.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-rl-001.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-rl-002.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-001.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-002.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-vertical-lr-001.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-vertical-lr-002.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-vertical-rl-001.html [add] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-vertical-rl-002.html [modify] https://crrev.com/ad7d8f49d5a7bfc93b168da450d4c60dc7aa725d/third_party/WebKit/Source/core/layout/LayoutBox.cpp
,
Apr 19 2018
Issue 834643 has been merged into this issue. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by e...@chromium.org
, Feb 4 2018