Issue 103213002: position:sticky should stick for the enclosing overflow ancestor

Issue 103213002: position:sticky should stick for the enclosing overflow ancestor (Closed)

Created:
7 years ago by ostap

Modified:
6 years, 11 months ago

Reviewers:
ojan, dglazkov1, shawnsingh, Julien - ping for review

CC:
blink-reviews, bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, blink-layers+watch_chromium.org, jchaffraix+rendering

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

position:sticky should stick for the enclosing overflow ancestor Make position: -webkit-sticky be constrained by an enclosing ancestor with non-visible overflow if there is one, rather than being constained always by the viewport. Port of changeset from webkit: http://trac.webkit.org/changeset/150395 BUG=322972, 231753 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164812

Patch Set 1 #

Total comments: 10

Patch Set 2 : Rebase patch + updated by review comments. #

Total comments: 6

Patch Set 3 : Patch updated by review comments. #

Patch Set 4 : Add back changes in RenderBoxModelObject.cpp eaten by rebase. #

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+226 lines, -41 lines)			Patch
A	LayoutTests/fast/css/sticky/sticky-top-overflow.html	View	1 2	1 chunk	+83 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/sticky/sticky-top-overflow-expected.html	View	1 2	1 chunk	+67 lines, -0 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBoxModelObject.h	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBoxModelObject.cpp	View	1 2 3	2 chunks	+24 lines, -8 lines	0 comments	Download
M	Source/core/rendering/RenderLayer.h	View	1 2 3	3 chunks	+7 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderLayer.cpp	View	1 2 3	6 chunks	+20 lines, -8 lines	0 comments	Download
M	Source/core/rendering/RenderLayerCompositor.cpp	View	1 2	5 chunks	+23 lines, -19 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Julien - ping for review

Ojan should comment on the new behavior. https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/sticky-top-overflow.html File LayoutTests/fast/css/sticky/sticky-top-overflow.html (right): https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/sticky-top-overflow.html#newcode37 LayoutTests/fast/css/sticky/sticky-top-overflow.html:37: position: -webkit-sticky; ...

6 years, 11 months ago (2014-01-06 17:22:07 UTC) #2

shawnsingh

https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/RenderLayerCompositor.cpp File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/RenderLayerCompositor.cpp#newcode1809 Source/core/rendering/RenderLayerCompositor.cpp:1809: if (stackingContainer->layer()->compositingState() != NotComposited On 2014/01/06 17:22:07, Julien Chaffraix ...

6 years, 11 months ago (2014-01-06 20:02:47 UTC) #4

ostap

https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/sticky-top-overflow.html File LayoutTests/fast/css/sticky/sticky-top-overflow.html (right): https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/sticky-top-overflow.html#newcode37 LayoutTests/fast/css/sticky/sticky-top-overflow.html:37: position: -webkit-sticky; On 2014/01/06 17:22:07, Julien Chaffraix - CET ...

6 years, 11 months ago (2014-01-08 01:46:39 UTC) #5

https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/...
File LayoutTests/fast/css/sticky/sticky-top-overflow.html (right):

https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/...
LayoutTests/fast/css/sticky/sticky-top-overflow.html:37: position:
-webkit-sticky;
On 2014/01/06 17:22:07, Julien Chaffraix - CET wrote:
> This is unprefixed in Blink (how did the test pass locally?) so it should be
> position: sticky.

Forgot to stage LayoutTests changes.
Done

https://codereview.chromium.org/103213002/diff/1/LayoutTests/fast/css/sticky/...
LayoutTests/fast/css/sticky/sticky-top-overflow.html:57: <body>
On 2014/01/06 17:22:07, Julien Chaffraix - CET wrote:
> Usually tests should state what they expect for passing and/or failing. Here
it
> seems like the passing condition is: There should be no red.
> 
> Also having a description of what is being tested (aka a description) is
useful,
> e.g. here: This test checks that sticky positioned elements are contained by
> their enclosing ancestor with an overflow clip.

Done.

https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.h:229: RenderLayer*
enclosingOverflowClipLayer(bool includeSelf = true) const;
On 2014/01/06 17:22:07, Julien Chaffraix - CET wrote:
> Why didn't we merge the IncludeSelfOrNot enum too? This makes the code more
> readable.

Done.

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

https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerCompositor.cpp:1809: if
(stackingContainer->layer()->compositingState() != NotComposited
On 2014/01/06 17:22:07, Julien Chaffraix - CET wrote:
> The old code was checking
> stackingContainer->layer()->hasCompositedLayerMapping(), I think this check
> should be kept instead of changed.

The original code in webkit patch calls stackingContainer->isComposited() .
Since in blink stacking container is separated from renderLayer and
isComposited() doesn't exist any more, I thought compositingState() check would
be valid replacement.

Julien - ping for review

lgtm with comments. https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/RenderLayerCompositor.cpp File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/103213002/diff/1/Source/core/rendering/RenderLayerCompositor.cpp#newcode1809 Source/core/rendering/RenderLayerCompositor.cpp:1809: if (stackingContainer->layer()->compositingState() != NotComposited On 2014/01/08 ...

6 years, 11 months ago (2014-01-08 15:31:45 UTC) #6

ostap

https://codereview.chromium.org/103213002/diff/60001/LayoutTests/fast/css/sticky/sticky-top-overflow-expected.html File LayoutTests/fast/css/sticky/sticky-top-overflow-expected.html (right): https://codereview.chromium.org/103213002/diff/60001/LayoutTests/fast/css/sticky/sticky-top-overflow-expected.html#newcode2 LayoutTests/fast/css/sticky/sticky-top-overflow-expected.html:2: On 2014/01/08 15:31:45, Julien Chaffraix - CET wrote: > ...

6 years, 11 months ago (2014-01-09 03:44:14 UTC) #7

commit-bot: I haz the power

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

6 years, 11 months ago (2014-01-09 03:44:33 UTC) #8

commit-bot: I haz the power

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

6 years, 11 months ago (2014-01-09 05:51:39 UTC) #9

commit-bot: I haz the power

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

6 years, 11 months ago (2014-01-09 21:06:52 UTC) #10

Message was sent while issue was closed.

Change committed as 164812

Expand Messages | Collapse Messages