Take padding into account during calculation of overflow constraining rect for sticky positioning.

Take padding into account during calculation of overflow constraining rect for sticky positioning.

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

[ojan, 2014-02-19 20:03:23]

FYI, it's fine to keep working on the implementation for this for now, but the
biggest delay in actually shipping sticky positioning will be getting a spec. I
encourage you to push on the CSS working group to get a spec for this ASAP. It
will take longer to get the spec completed than to get the code ready to ship I
think.

Incidentally, having a spec will make these patches easier to review because
there's something to reference when trying to understand what the correct
behavior should be.

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

https://codereview.chromium.org/168823003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:347:
clipRect.move(enclosingClippingBox->paddingLeft(),
enclosingClippingBox->paddingTop());
The equivalent webkit patch didn't have this
(https://bugs.webkit.org/attachment.cgi?id=224328&action=review). Can you
explain why this is needed?

[ostap, 2014-02-19 20:49:29]

On 2014/02/19 20:03:23, ojan wrote:
> FYI, it's fine to keep working on the implementation for this for now, but the
> biggest delay in actually shipping sticky positioning will be getting a spec.
I
> encourage you to push on the CSS working group to get a spec for this ASAP.

Yes, we have started working on this.
Thanks.

[ostap, 2014-02-19 20:49:59]

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

https://codereview.chromium.org/168823003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.cpp:347:
clipRect.move(enclosingClippingBox->paddingLeft(),
enclosingClippingBox->paddingTop());
On 2014/02/19 20:03:24, ojan wrote:
> The equivalent webkit patch didn't have this
> (https://bugs.webkit.org/attachment.cgi?id=224328&action=review). Can you
> explain why this is needed?

Webkit takes padding into account for the position in
computeStickyPositionConstraints() .
That's because of this commit: http://trac.webkit.org/changeset/152998 
I don't think this change is relevant for chromium.

[ojan, 2014-02-19 22:27:02]

lgtm

[commit-bot: I haz the power, 2014-02-19 22:30:09]

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

[commit-bot: I haz the power, 2014-02-19 22:30:15]

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

[commit-bot: I haz the power, 2014-02-19 22:30:15]

Failed to apply patch for Source/core/rendering/RenderBoxModelObject.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBoxModelObject.cpp
  Hunk #1 FAILED at 344.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/rendering/RenderBoxModelObject.cpp.rej

Patch:       Source/core/rendering/RenderBoxModelObject.cpp
Index: Source/core/rendering/RenderBoxModelObject.cpp
diff --git a/Source/core/rendering/RenderBoxModelObject.cpp
b/Source/core/rendering/RenderBoxModelObject.cpp
index
d5dfa279e43f7722398d7af39a0c67016f61c87c..109268ce6200f80c0db9196e7778737d769d1dc6
100644
--- a/Source/core/rendering/RenderBoxModelObject.cpp
+++ b/Source/core/rendering/RenderBoxModelObject.cpp
@@ -344,6 +344,9 @@ LayoutSize RenderBoxModelObject::stickyPositionOffset()
const
     if (enclosingClippingLayer) {
         RenderBox* enclosingClippingBox =
toRenderBox(enclosingClippingLayer->renderer());
         LayoutRect clipRect =
enclosingClippingBox->overflowClipRect(LayoutPoint(), 0); // FIXME: make this
work in regions.
+        clipRect.move(enclosingClippingBox->paddingLeft(),
enclosingClippingBox->paddingTop());
+        clipRect.contract(LayoutSize(enclosingClippingBox->paddingLeft() +
enclosingClippingBox->paddingRight(),
+            enclosingClippingBox->paddingTop() +
enclosingClippingBox->paddingBottom()));
         constrainingRect =
enclosingClippingBox->localToContainerQuad(FloatRect(clipRect),
view()).boundingBox();
     } else {
         LayoutRect viewportRect =
view()->frameView()->viewportConstrainedVisibleContentRect();

[ostap, 2014-02-19 22:43:20]

The CQ bit was checked by sl.ostapenko@samsung.com

[commit-bot: I haz the power, 2014-02-19 22:47:49]

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

[commit-bot: I haz the power, 2014-02-19 23:13:52]

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

[commit-bot: I haz the power, 2014-02-20 01:53:18]

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

[commit-bot: I haz the power, 2014-02-20 01:53:18]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[ostap, 2014-02-21 05:58:39]

The CQ bit was checked by sl.ostapenko@samsung.com

[commit-bot: I haz the power, 2014-02-21 05:58:47]

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

[commit-bot: I haz the power, 2014-02-21 06:31:49]

Change committed as 167572