Available height is wrongly computed for asbpos items with border-box and padding top
Reported by
san...@012345.ch,
Oct 6 2017
|
|||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36 Steps to reproduce the problem: 1. Open the attached test case and compare it to FF dev What is the expected behavior? A padding at the top but the box should stretch to the bottom. What went wrong? the distance of the padding-top is somehow also used at the bottom --> it looks like the box has a padding-bottom set. Did this work before? No Does this work in other browsers? Yes Chrome version: 61.0.3163.100 Channel: stable OS Version: 10.0 Flash Version: Codepen: https://codepen.io/sandrosc/pen/gGvXOG
,
Oct 9 2017
Thanks for reproducing. The white box you are referring to appears because grid layout wasn't supported until Chrome 57: http://caniuse.com/#feat=css-grid
,
Oct 9 2017
,
Oct 10 2017
It's clearly an issue and it only happens with "box-sizing: border-box" as the title says. Probably it's just a typo somewhere in the code and, of course, lack of tests for "box-sizing: border-box" and a positioned grid container.
,
Oct 10 2017
The issue seems not only related to Grid Layout but more generic. The problem related to Grid is in LayoutBox::AvailableLogicalHeightUsing() which is returning a wrong height in the case reported here. The problem are the following lines (https://chromium.googlesource.com/chromium/src.git/+/75c1d5ecd30560b94e713881f69ef18d831b3adf/third_party/WebKit/Source/core/layout/LayoutBox.cpp#3758): LayoutUnit new_content_height = computed_values.extent_ - block->BorderAndPaddingLogicalHeight() - block->ScrollbarLogicalHeight(); return AdjustContentBoxLogicalHeightForBoxSizing(new_content_height); They are calling AdjustContentBoxLogicalHeightForBoxSizing() without the border and padding, and that is wrong as that method will remove that again if "box-sizing" is "border-box". I guess that the solution would be to avoid removing the border and padding before calling this. The very same code can be found in LayoutBox::ComputeReplacedLogicalHeightUsing() (https://chromium.googlesource.com/chromium/src.git/+/75c1d5ecd30560b94e713881f69ef18d831b3adf/third_party/WebKit/Source/core/layout/LayoutBox.cpp#3629). I am attaching an example showing a very similar problem with an image.
,
Oct 24 2017
Attaching another case without Grid Layout usage, just using -webkit-fill-available for the height. The issue in LayoutBox::AvailableLogicalHeightUsing() will be fixed by https://chromium-review.googlesource.com/c/chromium/src/+/735141 The problem with replaced elements is still pending.
,
Oct 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7c532114fb03f053be230d0abbc9d61d207e4a31 commit 7c532114fb03f053be230d0abbc9d61d207e4a31 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Tue Oct 24 21:42:52 2017 Fix LayoutBox::AvailableLogicalHeightUsing() for positioned items This method was returning a wrong value for positioned items in combination with "box-sizing: border-box". The problem was that if the height of the element is given by its offset properties (top and bottom) we don't need to call AdjustContentBoxLogicalHeightForBoxSizing(). Created a test to verify that the problem is fixed using "height: -webkit-fill-available". Grid Layout is one of the places where AvailableLogicalHeight() is used, so this patch adds a new test for different cases based on grids. The test checks the combination of: positioned and non positioned elements, border-box and content-box, specific size and min-size. BUG= 772512 TEST=fast/box-sizing/fill-available.html TEST=external/wpt/css/css-grid-1/grid-model/grid-box-sizing-001.html Change-Id: Ia5b943c6a3a2e33715fe9883edbbcdcae9d0c608 Reviewed-on: https://chromium-review.googlesource.com/735141 Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com> Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Reviewed-by: Sergio Villar <svillar@igalia.com> Cr-Commit-Position: refs/heads/master@{#511271} [add] https://crrev.com/7c532114fb03f053be230d0abbc9d61d207e4a31/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/grid-model/grid-box-sizing-001.html [add] https://crrev.com/7c532114fb03f053be230d0abbc9d61d207e4a31/third_party/WebKit/LayoutTests/fast/box-sizing/fill-available-expected.html [add] https://crrev.com/7c532114fb03f053be230d0abbc9d61d207e4a31/third_party/WebKit/LayoutTests/fast/box-sizing/fill-available.html [modify] https://crrev.com/7c532114fb03f053be230d0abbc9d61d207e4a31/third_party/WebKit/Source/core/layout/LayoutBox.cpp
,
Oct 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/71cd7c450aebdd58b7368081cd77f27922219e92 commit 71cd7c450aebdd58b7368081cd77f27922219e92 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Fri Oct 27 00:30:07 2017 Fix LayoutBox::ComputeReplacedLogicalHeightUsing() for positioned containing block This method was returning a wrong value when computing percentage heights on replaced elements, if the containing block is a positioned element and its height is given by its offset properties (top and bottom). In that case we don't need to call AdjustContentBoxLogicalHeightForBoxSizing(). This is the same approach that was used to fix LayoutBox::AvailableLogicalHeightUsing() in r735141. The patch adds a new test to verify that the problem is fixed for a replaced element with 100% height. And also modifies the test added in the previous patch (fill-available.html) so it uses the same format and verifies more stuff. BUG= 772512 TEST=fast/box-sizing/replaced.html Change-Id: I74ccc85ff81a8c7e41d089d3c336e165dde8936a Reviewed-on: https://chromium-review.googlesource.com/737629 Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com> Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Cr-Commit-Position: refs/heads/master@{#512041} [delete] https://crrev.com/f4f97a92633d239cfd7911dc576600ff8c0dd4b0/third_party/WebKit/LayoutTests/fast/box-sizing/fill-available-expected.html [modify] https://crrev.com/71cd7c450aebdd58b7368081cd77f27922219e92/third_party/WebKit/LayoutTests/fast/box-sizing/fill-available.html [add] https://crrev.com/71cd7c450aebdd58b7368081cd77f27922219e92/third_party/WebKit/LayoutTests/fast/box-sizing/replaced.html [modify] https://crrev.com/71cd7c450aebdd58b7368081cd77f27922219e92/third_party/WebKit/Source/core/layout/LayoutBox.cpp
,
Oct 27 2017
All cases should be fixed now. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by krajshree@chromium.org
, Oct 9 2017Status: Untriaged (was: Unconfirmed)
109 KB
109 KB View Download