Bug 90046

<rdar://problem/11508054>

Created attachment 153696 [details] Add ENABLE_CSS_STICKY_POSITION, turned off

Created attachment 153699 [details] Add support for parsing -webkit-sticky

Created attachment 153700 [details] Add ENABLE_CSS_STICKY_POSITION, turned off

Created attachment 153701 [details] Add support for parsing -webkit-sticky (depends on previous patch)

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 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 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 on attachment 153700 [details] Add ENABLE_CSS_STICKY_POSITION, turned off http://trac.webkit.org/changeset/123350

Comment on attachment 153701 [details] Add support for parsing -webkit-sticky (depends on previous patch) http://trac.webkit.org/changeset/123379

Created attachment 154208 [details] Patch

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 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 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 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

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 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 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 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 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.

(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."

> > 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.

Created attachment 160060 [details] Patch

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.

Created attachment 160217 [details] Some renames in prep for sticky

Comment on attachment 160217 [details] Some renames in prep for sticky r=me

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. :)

(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.

(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.

Created attachment 160291 [details] Patch

Comment on attachment 160217 [details] Some renames in prep for sticky This was landed in http://trac.webkit.org/changeset/126520

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 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.

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. :)

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.

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.

Created attachment 160533 [details] Patch

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 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?

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.

(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.

Followup for having left and right specified: bug 95146.