Bug 90046

Summary: Implement sticky positioning
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, cmarcelo, dglazkov, donggwan.kim, d-r, eoconnor, ericbidelman, eric, falken, gyuyoung.kim, hyatt, jchaffraix, jonlee, joone, jrogers, kenneth, kennyluck, macpherson, menard, mifenton, mihnea, mike, ojan, paulirish, peter, rakuco, simon.fraser, skyostil, thorton, tonikitoo, tony, webkit-bug-importer, webkit.review.bot, yael
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://lists.w3.org/Archives/Public/www-style/2012Jun/0627.html
Bug Depends on: 90045    
Bug Blocks: 92063    
Attachments:
Description Flags
Add ENABLE_CSS_STICKY_POSITION, turned off
none
Add support for parsing -webkit-sticky
none
Add ENABLE_CSS_STICKY_POSITION, turned off
none
Add support for parsing -webkit-sticky (depends on previous patch)
none
Patch
none
Archive of layout-test-results from gce-cr-linux-07
none
Patch
none
Some renames in prep for sticky
none
Patch
none
Patch ojan: review+

Description Simon Fraser (smfr) 2012-06-26 22:53:03 PDT
We should experiment with sticky positioning, as proposed here:
<http://lists.w3.org/Archives/Public/www-style/2012Jun/0627.html>
Comment 1 Simon Fraser (smfr) 2012-06-26 22:59:38 PDT
<rdar://problem/11508054>
Comment 2 Simon Fraser (smfr) 2012-07-22 10:04:30 PDT
Created attachment 153696 [details]
Add ENABLE_CSS_STICKY_POSITION, turned off
Comment 3 Simon Fraser (smfr) 2012-07-22 11:51:59 PDT
Created attachment 153699 [details]
Add support for parsing -webkit-sticky
Comment 4 Simon Fraser (smfr) 2012-07-22 12:09:16 PDT
Created attachment 153700 [details]
Add ENABLE_CSS_STICKY_POSITION, turned off
Comment 5 Simon Fraser (smfr) 2012-07-22 12:11:18 PDT
Created attachment 153701 [details]
Add support for parsing -webkit-sticky (depends on previous patch)
Comment 6 Gyuyoung Kim 2012-07-23 00:29:01 PDT
Comment on attachment 153700 [details]
Add ENABLE_CSS_STICKY_POSITION, turned off

View in context: https://bugs.webkit.org/attachment.cgi?id=153700&action=review

> Source/cmake/WebKitFeatures.cmake:32
> +    WEBKIT_OPTION_DEFINE(ENABLE_CSS_STICKY_POSITION "Toggle CSS sticky position support" OFF)

Could you add this feature macro below file as well ?

http://trac.webkit.org/browser/trunk/Source/cmakeconfig.h.cmake#L23
Comment 7 Ojan Vafai 2012-07-23 07:41:01 PDT
Comment on attachment 153700 [details]
Add ENABLE_CSS_STICKY_POSITION, turned off

View in context: https://bugs.webkit.org/attachment.cgi?id=153700&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Implement sticky positioning

Might want to use a different bug and leave this as a meta bug for sticky positioning patches. With this line, it sounds like this patch does a lot more than it does. :)
Comment 8 Ojan Vafai 2012-07-23 07:49:30 PDT
Comment on attachment 153701 [details]
Add support for parsing -webkit-sticky (depends on previous patch)

View in context: https://bugs.webkit.org/attachment.cgi?id=153701&action=review

Typically we do one patch per bug, no?

Also, you're ChangeLog files are all messed up (out of order + OOPs lines).

The change itself looks good. :)

> LayoutTests/fast/css/sticky/resources/parsing-position-sticky.js:1
> +description("This tests checks that the -webkit-sticky value for position parses correctly.");

Can we inline this into parsing-position-sticky.html? It makes sense to have external JS files for tests that we want to run as pure-JS (e.g. in JSC), but for these sorts of tests it just adds overhead to maintaining the test to have it in a separate file.
Comment 9 Simon Fraser (smfr) 2012-07-23 10:46:14 PDT
Comment on attachment 153700 [details]
Add ENABLE_CSS_STICKY_POSITION, turned off

http://trac.webkit.org/changeset/123350
Comment 10 Simon Fraser (smfr) 2012-07-23 14:04:23 PDT
Comment on attachment 153701 [details]
Add support for parsing -webkit-sticky (depends on previous patch)

http://trac.webkit.org/changeset/123379
Comment 11 Simon Fraser (smfr) 2012-07-24 18:28:14 PDT
Created attachment 154208 [details]
Patch
Comment 12 Simon Fraser (smfr) 2012-07-24 18:30:09 PDT
Things I don't yet handle:
* sticky to some overflow container
* sticky element overflowing its containing block
* some relative-inline-with-abs-child stuff
Comment 13 Tim Horton 2012-07-24 18:38:21 PDT
Comment on attachment 154208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154208&action=review

I'll be a manual spell checker instead of something more useful, since this patch is large.

> Source/WebCore/ChangeLog:8
> +        Initial implementation of position: -webit-sticky, which 

s/webit/webkit

> Source/WebCore/ChangeLog:9
> +        constraints an element to be positioned inside the interection

s/constraints/constrains/, s/interection/intersection/

> Source/WebCore/rendering/RenderBox.cpp:1515
> +            // FIXME: why doesn't this use isRelPositioned?

Probably shouldn't add a comment that uses the old name.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:560
> +    // Do the math in the cooridinate space of the containing block.

s/cooridinate/coordinate/

> Source/WebCore/rendering/RenderBoxModelObject.cpp:564
> +    // FIXME: no idea if these are right.

Uhh.
Comment 14 Tim Horton 2012-07-24 18:38:23 PDT
Comment on attachment 154208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154208&action=review

I'll be a manual spell checker instead of something more useful, since this patch is large.

> Source/WebCore/ChangeLog:8
> +        Initial implementation of position: -webit-sticky, which 

s/webit/webkit

> Source/WebCore/ChangeLog:9
> +        constraints an element to be positioned inside the interection

s/constraints/constrains/, s/interection/intersection/

> Source/WebCore/rendering/RenderBox.cpp:1515
> +            // FIXME: why doesn't this use isRelPositioned?

Probably shouldn't add a comment that uses the old name.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:560
> +    // Do the math in the cooridinate space of the containing block.

s/cooridinate/coordinate/

> Source/WebCore/rendering/RenderBoxModelObject.cpp:564
> +    // FIXME: no idea if these are right.

Uhh.
Comment 15 WebKit Review Bot 2012-07-24 20:38:52 PDT
Comment on attachment 154208 [details]
Patch

Attachment 154208 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13353065

New failing tests:
fast/loader/loadInProgress.html
fast/inline/positionedLifetime.html
fast/css/sticky/sticky-left-percentage.html
Comment 16 WebKit Review Bot 2012-07-24 20:39:01 PDT
Created attachment 154235 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 17 Tony Chang 2012-07-25 14:30:33 PDT
Comment on attachment 154208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154208&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:459
> +    setStickyPositioned(styleToUse->position() == StickyPosition);

Should we call setStickyPositioned(false) in RenderObject::styleWillChange? We reset the other position bits.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:568
> +    LayoutUnit marginLeftLength = minimumValueForLength(style()->marginLeft(), containingBlockLogicalWidthForContent(), view());
> +    LayoutUnit marginTopLength = minimumValueForLength(style()->marginTop(), containingBlock->availableLogicalHeight(), view());
> +    LayoutUnit marginRightLength = minimumValueForLength(style()->marginRight(), containingBlockLogicalWidthForContent(), view());
> +    LayoutUnit marginBottomLength = minimumValueForLength(style()->marginBottom(), containingBlock->availableLogicalHeight(), view());

It looks like this is mixing absolute directions with logical directions.  Should we be using absolute values for everything?  You could write a test case using a vertical writing mode to test.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:588
> +

Nit: Extra blank line?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:597
> +        if (stickyBoxRect.x() < containingBlockRect.x())
> +            stickyBoxRect.setX(containingBlockRect.x());

Is it possible that moving the box based on right: will cause us to break the rule above for left: (e.g., if the width of the containing block was too small)?  Seems OK, just wanted to make sure that the expected behavior is that right: overrides left: and bottom: overrides top:.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:608
> +

Nit: Extra blank line?

> Source/WebCore/rendering/RenderInline.cpp:1000
> +        if ((inlineFlow->style()->position() == RelativePosition || inlineFlow->style()->position() == StickyPosition) && inlineFlow->hasLayer())

Would it be worth adding positionRelativeOrSticky() to RenderStyle?

> Source/WebCore/rendering/RenderObject.h:1014
> +        ADD_BOOLEAN_BITFIELD(stickyPositioned, StickyPositioned);

Do we have to use a bit here can can isStickyPositioned() be implemented as style()->position() == StickyPosition?

> LayoutTests/ChangeLog:8
> +        Various ref tests for sticky positioning.

It would be nice to have some repaint tests for changing the display type to/from sticky and making sure we relayout and repaint properly.  Might be better as a follow up patch.
Comment 18 WebKit Review Bot 2012-07-27 05:02:18 PDT
Comment on attachment 153700 [details]
Add ENABLE_CSS_STICKY_POSITION, turned off

Cleared Ojan Vafai's review+ from obsolete attachment 153700 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 19 WebKit Review Bot 2012-07-27 05:02:26 PDT
Comment on attachment 153701 [details]
Add support for parsing -webkit-sticky (depends on previous patch)

Cleared Ojan Vafai's review+ from obsolete attachment 153701 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 20 Dave Hyatt 2012-07-27 09:09:44 PDT
Comment on attachment 154208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154208&action=review

Looks pretty good, but I am confused about the margin issue. What is keeping you from using the values on the RenderObject instead of trying to compute them yourself.

> Source/WebCore/rendering/RenderBox.cpp:1517
> +            if ((styleToUse->position() == RelativePosition || styleToUse->position() == StickyPosition) && layer())
> +                rect.move(layer()->offsetForInFlowPosition());

It does not use the RenderObject bits because this can be called when re-resolving style, before the bits have been set. Rather than making this a FIXME, you could just explain that.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:567
> +    LayoutUnit marginLeftLength = minimumValueForLength(style()->marginLeft(), containingBlockLogicalWidthForContent(), view());
> +    LayoutUnit marginTopLength = minimumValueForLength(style()->marginTop(), containingBlock->availableLogicalHeight(), view());
> +    LayoutUnit marginRightLength = minimumValueForLength(style()->marginRight(), containingBlockLogicalWidthForContent(), view());

As Tony mentioned, this isn't correct. You're missing physical and logical coordinate systems here. I'm a bit confused why you don't have your margins computed already? Why can't you just use the RenderBox accessors?

> Source/WebCore/rendering/RenderInline.cpp:173
> +void RenderInline::styleWillChange(StyleDifference diff, const RenderStyle* newStyle)
> +{
> +    if (FrameView *frameView = view()->frameView()) {
> +        RenderStyle* oldStyle = style();
> +        bool newStyleIsSticky = newStyle && newStyle->position() == StickyPosition;
> +        bool oldStyleIsSticky = oldStyle && oldStyle->position() == StickyPosition;
> +        if (newStyleIsSticky != oldStyleIsSticky) {
> +            if (newStyleIsSticky)
> +                frameView->addFixedObject(this);
> +            else
> +                frameView->removeFixedObject(this);
> +        }
> +    }
> +
> +    RenderBoxModelObject::styleWillChange(diff, newStyle);
> +}

Have you verified that the scrolling code that looks at fixed objects does the right thing with inlines? I'm suspicious as to whether it really does or not.

> Source/WebCore/rendering/RenderInline.cpp:1001
> +            repaintRect.move(toRenderInline(inlineFlow)->layer()->offsetForInFlowPosition());

Agree with Tony, but I would just use your in-flow terminology, e.g., inlineFlow->style()->isInFlowPositioned(). isOutOfFlowPositioned() would probably be good too, since I bet we ask absolute || fixed of the style also.

> Source/WebCore/rendering/RenderInline.cpp:1054
> +            if ((style()->position() == RelativePosition || style()->position() == StickyPosition) && layer())

Make a helper for this. isInFlowPositioned()

> Source/WebCore/rendering/RenderInline.cpp:1083
> +    if ((style()->position() == RelativePosition || style()->position() == StickyPosition) && layer()) {

Ditto.
Comment 21 Dave Hyatt 2012-07-27 09:10:48 PDT
(In reply to comment #20)
> (
> As Tony mentioned, this isn't correct. You're missing physical and logical coordinate systems here. I'm a bit confused why you don't have your margins computed already? Why can't you just use the RenderBox accessors?

I mean to say "mixing" not "missing."
Comment 22 Simon Fraser (smfr) 2012-08-15 16:24:40 PDT
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:567
> > +    LayoutUnit marginLeftLength = minimumValueForLength(style()->marginLeft(), containingBlockLogicalWidthForContent(), view());
> > +    LayoutUnit marginTopLength = minimumValueForLength(style()->marginTop(), containingBlock->availableLogicalHeight(), view());
> > +    LayoutUnit marginRightLength = minimumValueForLength(style()->marginRight(), containingBlockLogicalWidthForContent(), view());
> 
> As Tony mentioned, this isn't correct. You're missing physical and logical coordinate systems here. I'm a bit confused why you don't have your margins computed already? Why can't you just use the RenderBox accessors?

I can't use the margins computed on RenderBox because these are the minimum possible margins, not the ones we've computed and stored already.
Comment 23 Simon Fraser (smfr) 2012-08-22 18:32:12 PDT
Created attachment 160060 [details]
Patch
Comment 24 Ojan Vafai 2012-08-22 19:00:03 PDT
Comment on attachment 160060 [details]
Patch

Would you be willing to add the hasInFlowPosition and the renames of all the relative positioning stuff (e.g. hasRelPositionedInlineAncestor-->inFlowPositionedInlineAncestor) in a separate patch? That patch would be super easy to review and would make the actual logic changes in this patch a lot easier to review.
Comment 25 Simon Fraser (smfr) 2012-08-23 12:43:57 PDT
Created attachment 160217 [details]
Some renames in prep for sticky
Comment 26 Dave Hyatt 2012-08-23 12:45:36 PDT
Comment on attachment 160217 [details]
Some renames in prep for sticky

r=me
Comment 27 Ojan Vafai 2012-08-23 13:58:46 PDT
Comment on attachment 160217 [details]
Some renames in prep for sticky

View in context: https://bugs.webkit.org/attachment.cgi?id=160217&action=review

Thanks! I think this will make the final patch much easier to review.

> Source/WebCore/rendering/style/RenderStyle.h:963
> +    // Lines have horizontal orientation; modes horizontal-tb or horizontal-bt.
>      bool isHorizontalWritingMode() const { return writingMode() == TopToBottomWritingMode || writingMode() == BottomToTopWritingMode; }
> +    // Bottom of the line occurs earlier in the block; modes vertical-rl or horizontal-bt.
>      bool isFlippedLinesWritingMode() const { return writingMode() == LeftToRightWritingMode || writingMode() == BottomToTopWritingMode; }
> +    // Block progression increases in the opposite direction to normal; modes vertical-rl or horizontal-bt.

Nit: the part after the semi-colon in each of these comments is redundant with the actual code. Maciej would certainly complain that these are "what" comments. :)
Comment 28 Simon Fraser (smfr) 2012-08-23 14:24:12 PDT
(In reply to comment #27)
> (From update of attachment 160217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160217&action=review
> 
> Thanks! I think this will make the final patch much easier to review.
> 
> > Source/WebCore/rendering/style/RenderStyle.h:963
> > +    // Lines have horizontal orientation; modes horizontal-tb or horizontal-bt.
> >      bool isHorizontalWritingMode() const { return writingMode() == TopToBottomWritingMode || writingMode() == BottomToTopWritingMode; }
> > +    // Bottom of the line occurs earlier in the block; modes vertical-rl or horizontal-bt.
> >      bool isFlippedLinesWritingMode() const { return writingMode() == LeftToRightWritingMode || writingMode() == BottomToTopWritingMode; }
> > +    // Block progression increases in the opposite direction to normal; modes vertical-rl or horizontal-bt.
> 
> Nit: the part after the semi-colon in each of these comments is redundant with the actual code. Maciej would certainly complain that these are "what" comments. :)

Not really; they are a reminder of the mapping between the CSS values and the methods, and helpful for people grepping for those property names.
Comment 29 Ojan Vafai 2012-08-23 14:39:10 PDT
(In reply to comment #28)
> Not really; they are a reminder of the mapping between the CSS values and the methods, and helpful for people grepping for those property names.

Regardless, it's just a nit, so feel free to commit either way.
Comment 30 Simon Fraser (smfr) 2012-08-23 17:32:12 PDT
Created attachment 160291 [details]
Patch
Comment 31 Simon Fraser (smfr) 2012-08-23 17:52:56 PDT
Comment on attachment 160217 [details]
Some renames in prep for sticky

This was landed in http://trac.webkit.org/changeset/126520
Comment 32 Dave Hyatt 2012-08-24 11:50:47 PDT
Comment on attachment 160291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160291&action=review

r=me

> Source/WebCore/rendering/RenderBoxModelObject.cpp:548
> +            if (offsetParent->isBox() && offsetParent->isBody() && !offsetParent->isInFlowPositioned() && !offsetParent->isOutOfFlowPositioned())

Maybe add an isPositioned method that includes both out of flow and in flow? Then you could just say && !offsetParent->isPositioned() here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:586
> +    // Horizontal postion.

Typo. "position"
Comment 33 Ojan Vafai 2012-08-24 12:04:37 PDT
Comment on attachment 160291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160291&action=review

Very excited to see this go in!

What should happen when a sticky positioned item is placed in another sticky positioned item? Mind adding a test or two for this?

What's the plan for getting this into an editor's draft spec? Is Edward working on that? Having concrete spec text might help get more concrete feedback from other vendors.

> Source/WebCore/ChangeLog:4
> +        Implement sticky positioning
> +        https://bugs.webkit.org/show_bug.cgi?id=90046

This description needs updating (e.g. has collectMatchingRulesForList) and claims to add isInFlowPositioned.

> Source/WebCore/page/FrameView.cpp:1505
> +        if (renderer->style()->position() != FixedPosition && renderer->style()->position() != StickyPosition)

There are 4 places where we check if something is fixed or sticky. I wonder if it makes sense to come up with a name for this and add an accessor to RenderStyle in the way that we already have hasOutOfFlowPosition and hasInFlowPosition.

hasViewportRelativePosition? hasViewportContainedPosition?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:563
> +    // Have to recompute the margins, because the already-computed margins include extra space.

Can you clarify this comment a bit? Extra space from what? Is this just in the case of margin: auto? If so, I think it'd be better to having something like the following:
LayoutUnit leftMargin = style()->marginLeft().auto() ? 0 : marginLeft();

Then it's clear that you're special casing auto margins and you don't even need this explanatory comment.

That said, it's not even clear to me why we'd want to do this. Wouldn't you want margin: auto to apply as normal?

Can you add a test-case that covers this?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:567
> +    LayoutUnit minLeftMargin = minimumValueForLength(style()->marginLeft(), containingBlock->availableWidth(), view());
> +    LayoutUnit minTopMargin = minimumValueForLength(style()->marginTop(), containingBlock->availableHeight(), view());
> +    LayoutUnit minRightMargin = minimumValueForLength(style()->marginRight(), containingBlock->availableWidth(), view());
> +    LayoutUnit minBottomMargin = minimumValueForLength(style()->marginBottom(), containingBlock->availableHeight(), view());

The available size of margins is always based off the logical width (see, for example, computeMargin in RenderInline.cpp). So, contrary to earlier comments, all four of these should be using containingBlock->availableLogicalWidth(). :)

> Source/WebCore/rendering/RenderBoxModelObject.cpp:581
> +    // We can't call localToAbsolute on |this| because that will recur. For now, assume that |this| is not transformed.

Should this be a FIXME to make transforms work on sticky positioned elements?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:593
> +        if (absoluteStickyBoxRect.maxX() > absContainerContentRect.maxX())
> +            absoluteStickyBoxRect.setX(absContainerContentRect.maxX() - absoluteStickyBoxRect.width());

This seems like it'd do the wrong thing if the position:sticky item is wider than its containing block? Actually, what is the right thing to do in that case? Mind adding a test-case that covers this? Ditto for the right/top/bottom cases below.
Comment 34 Ojan Vafai 2012-08-24 12:08:36 PDT
I'd be fine with just filing bugs to fix in follow-up patches for the nesting case and the sticky element being larger than its containing block case. My other comments seem simple enough that they should be easy to fix in this patch.

Or if you want to address it all in this patch, that's fine by me too. :)
Comment 35 Dave Hyatt 2012-08-24 12:16:46 PDT
NIce catch on the margins. availableLogicalWidth is indeed correct there, and the patch is wrong. That should be fixed before landing, so r- should stand.
Comment 36 Ojan Vafai 2012-08-24 12:38:25 PDT
Another case that it'd be good to have FIXME's, tests and a bug for is if you set both left and right or top and bottom. If this were to behave like position:fixed it should change the width/height of the sticky item. If this were to behave like position: relative, top/left would win over bottom/right.

Right now, it doesn't do either of those. Seems like it should be like position relative, since making it larger would affect it's inflow position, which is clearly wrong.
Comment 37 Simon Fraser (smfr) 2012-08-24 17:51:13 PDT
Created attachment 160533 [details]
Patch
Comment 38 Simon Fraser (smfr) 2012-08-24 17:55:20 PDT
Comment on attachment 160291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160291&action=review

>> Source/WebCore/page/FrameView.cpp:1505
>> +        if (renderer->style()->position() != FixedPosition && renderer->style()->position() != StickyPosition)
> 
> There are 4 places where we check if something is fixed or sticky. I wonder if it makes sense to come up with a name for this and add an accessor to RenderStyle in the way that we already have hasOutOfFlowPosition and hasInFlowPosition.
> 
> hasViewportRelativePosition? hasViewportContainedPosition?

Done (hasViewportConstrainedPosition).

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:548
>> +            if (offsetParent->isBox() && offsetParent->isBody() && !offsetParent->isInFlowPositioned() && !offsetParent->isOutOfFlowPositioned())
> 
> Maybe add an isPositioned method that includes both out of flow and in flow? Then you could just say && !offsetParent->isPositioned() here.

Done, and called it in a bunch of places.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:567
>> +    LayoutUnit minBottomMargin = minimumValueForLength(style()->marginBottom(), containingBlock->availableHeight(), view());
> 
> The available size of margins is always based off the logical width (see, for example, computeMargin in RenderInline.cpp). So, contrary to earlier comments, all four of these should be using containingBlock->availableLogicalWidth(). :)

Fixed.

I'm not sure what should happen with auto margins and sticky. For now. we'll treat auto as 0 and allow a sticky element to get pushed into its margin area. We may revise that. I added a test.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:581
>> +    // We can't call localToAbsolute on |this| because that will recur. For now, assume that |this| is not transformed.
> 
> Should this be a FIXME to make transforms work on sticky positioned elements?

Done, and filed bug 94992.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:586
>> +    // Horizontal postion.
> 
> Typo. "position"

Fixed

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:593
>> +            absoluteStickyBoxRect.setX(absContainerContentRect.maxX() - absoluteStickyBoxRect.width());
> 
> This seems like it'd do the wrong thing if the position:sticky item is wider than its containing block? Actually, what is the right thing to do in that case? Mind adding a test-case that covers this? Ditto for the right/top/bottom cases below.

Yes, that case isn't handled right now. Filed bug 94991.
Comment 39 Ojan Vafai 2012-08-24 18:10:04 PDT
Comment on attachment 160533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160533&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:563
> +    // Have to recompute the margins, because the already-computed margins include extra space.

This comment is still needlessly vague IMO since it's really just auto-margins that are being special handled here. I'd still prefer the code below to special case auto as I suggested in the previous review, but that's up to you.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:585
> +

Mind adding a FIXME that this should be reordered in order to match the position:relative behavior when both top and bottom or left and right are specified?

> Source/WebCore/rendering/RenderLayer.h:571
> +        HasSeenFixedOrStickyAncestor = 1 << 0,

HasSeenViewportConstrainedAncestor maybe?
Comment 40 Simon Fraser (smfr) 2012-08-27 10:40:01 PDT
http://trac.webkit.org/changeset/126774
Comment 41 Julien Chaffraix 2012-08-27 11:24:50 PDT
The test cases are failing on Chromium bots and some of the failures seem really fishy:

* Scrollbars appearing on the actual result:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss%2Fsticky%2Fsticky-side-margins.html

* Block being positioned differently on the actual result:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fcss%2Fsticky%2Fsticky-writing-mode-vertical-lr.html

I didn't mention some sub-pixel layout differences that are making some tests fail on top of the previous failures.
Comment 42 Simon Fraser (smfr) 2012-08-27 11:40:17 PDT
(In reply to comment #41)
> The test cases are failing on Chromium bots and some of the failures seem really fishy:
> 
> * Scrollbars appearing on the actual result:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss%2Fsticky%2Fsticky-side-margins.html

Fixed in r126778.

> * Block being positioned differently on the actual result:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fcss%2Fsticky%2Fsticky-writing-mode-vertical-lr.html

Will investigate.

> I didn't mention some sub-pixel layout differences that are making some tests fail on top of the previous failures.

Ick.
Comment 43 Simon Fraser (smfr) 2012-08-27 15:51:23 PDT
Followup for having left and right specified: bug 95146.