New issue
Advanced search Search tips

Issue 772512 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Available height is wrongly computed for asbpos items with border-box and padding top

Reported by san...@012345.ch, Oct 6 2017

Issue description

UserAgent: 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
 
index.html
637 bytes View Download
Labels: M-63 Needs-Triage-M61 OS-Linux OS-Mac
Status: Untriaged (was: Unconfirmed)
Able to reproduce this issue on Mac 10.12.6, Win-10 and Ubuntu 14.04 using chrome reported version #61.0.3163.100 and latest canary #63.0.3235.0.

This is a non-regression issue as it is observed from M57 old builds. 
Note: From M50 till M56 builds, opening the URL:https://codepen.io/sandrosc/pen/gGvXOG, yielded a complete while box as in the attached screen shot.

Hence, marking it as untriaged to get more inputs from dev team.

Thanks...!!
772512.JPG
109 KB View Download

Comment 2 by san...@012345.ch, 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
Components: -Blink>CSS Blink>Layout>Grid

Comment 4 by r...@igalia.com, Oct 10 2017

Cc: svil...@igalia.com jfernan...@igalia.com r...@igalia.com
Status: Available (was: Untriaged)
Summary: [css-grid] Gap at the bottom of the box when using grid layout with a padding at the top, position absolute and border-box (was: Gap at the bottom of the box when using grid layout with a padding at the top, position absolute and border-box)
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.

Comment 5 by r...@igalia.com, Oct 10 2017

Components: -Blink>Layout>Grid Blink>Layout
Summary: Available height is wrongly computed for asbpos items with border-box and padding top (was: [css-grid] Gap at the bottom of the box when using grid layout with a padding at the top, position absolute and border-box)
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.

test-padding-bottom.html
413 bytes View Download
test-padding-bottom.png
29.5 KB View Download

Comment 6 by r...@igalia.com, 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.
fill-available.html
328 bytes View Download
fill-available.png
5.1 KB View Download
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Comment 9 by r...@igalia.com, Oct 27 2017

Owner: r...@igalia.com
Status: Fixed (was: Available)
All cases should be fixed now.

Sign in to add a comment