Detect a change in border that affects a positioned object's height or position

Detect a change in border that affects a positioned object's height or position

BUG=561612
Committed: https://crrev.com/4d23825e7f84e0057442cd94f4870547da64c483
Cr-Commit-Position: refs/heads/master@{#376962}

[rhogan, 2016-02-08 20:58:20]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-02-09 09:08:52]

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html
(right):

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html:8:
border-width: 150px 150px;
This has no effect with border-style being none, so you can just remove the
declaration.

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html:17:
border-width: 150px 150px;
This overrides the width set in the shorthand above.

Why not just "border: 150px solid rgba(...)"?

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html:19:
#parent div {
looks like #child to me. :)

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html:29:
<p>crbug.com/581612: Changing block-direction border width on a positioned
element with auto height should flex the height of its descendants.</p>
How about a human observable pass condition? There seems to be pretty borders
and backgrounds here, so why not describe what we're expected to see?

I like tests that say "There should be no red below", or "There should be a
<your favorite color> square below", or something else that a human can use to
easily determine that it's a pass or fail.

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1181: if
(style->hasStaticBlockPosition(isHorizontal) || checkForChangeInAvailableHeight)
{
I was wondering why we don't need to do the same for width changes. Turns out
that it's widthAvailableToChildrenHasChanged() (thanks to
m_widthAvailableToChildrenChanged being set) that comes to the rescue and sets
relayoutChildren. Shouldn't we just introduce a similar mechanism for heights,
instead of having two completely different mechanisms?

[rhogan, 2016-02-09 18:38:30]

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1181: if
(style->hasStaticBlockPosition(isHorizontal) || checkForChangeInAvailableHeight)
{
On 2016/02/09 at 09:08:51, mstensho wrote:
> I was wondering why we don't need to do the same for width changes. Turns out
that it's widthAvailableToChildrenHasChanged() (thanks to
m_widthAvailableToChildrenChanged being set) that comes to the rescue and sets
relayoutChildren. Shouldn't we just introduce a similar mechanism for heights,
instead of having two completely different mechanisms?

If we can come up with additional use cases, definitely. I think we're
collecting them in this function now. :)

[mstensho (USE GERRIT), 2016-02-10 10:39:10]

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1181: if
(style->hasStaticBlockPosition(isHorizontal) || checkForChangeInAvailableHeight)
{
On 2016/02/09 18:38:30, rhogan wrote:
> On 2016/02/09 at 09:08:51, mstensho wrote:
> > I was wondering why we don't need to do the same for width changes. Turns
out
> that it's widthAvailableToChildrenHasChanged() (thanks to
> m_widthAvailableToChildrenChanged being set) that comes to the rescue and sets
> relayoutChildren. Shouldn't we just introduce a similar mechanism for heights,
> instead of having two completely different mechanisms?
> 
> If we can come up with additional use cases, definitely.

<!DOCTYPE html>
<p>There should be a blue <em>square</em> below.</p>
<div id="container" style="height:100px; box-sizing:border-box;">
    <div style="-webkit-writing-mode:vertical-rl; width:40px; height:100%;
background:blue;"></div>
</div>
<script>
    document.body.offsetTop;
    document.getElementById("container").style.paddingBottom = '60px';
</script>

[rhogan, 2016-02-11 19:47:50]

On 2016/02/10 at 10:39:10, mstensho wrote:
>
https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
> 
>
https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1181: if
(style->hasStaticBlockPosition(isHorizontal) || checkForChangeInAvailableHeight)
{
> On 2016/02/09 18:38:30, rhogan wrote:
> > On 2016/02/09 at 09:08:51, mstensho wrote:
> > > I was wondering why we don't need to do the same for width changes. Turns
out
> > that it's widthAvailableToChildrenHasChanged() (thanks to
> > m_widthAvailableToChildrenChanged being set) that comes to the rescue and
sets
> > relayoutChildren. Shouldn't we just introduce a similar mechanism for
heights,
> > instead of having two completely different mechanisms?
> > 
> > If we can come up with additional use cases, definitely.
> 
> <!DOCTYPE html>
> <p>There should be a blue <em>square</em> below.</p>
> <div id="container" style="height:100px; box-sizing:border-box;">
>     <div style="-webkit-writing-mode:vertical-rl; width:40px; height:100%;
background:blue;"></div>
> </div>
> <script>
>     document.body.offsetTop;
>     document.getElementById("container").style.paddingBottom = '60px';
> </script>

This seems like a problem with borderOrPaddingLogicalWidthChanged() in: 

 m_widthAvailableToChildrenChanged |= oldStyle && diff.needsFullLayout() &&
needsLayout() && borderOrPaddingLogicalWidthChanged(*oldStyle, newStyle);

It needs to see if the change in border/padding will affect the child's width
according to the child's direction as well as it's own.

I'm not sure the right way to do that offhand but it seems separate to the issue
in positioned layout.

[mstensho (USE GERRIT), 2016-02-11 20:17:47]

On 2016/02/11 19:47:50, rhogan wrote:
> On 2016/02/10 at 10:39:10, mstensho wrote:
> >
>
https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1181: if
> (style->hasStaticBlockPosition(isHorizontal) ||
checkForChangeInAvailableHeight)
> {
> > On 2016/02/09 18:38:30, rhogan wrote:
> > > On 2016/02/09 at 09:08:51, mstensho wrote:
> > > > I was wondering why we don't need to do the same for width changes.
Turns
> out
> > > that it's widthAvailableToChildrenHasChanged() (thanks to
> > > m_widthAvailableToChildrenChanged being set) that comes to the rescue and
> sets
> > > relayoutChildren. Shouldn't we just introduce a similar mechanism for
> heights,
> > > instead of having two completely different mechanisms?
> > > 
> > > If we can come up with additional use cases, definitely.
> > 
> > <!DOCTYPE html>
> > <p>There should be a blue <em>square</em> below.</p>
> > <div id="container" style="height:100px; box-sizing:border-box;">
> >     <div style="-webkit-writing-mode:vertical-rl; width:40px; height:100%;
> background:blue;"></div>
> > </div>
> > <script>
> >     document.body.offsetTop;
> >     document.getElementById("container").style.paddingBottom = '60px';
> > </script>
> 
> This seems like a problem with borderOrPaddingLogicalWidthChanged() in: 
> 
>  m_widthAvailableToChildrenChanged |= oldStyle && diff.needsFullLayout() &&
> needsLayout() && borderOrPaddingLogicalWidthChanged(*oldStyle, newStyle);
> 
> It needs to see if the change in border/padding will affect the child's width
> according to the child's direction as well as it's own.
> 
> I'm not sure the right way to do that offhand but it seems separate to the
issue
> in positioned layout.

It's the padding-bottom of a horizontal-tb block that changes, so it's the
padding *logical height* that changes, and we don't have the necessary machinery
to detect it (we only care about border+padding logical width changes at the
moment). It's the *logical height* available to children that changes, and we
don't detect it when re-laying out.

Setting relayoutChildren in layoutBlockFlow() in this situation should fix both
my new test and the original bug. And then we'll have consistent pieces of
machinery, regardless of whether it's the border+padding logical width or height
that changes. :)

[rhogan, 2016-02-16 20:25:53]

On 2016/02/11 at 20:17:47, mstensho wrote:
> On 2016/02/11 19:47:50, rhogan wrote:
> > On 2016/02/10 at 10:39:10, mstensho wrote:
> > >
> >
https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1674323002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1181: if
> > (style->hasStaticBlockPosition(isHorizontal) ||
checkForChangeInAvailableHeight)
> > {
> > > On 2016/02/09 18:38:30, rhogan wrote:
> > > > On 2016/02/09 at 09:08:51, mstensho wrote:
> > > > > I was wondering why we don't need to do the same for width changes.
Turns
> > out
> > > > that it's widthAvailableToChildrenHasChanged() (thanks to
> > > > m_widthAvailableToChildrenChanged being set) that comes to the rescue
and
> > sets
> > > > relayoutChildren. Shouldn't we just introduce a similar mechanism for
> > heights,
> > > > instead of having two completely different mechanisms?
> > > > 
> > > > If we can come up with additional use cases, definitely.
> > > 
> > > <!DOCTYPE html>
> > > <p>There should be a blue <em>square</em> below.</p>
> > > <div id="container" style="height:100px; box-sizing:border-box;">
> > >     <div style="-webkit-writing-mode:vertical-rl; width:40px; height:100%;
> > background:blue;"></div>
> > > </div>
> > > <script>
> > >     document.body.offsetTop;
> > >     document.getElementById("container").style.paddingBottom = '60px';
> > > </script>
> > 
> > This seems like a problem with borderOrPaddingLogicalWidthChanged() in: 
> > 
> >  m_widthAvailableToChildrenChanged |= oldStyle && diff.needsFullLayout() &&
> > needsLayout() && borderOrPaddingLogicalWidthChanged(*oldStyle, newStyle);
> > 
> > It needs to see if the change in border/padding will affect the child's
width
> > according to the child's direction as well as it's own.
> > 
> > I'm not sure the right way to do that offhand but it seems separate to the
issue
> > in positioned layout.
> 
> It's the padding-bottom of a horizontal-tb block that changes, so it's the
padding *logical height* that changes, and we don't have the necessary machinery
to detect it (we only care about border+padding logical width changes at the
moment). It's the *logical height* available to children that changes, and we
don't detect it when re-laying out.
> 
> Setting relayoutChildren in layoutBlockFlow() in this situation should fix
both my new test and the original bug. And then we'll have consistent pieces of
machinery, regardless of whether it's the border+padding logical width or height
that changes. :)
Ready for another look.

[mstensho (USE GERRIT), 2016-02-17 10:19:42]

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html
(right):

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/positioning/positioned-container-changes-block-direction-border-with-positioned-descendant.html:32:
<div id="child" data-expected-height=140></div>
There are tabs here.

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:297: // end up being the
same. We keep track of this change so in layoutBlock, we can know to set
relayoutChildren=true.
Should probably update this comment.

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:255: if
(newStyle.isHorizontalWritingMode() && logicalExtent == LogicalWidth) {
I think "if (newStyle.isHorizontalWritingMode() == (logicalExtent ==
LogicalWidth))" should work better, or you'd end up always checking physical
padding/border height for LogicalHeight.

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.h:494: mutable unsigned
m_heightAvailableToChildrenChanged  : 1;
Please put this next to m_widthAvailableToChildrenChanged. And does it really
have to be mutable?

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1674323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:433:
m_heightAvailableToChildrenChanged = false;
So we'll only reset this flag for LayoutBlockFlow, and not for other LayoutBlock
types, such as LayoutTable and LayoutFlexibleBox.

m_widthAvailableToChildrenChanged has the same problem, though.

updateLogicalWidthAndColumnWidth(), which *may* call
widthAvailableToChildrenHasChanged(), which resets the flag.
updateLogicalWidthAndColumnWidth() is only called from LayoutBlockFlow and
LayoutFlexibleBox, but the flag is never reset if oldWidth != logicalWidth() in
updateLogicalWidthAndColumnWidth().

We should probably clean up this, but that's for a different CL. Could you add a
TODO here, though?

[rhogan, 2016-02-21 10:20:30]

On 2016/02/17 at 10:19:42, mstensho wrote:
> We should probably clean up this, but that's for a different CL. Could you add
a TODO here, though?

Applied your comments. THanks!

[rhogan, 2016-02-21 10:20:30]

On 2016/02/17 at 10:19:42, mstensho wrote:
> We should probably clean up this, but that's for a different CL. Could you add
a TODO here, though?

Applied your comments. THanks!

[mstensho (USE GERRIT), 2016-02-22 10:05:19]

lgtm

https://codereview.chromium.org/1674323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1674323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:108: typedef
WTF::HashSet<LayoutBlock*> DelayedUpdateScrollInfoSet;
I've mentioned this quite a few times already:

Can you PLEASE submit master rebases and your own changes as separate patches?

It's impossible for me to figure out what *you* did here and what *they* did
here.

https://codereview.chromium.org/1674323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:904: // TODO(robhogan):
Does m_widthAvailableToChildrenChanged always get reset when it needs to?
To answer your rhetorical question: No! :)

Yeah, I think this is a suitable place for the TODO. I think it's problematic
that you have to ask if the flag is set just in order to be able to reset it.
Just look at what updateLogicalWidthAndColumnWidth() does.

https://codereview.chromium.org/1674323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1004: bool static
changeInAvailableLogicalHeightAffectsChild(LayoutBlock* parent, LayoutBox&
child)
"bool static"? Can you really say that? Shouldn't it be "static bool"? Could
even say "static inline bool".

[rhogan, 2016-02-22 21:09:00]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-02-22 21:09:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1674323002/#ps120001
(title: "Updated")

[commit-bot: I haz the power, 2016-02-22 21:11:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674323002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674323002/120001

[rhogan, 2016-02-22 21:14:43]

On 2016/02/22 at 10:05:19, mstensho wrote:
> I've mentioned this quite a few times already:
> 
> Can you PLEASE submit master rebases and your own changes as separate patches?
> 
> It's impossible for me to figure out what *you* did here and what *they* did
here.

All I did for this patch was:

git checkout -b 561612-2 origin/master
git cl patch 1674323002
[my changes]
git cl upload --no-find-copies

I know you use the old ui in rietveld and the artefacts you're describing don't
show up in the new ui. Is it possible that a bug has crept in to rietveld on the
old ui? I'm struggling to understand what I'm doing wrong to cause this issue
for you.

[commit-bot: I haz the power, 2016-02-23 00:25:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 00:25:23]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rhogan, 2016-02-23 07:22:38]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-02-23 07:23:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674323002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674323002/120001

[commit-bot: I haz the power, 2016-02-23 10:22:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-23 10:23:27]

Description was changed from

==========
Detect a change in border that affects a positioned object's height or position

BUG=561612
==========

to

==========
Detect a change in border that affects a positioned object's height or position

BUG=561612

Committed: https://crrev.com/4d23825e7f84e0057442cd94f4870547da64c483
Cr-Commit-Position: refs/heads/master@{#376962}
==========

[commit-bot: I haz the power, 2016-02-23 10:23:28]

Patchset 7 (id:??) landed as
https://crrev.com/4d23825e7f84e0057442cd94f4870547da64c483
Cr-Commit-Position: refs/heads/master@{#376962}