third_party/WebKit/Source/core/layout/LayoutBox.cpp - Issue 1876123002: Absolute positioned child with percent should include containing block padding

Side by Side Diff: third_party/WebKit/Source/core/layout/LayoutBox.cpp

Issue 1876123002: Absolute positioned child with percent should include containing block padding (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Update commit message Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« third_party/WebKit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-left.html ('K') | « third_party/WebKit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-left-expected.txt ('k') | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)	2 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)

3 * (C) 1999 Antti Koivisto (koivisto@kde.org)	3 * (C) 1999 Antti Koivisto (koivisto@kde.org)

4 * (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)	4 * (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)

5 * (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)	5 * (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)

6 * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserv ed.	6 * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserv ed.

7 * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.	7 * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.

8 *	8 *

9 * This library is free software; you can redistribute it and/or	9 * This library is free software; you can redistribute it and/or

10 * modify it under the terms of the GNU Library General Public	10 * modify it under the terms of the GNU Library General Public

(...skipping 1609 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1620 height -= cb->paddingLogicalHeight();	1620 height -= cb->paddingLogicalHeight();

1621 return height;	1621 return height;

1622 }	1622 }

1623	1623

1624 LayoutUnit LayoutBox::containingBlockLogicalWidthForContent() const	1624 LayoutUnit LayoutBox::containingBlockLogicalWidthForContent() const

1625 {	1625 {

1626 if (hasOverrideContainingBlockLogicalWidth())	1626 if (hasOverrideContainingBlockLogicalWidth())

1627 return overrideContainingBlockContentLogicalWidth();	1627 return overrideContainingBlockContentLogicalWidth();

1628	1628

1629 LayoutBlock* cb = containingBlock();	1629 LayoutBlock* cb = containingBlock();

	1630 if (!isTable() && styleRef().position() == AbsolutePosition)
	mstensho (USE GERRIT) 2016/05/11 08:36:22 Why not for tables? And rather than "styleRef().p Why not for tables? And rather than "styleRef().position() == AbsolutePosition", you could say "isOutOfFlowPositioned()". Deokjin Kim 2016/05/11 14:10:04 In case of tables, even though the absolute positi Show quoted text On 2016/05/11 08:36:22, mstensho wrote: > Why not for tables? In case of tables, even though the absolute positioned child has percent-based width/height, containing block's width doesn't need to include padding. In fact, without "!Table()", test case "fast/table/absolute-table-percent-lengths.html" is failed. Show quoted text > And rather than "styleRef().position() == AbsolutePosition", you could say > "isOutOfFlowPositioned()". Done. mstensho (USE GERRIT) 2016/05/12 09:34:03 That's weird. I don't think there's anything in th Show quoted text On 2016/05/11 14:10:04, Deokjin Kim wrote: > On 2016/05/11 08:36:22, mstensho wrote: > > Why not for tables? > In case of tables, even though the absolute positioned child has percent-based > width/height, containing block's width doesn't need to include padding. In fact, > without "!Table()", test case "fast/table/absolute-table-percent-lengths.html" > is failed. That's weird. I don't think there's anything in the spec that suggests such behavior. We need to figure out why this happens. It's too unsettling to accept without a good explanation. Deokjin Kim 2016/05/12 11:24:20 In updateLogicalWidth() of LayoutTable.cpp, if abs Show quoted text On 2016/05/12 09:34:03, mstensho wrote: > On 2016/05/11 14:10:04, Deokjin Kim wrote: > > On 2016/05/11 08:36:22, mstensho wrote: > > > Why not for tables? > > In case of tables, even though the absolute positioned child has percent-based > > width/height, containing block's width doesn't need to include padding. In > fact, > > without "!Table()", test case "fast/table/absolute-table-percent-lengths.html" > > is failed. > > That's weird. I don't think there's anything in the spec that suggests such > behavior. > We need to figure out why this happens. It's too unsettling to accept without a > good explanation. In updateLogicalWidth() of LayoutTable.cpp, if absolute or fixed, then add paddingLogicalWidth 1 more time. I think this code is not needed anymore.
	1631 return cb->clientLogicalWidth();
	mstensho (USE GERRIT) 2016/05/11 08:36:22 Right, the padding box (and not the content box) o Right, the padding box (and not the content box) of the containing block is to be used for absolutely positioned descendants, but that's a very fundamental thing in layout. Makes you wonder how we survived without this check in the past, doesn't it? Surely we were already able to lay out absolutely positioned objects correctly? Looks like containingBlockLogicalWidthForContent() isn't called much at all for absolutely positioned descendants. See e.g. LayoutBox::computeLogicalWidth(), where we perform an early check for isOutOfFlowPositioned() and defer to computePositionedLogicalWidth() for such objects, which doesn't involve calling containingBlockLogicalWidthForContent(), at least not directly. However, if the absolutely positioned child has percent-based padding, containingBlockLogicalWidthForContent() is involved to resolve the percentages, and you're (inadvertently?) fixing an old layout bug here, which it seems we have no test coverage for. Here's what you fixed: <!DOCTYPE html> <p>The word "PASS" should be seen below, to the right of a black rectangle.</p> <div style="position:relative; padding:2em; width:4em; color:black; background:black;"> <div style="position:absolute; left:0; top:0; padding-left:100%;">PASS</div> </div> We need a layout test for this as well, and the description needs to be updated, since you're fixing more than what you claim. :) Deokjin Kim 2016/05/11 14:10:04 First time I thought that this code is only used f Show quoted text On 2016/05/11 08:36:22, mstensho wrote: > Right, the padding box (and not the content box) of the containing block is to > be used for absolutely positioned descendants, but that's a very fundamental > thing in layout. Makes you wonder how we survived without this check in the > past, doesn't it? Surely we were already able to lay out absolutely positioned > objects correctly? First time I thought that this code is only used for getComputedStyle. To answer your question, I need more time for investigation. :) Show quoted text > Looks like containingBlockLogicalWidthForContent() isn't called much at all for > absolutely positioned descendants. See e.g. LayoutBox::computeLogicalWidth(), > where we perform an early check for isOutOfFlowPositioned() and defer to > computePositionedLogicalWidth() for such objects, which doesn't involve calling > containingBlockLogicalWidthForContent(), at least not directly. > > However, if the absolutely positioned child has percent-based padding, > containingBlockLogicalWidthForContent() is involved to resolve the > percentages, and you're (inadvertently?) fixing an old layout bug here, which it > seems we have no test coverage for. > > Here's what you fixed: > > <!DOCTYPE html> > <p>The word "PASS" should be seen below, to the right of a black rectangle.</p> > <div style="position:relative; padding:2em; width:4em; color:black; > background:black;"> > <div style="position:absolute; left:0; top:0; padding-left:100%;">PASS</div> > </div> Done. Show quoted text > We need a layout test for this as well, and the description needs to be updated, > since you're fixing more than what you claim. :) Done.
1630 return cb->availableLogicalWidth();	1632 return cb->availableLogicalWidth();

1631 }	1633 }

1632	1634

1633 LayoutUnit LayoutBox::containingBlockLogicalHeightForContent(AvailableLogicalHei ghtType heightType) const	1635 LayoutUnit LayoutBox::containingBlockLogicalHeightForContent(AvailableLogicalHei ghtType heightType) const

1634 {	1636 {

1635 if (hasOverrideContainingBlockLogicalHeight())	1637 if (hasOverrideContainingBlockLogicalHeight())

1636 return overrideContainingBlockContentLogicalHeight();	1638 return overrideContainingBlockContentLogicalHeight();

1637	1639

1638 LayoutBlock* cb = containingBlock();	1640 LayoutBlock* cb = containingBlock();

1639 return cb->availableLogicalHeight(heightType);	1641 return cb->availableLogicalHeight(heightType);

(...skipping 3097 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
4737	4739

4738 void LayoutBox::clearPercentHeightDescendants()	4740 void LayoutBox::clearPercentHeightDescendants()

4739 {	4741 {

4740 for (LayoutObject* curr = slowFirstChild(); curr; curr = curr->nextInPreOrde r(this)) {	4742 for (LayoutObject* curr = slowFirstChild(); curr; curr = curr->nextInPreOrde r(this)) {

4741 if (curr->isBox())	4743 if (curr->isBox())

4742 toLayoutBox(curr)->removeFromPercentHeightContainer();	4744 toLayoutBox(curr)->removeFromPercentHeightContainer();

4743 }	4745 }

4744 }	4746 }

4745	4747

4746 } // namespace blink	4748 } // namespace blink

OLD	NEW