Fix overconstrained behaviour for position: sticky.

Fix overconstrained behaviour for position: sticky.

Ignore one of sticky offsets if both opposite offsets are specified and sum of
opposite offsets is bigger than corresponding dimension of containing block.

BUG=370632
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174678

[Julien - ping for review, 2014-05-16 13:17:49]

It really looks like this change needs more details: this change is not fixing
all the over-constrained cases but the description seems like it does. We should
update the description to reflect that and add the appropriate FIXMEs in the
code.

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:332: float offsetsSum =
constraints.rightOffset() + constraints.leftOffset();
We should rename this to a better name: verticalOffsets

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:340:
constraints.removeAnchorEdge(ViewportConstraints::AnchorEdgeLeft);
It would be cleaner if we didn't set the anchor edge in the first place.

The logic doesn't work if we set the right offset with a LTR direction so that
offsetsSum is bigger that the containing block's width. This is an undefined
behavior in the specification but I would have liked to see this mentioned.

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:359: }
Shouldn't you check if our writing mode is not bottom-to-top? This is similar to
what you did above for RTL direction.

[ostap, 2014-05-22 04:33:09]

On 2014/05/16 13:17:49, Julien Chaffraix - CET wrote:
> It really looks like this change needs more details: this change is not fixing
> all the over-constrained cases but the description seems like it does. We
should
> update the description to reflect that and add the appropriate FIXMEs in the
> code.
> 

I'm sorry. I've missed some checks in the 1st patch, so it was not really clear
what is "over-constrained". Similar to position relative, over-constrained means
that both top and bottom or right and left edges are specified. I think with new
patch which takes this into account covers all over-constrained cases.

[ostap, 2014-05-22 04:33:18]

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:332: float offsetsSum =
constraints.rightOffset() + constraints.leftOffset();
On 2014/05/16 13:17:49, Julien Chaffraix - CET wrote:
> We should rename this to a better name: verticalOffsets

Done.

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:340:
constraints.removeAnchorEdge(ViewportConstraints::AnchorEdgeLeft);
On 2014/05/16 13:17:49, Julien Chaffraix - CET wrote:
> It would be cleaner if we didn't set the anchor edge in the first place.

Done

> The logic doesn't work if we set the right offset with a LTR direction so that
> offsetsSum is bigger that the containing block's width. This is an undefined
> behavior in the specification but I would have liked to see this mentioned.

Actually, the over-constrained means that both left and right or top and bottom
edges are not auto (similar to relative positioning). This check was actually
missed from my 1st patch and I fixed this.
When only one edge offset is bigger than containing block is a different case
and existing calculations do it properly similar to mozilla implementation and
as it was discussed on the CSS mailing list here:
http://lists.w3.org/Archives/Public/www-style/2014Mar/0141.html
It seems this case (with single offset bigger than container) is not covered by
any existing layout test, but it is not related to this patch and I would make
it separately.

https://codereview.chromium.org/267063007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:359: }
On 2014/05/16 13:17:49, Julien Chaffraix - CET wrote:
> Shouldn't you check if our writing mode is not bottom-to-top? This is similar
to
> what you did above for RTL direction.

The over-constrained behavior is done similar to position:relative. If we change
it here then, I think, relative positioning should be also changed and spec.
updated. But I'll ask on mozilla bug report which has related discussion:
https://bugzilla.mozilla.org/show_bug.cgi?id=979602

[Julien - ping for review, 2014-05-22 15:49:02]

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
File
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-ltr-expected.html
(right):

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-ltr-expected.html:8:
height: 600px;
Same comment about overflowing.

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
File
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-rtl-expected.html
(right):

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-rtl-expected.html:54:
<div id="overflow">
This overflows the viewport (set to 800x600 in DRT) by 2 pixels horizontally
(the border) and thus removes some testing space. Fixing it just means to put
box-sizing: border-box or fixing your size in the style declaration.

https://codereview.chromium.org/267063007/diff/40001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingConstraints.h (right):

https://codereview.chromium.org/267063007/diff/40001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingConstraints.h:62: void
removeAnchorEdge(AnchorEdgeFlags edgeFlag) { m_anchorEdges &= ~edgeFlag; }
This function is unused so it should be removed.

https://codereview.chromium.org/267063007/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/267063007/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:327: || horizontalOffsets +
containerContentRect.width().toFloat() > constrainingRect.width()) {
[It's better to use a generated 'reply' comment, that avoids having the reviewer
do some copy & paste which loses a lot of the context :( ]

> When only one edge offset is bigger than containing block is a different case
> and existing calculations do it properly similar to mozilla implementation and
> as it was discussed on the CSS mailing list here:
> http://lists.w3.org/Archives/Public/www-style/2014Mar/0141.html
> It seems this case (with single offset bigger than container) is not covered
by
> any existing layout test, but it is not related to this patch and I would make
> it separately.

I am fine with that (I usually ask for a FIXME). It seems to me that CSS 2.1 is
clear on what would happen in this case so we may as well just match the in flow
case and have some coverage for it.

https://codereview.chromium.org/267063007/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:343: bool skipBottom = false;
Let's add a FIXME about that and ideally a link to the www-style change request.

[ostap, 2014-05-22 23:13:11]

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
File
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-ltr-expected.html
(right):

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-ltr-expected.html:8:
height: 600px;
On 2014/05/22 15:49:02, Julien Chaffraix - CET wrote:
> Same comment about overflowing.

Done.

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
File
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-rtl-expected.html
(right):

https://codereview.chromium.org/267063007/diff/40001/LayoutTests/fast/css/sti...
LayoutTests/fast/css/sticky/sticky-horizontally-overconstrained-rtl-expected.html:54:
<div id="overflow">
On 2014/05/22 15:49:02, Julien Chaffraix - CET wrote:
> This overflows the viewport (set to 800x600 in DRT) by 2 pixels horizontally
> (the border) and thus removes some testing space. Fixing it just means to put
> box-sizing: border-box or fixing your size in the style declaration.

Done.

https://codereview.chromium.org/267063007/diff/40001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingConstraints.h (right):

https://codereview.chromium.org/267063007/diff/40001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingConstraints.h:62: void
removeAnchorEdge(AnchorEdgeFlags edgeFlag) { m_anchorEdges &= ~edgeFlag; }
On 2014/05/22 15:49:02, Julien Chaffraix - CET wrote:
> This function is unused so it should be removed.

Done.

https://codereview.chromium.org/267063007/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/267063007/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:343: bool skipBottom = false;
On 2014/05/22 15:49:02, Julien Chaffraix - CET wrote:
> Let's add a FIXME about that and ideally a link to the www-style change
request.

Done.

[Julien - ping for review, 2014-05-23 10:28:07]

lgtm

[commit-bot: I haz the power, 2014-05-23 10:28:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/267063007/60001

[commit-bot: I haz the power, 2014-05-23 10:51:07]

Change committed as 174678