WebKit implements position:sticky, (with a -webkit- prefix, I believe), which is useful for a common pattern in mobile (and perhaps desktop) user interfaces, where headings are initially in the normal flow but then temporarily become position:fixed at the top of the scrollable area while the area they are a heading for is displayed. I think this is a relatively straightforward feature, and we should implement it. The initial proposal is: http://lists.w3.org/Archives/Public/www-style/2012Jun/0627.html (and the thread may have continued in that month and later months) Also see: http://lists.w3.org/Archives/Public/www-style/2013Mar/0176.html http://updates.html5rocks.com/2012/08/Stick-your-landings-position-sticky-lands-in-WebKit

Continuations of that thread include: http://lists.w3.org/Archives/Public/www-style/2012Jul/0009.html http://lists.w3.org/Archives/Public/www-style/2012Sep/0066.html This was also discussed in a CSSWG meeting in August: http://lists.w3.org/Archives/Public/www-style/2012Aug/0900.html Some other use cases include * sidebars, e.g. http://news.google.com/ * table header rows/columns To start, I'm working on writing a reasonably precise specification of how this should actually behave.

A handful of reftests for some of the basic behavior (sticky-top, with the element already at the top of its containing block). With -webkit- added, these match on WebKit nightly.

Added a few reftests for sticky-bottom positioning, which is an easier starting point because (a) the effect can be demonstrated without scrolling, and (b) the containing block's height doesn't need to factor into calculations.

I'm not too certain I'm really on the right track yet, but this is what I'm working with at the moment. At least it passes the sticky-bottom-{1..4} reftests. Some of the biggest outstanding questions (which dbaron and I can also discuss when we meet tomorrow): * since the position depends on scrolling (or in other words, on the relative positions of the containing block and scrolling container), it probably can't be handled (only) during reflow. * for top- and left-sticky elements, keeping them within the bottom/right edge of their containing block requires knowing the containing block's size. In fact, I think sticky position uniquely has the dependencies (element size => containing block size => element position), which might complicate how soon we're actually able to compute the position.

Comment on attachment 773700 [details] [diff] [review] Current progress Per our conversation just now -- probably worth landing the ApplyRelativePositioning refactor anytime. Also it's probably more useful to post separate patches rather than a squashed version of the patch queue. The approach so far seems fine -- though I didn't look too closely at the actual math. Maybe a few more rel-pos cases to catch; but the fun part is going to be (1) making sticky frames move back when their container scrolls (scrolling container needs a list in a frame property, probably and (2) dealing with the parts of our display list / layer system that do fancy things for fixed positioning.

Excerpted wisdom from mattwoodrow via IRC: <mattwoodrow> corey: So when we scroll, we just adjust the scrollable frame <mattwoodrow> and we won't reflow all the children <corey> yup, so I need to update the position somewhere else. <mattwoodrow> maybe have the scrollable frame have a list of sticky descendants? <corey> that sounds like a good start. <mattwoodrow> or maybe we have scroll event listeners you could use instead <mattwoodrow> corey: somewhere around here - http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1995 <corey> mattwoodrow: hmm, okay, thanks for the pointer. <mattwoodrow> corey: You can use AddScrollPositionListener to add your sticky frames to that mListeners array <mattwoodrow> our early async scrolling on android didn't know about position:fixed. They would scroll with your finger for a bit, and then jump back into place on each content update <mattwoodrow> but now we have that fixed, it wouldn't be hard to add the same annotations as we do for position:fixed with an extra data point about which scroll ranges to fix it for and which to scroll it <corey> mattwoodrow: so to start, it sounds like when I find the scroll container somewhere during frame construction/reflow, I should add the sticky frame (or some sort of listener) to its mListeners, which listener can then redo the position calculations? <mattwoodrow> corey: Yeah, that sounds right <corey> mattwoodrow: oh, and should I be worrying about layers and/or display lists for painting in any way? <corey> (though I probably have no idea what I'm talking about there) <mattwoodrow> corey: You shouldn't need to, no <mattwoodrow> until you get to async <mattwoodrow> we might need some display list changes to get the best performance I guess <mattwoodrow> but not correctness

Good idea, I can separate out this part of the patch. On try: https://tbpl.mozilla.org/?tree=Try&rev=21cbc17b0d24

Comment on attachment 774360 [details] [diff] [review] Refactor the application of CSS relative positioning. So this looks fine, except for the fact that you missed one case: search for PFD_RELATIVEPOS in nsLineLayout.cpp. It's not immediately obvious to me what should happen for that case -- it might not be all that easy to get an nsHTMLReflowState in to where the PFD_RELATIVEPOS bit is read. (So that might call for this being a static method instead...)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10) > So this looks fine, except for the fact that you missed one case: search > for PFD_RELATIVEPOS in nsLineLayout.cpp. You're right, I hadn't figured out what to do there. And I'm not sure that making it a static method but carrying along mComputedOffsets (and maybe position:sticky would need more?) is ideal. But then, I don't fully grasp what's going on in nsLineLayout. I also still need to modify nsCSSFrameConstructor::RecomputePosition. https://hg.mozilla.org/mozilla-central/file/3433a021847b/layout/base/nsCSSFrameConstructor.cpp#l12449

(In reply to Corey Ford [:corey] from comment #11) > I also still need to modify nsCSSFrameConstructor::RecomputePosition. > https://hg.mozilla.org/mozilla-central/file/3433a021847b/layout/base/ > nsCSSFrameConstructor.cpp#l12449 I wouldn't worry about that as part of this patch. RecomputePosition is a performance optimization to make dynamic changes to top/right/bottom/left fast. That's far less important for position:sticky, where I suspect dynamic changes to the offset properties will be quite rare. And it's not trivial to make the optimization work for sticky. So I would suggest: (a) don't worry about RecomputePosition as part of this refactoring (b) in a later position:sticky patch, just make RecomputePosition do a reflow and return false in the position:sticky case.

Oh, and what's going on in line layout is basically this: we reflow the frames, but we do vertical alignment substantially after reflow -- basically once we're done layout of the entire line. And we do relative positioning at the end of the vertical alignment process (essentially at the end of the layout process, so that we have the correct without-relative-position position). So in the PerFrameData, we cache the data we need to do the relative positioning, and then come back and use it at the end of laying out the entire line.

Comment on attachment 774360 [details] [diff] [review] Refactor the application of CSS relative positioning. Continuing work on this part in bug 893962.

This includes percentage offsets computed in terms of the scroll container. It doesn't take into account that the scroll container might be auto-height (or width), though I guess behavior in that case is open for debate. Looks like I also need to do more work to account for scrollbars.

This works reasonably well in a simple testcase of a top+bottom sticky element with an overflow:auto ancestor. It assumes my previous patches on this bug and my refactor from bug 894716. The sticky element currently disappears outside of a certain scrolling range, though I haven't been able to identify the boundaries. But highlighting the frame via the devtools shows its position is still as expected. Maybe the overflow areas aren't being updated properly, or something? Left/right should just be parallel logic to top/bottom. I realize I also still need to add in the sticky element's margins. Also, StickyScrollingContainer is a terrible (and wordy) name for this thing. Maybe just StickyManager?

This is what I've been using to manually test sticky positioning.

Comment on attachment 778212 [details] [diff] [review] Sticky positioning with nsStickyScrollingContainer object Review of attachment 778212 [details] [diff] [review]: ----------------------------------------------------------------- Looks basically good to me. The name sounds OK to me, although you can lose the "ns" prefix and put it in the mozilla namespace. ::: layout/generic/nsStickyScrollingContainer.cpp @@ +37,5 @@ > + nsIFrame* cbFrame = mFrame->GetContainingBlock(); > + nsRect scrollRect = scrollFrame->GetPaddingRect(); > + nsRect cbRect = cbFrame->GetContentRect(); > + nsRect rect = mFrame->GetRect(); > + nscoord temp; Move declarations of variables as far down as possible.

Comment on attachment 778154 [details] [diff] [review] Part 3: Compute sticky positioning offsets. >+ nsRect scrollContainerPaddingRect = scrollContainer->GetPaddingRect(); I *think* this is going to be the rect outside the scrollbar. I'd have guessed that you want the rect inside the scrollbar. (In other words, the scrollbar lives in the space between the border and padding. I agree you want a rect between the border and the padding. The question is whether you want the rect that includes the scrollbar (outside) or that doesn't include the scrollbar (inside).) I'm not sure about either statement, though; they should both be tested. Otherwise this looks fine to me.

Comment on attachment 778212 [details] [diff] [review] Sticky positioning with nsStickyScrollingContainer object What roc said in comment 18, plus: You should make nsStickyScrollingContainer MOZ_FINAL, and then make its destructor non-virtual. I think you should also make DestroyStickyScrollingContainer out-of-line rather than inline; I think it's better to avoid the risk of emitting it in multiple places. In the .h file: >+ nsIFrame *mFrame; >+ nsIScrollableFrame *mScrollFrame; maybe make these const given that they're untouched after the constructor? (nsIFrame * const, etc.) In the .cpp file: >+ /* TODO check for null mFrame? */ >+ /* TODO handle null mScrollFrame (when would this happen?) */ You shouldn't need to handle the first. I think the second might happen in a XUL document, but I'm not sure if the stuff that's necessary for that definition of XUL document is still accessible from Web content. >+ //mScrollPosition = mScrollFrame->GetScrollPosition(); You do need to initialize mScrollPosition somehow. The initial position may not be 0,0, particularly for RTL. (Though that leads to another question: do you want GetScrollPosition or GetLogicalScrollPosition?) I didn't check the math in ComputePosition since you said you're not looking for a detailed review. >+nsStickyScrollingContainer::~nsStickyScrollingContainer() { Opening brace of a function on its own line, please (throughout). >+ for (nsIFrame *f = mFrame->GetParent(); f != cbFrame; f = f->GetParent()) { >+ cbOffset += f->GetPosition(); This should use nsIFrame::GetOffsetTo rather than doing it manually. >+ nsStickyScrollingContainer* c = static_cast<nsStickyScrollingContainer*>( >+ aFrame->Properties().Get( >+ nsStickyScrollingContainer::StickyScrollingContainerProperty())); Maybe put a static inline helper for this in nsStickyScrollingContainer? (nsStickyScrollingContainer::StickyScrollingContainerForFrame(nsIFrame*)?) (And maybe the same for the chunk that's in nsFrame::Init. Better to have those details in one place rather than spread out.) I think it might make more sense to call "normal position" the "initial position" instead.

Thanks! (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #21) > I think you should also make DestroyStickyScrollingContainer out-of-line > rather > than inline; I think it's better to avoid the risk of emitting it in > multiple places. All the other frame property destructors I've found (mostly in nsIFrame.h) are inline. > >+ /* TODO handle null mScrollFrame (when would this happen?) */ > > You shouldn't need to handle the first. I think the second might happen > in a XUL document, but I'm not sure if the stuff that's necessary for > that definition of XUL document is still accessible from Web content. GetNearestScrollableFrame makes no explicit promise not to return null, though, so I'd feel better at least asserting in the constructor. > (Though that leads to another question: do you want GetScrollPosition > or GetLogicalScrollPosition?) I can't think of a reason to want a reversed x-axis here? > I think it might make more sense to call "normal position" the "initial > position" instead. "normal position" is sort of consistent with how CSS2.1 describes relative positioning (and unlike relative-offset elements, sticky positioned elements are actually sometimes in their "normal position"). I continue to contemplate having one StickyScrollingContainer per scroll container rather than per sticky element. That would be a little more complicated to set up, but would avoid multiple scroll-position and overflow-area updates in the (maybe not so uncommon) case of multiple sticky elements inside the same scrolling container. It would also make the StickyScrollingContainer name make sense ;)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > Move declarations of variables as far down as possible. In general, it looks like the points/rects are actually needed for each direction of positioning, so should stay declared outside of and before the if blocks. Though there might turn out to be a few only needed for bottom & right that I could declare halfway through.

Updated this part to use the scroll frame's content rect (as we've discussed), and to store the offsets directly in a frame property.

A similar update to these computations, plus a mochitest (currently in test_computed_style.html, but could go in its own file instead).

Rebased to account for nsStyleConsts.h move.

And another revision to include sticky in the property_database.js test file.

Removed the aDirection parameter -- since determining "overconstrained" positioning depends on sizes of the sticky element and scroll container, it will be handled in the main positioning algorithm.

Comment on attachment 786115 [details] [diff] [review] Part 1 v3: Support position:sticky in the CSS parser. This is good, but I think it would be better if you could create a part 0 patch for the pref that this feature will live behind, and then have this part 1 patch handle "sticky" being behind preffable from the start. Handling a preffable value (as opposed to a whole property) is a bit awkward, unfortunately. You can see how display:flex is switched on and off by FlexboxEnabledPrefChangeCallback in nsLayoutUtils.cpp.

Comment on attachment 770887 [details] [diff] [review] Part 2: Include sticky positioning in nsStyleDisplay::IsRelativelyPositionedStyle Review of attachment 770887 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 784130 [details] [diff] [review] Part 4 v2: Compute sticky positioning offsets for getComputedStyle(). Review of attachment 784130 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: layout/style/nsComputedDOMStyle.cpp @@ +4110,5 @@ > return true; > } > > bool > +nsComputedDOMStyle::GetScrollFramePaddingWidth(nscoord& aWidth) Should this be called GetScrollFrameContentWidth? ::: layout/style/nsComputedDOMStyle.h @@ +488,5 @@ > > bool GetCBContentWidth(nscoord& aWidth); > bool GetCBContentHeight(nscoord& aWidth); > + bool GetScrollFramePaddingWidth(nscoord& aWidth); > + bool GetScrollFramePaddingHeight(nscoord& aWidth); aHeight ::: layout/style/test/test_computed_style.html @@ +242,5 @@ > > p.parentNode.removeChild(p); > })(); > + > +(function test_bug_886646() { I think it'd be better to have this in a separate file. (Not sure why those existing, loosely related tests are all bundled up in the same file in the first place.) @@ +268,5 @@ > + p.style[prop] = offsets[prop] + "%"; > + is(cs[prop], offsets[prop] + "px"); > + } > + > + // ... even in the presence of scrollbars Could you also test with border/margin on the scroller, to make sure they're not included in the percentage computations?

I just duplicated the logic for the flexbox pref; as dholbert and I briefly discussed on IRC, this could be made more generic, but since the method is a bit fragile it would need to be used with caution. I'll see about adding reftests (to my sticky reftests patch) to make sure the preference works. (But I checked that my other reftests pass if and only if I enable the pref via reftest.list.)

Comment on attachment 787158 [details] [diff] [review] Part 1 v4: Support position:sticky in the CSS parser, enabled by a preference. >+++ b/layout/style/nsCSSProps.h >- static const int32_t kPositionKTable[]; >+ // Not const because we modify its entries when CSS prefs change. >+ static int32_t kPositionKTable[]; nit: s/CSS prefs change/the pref "layout.css.sticky.enabled" changes/ It's good to mention the pref there, so that when we eventually drop the "layout.css.sticky.enabled" pref altogether (and search for all the occurrences of that pref in our code), we'll find this line and realize that we can make this array const again. [aside: This applies to my own code-comment for kDisplayKTable, too -- I'll push a tweak to that comment in a few minutes.]

Renamed things, added border and margin to the testcase and split it into a separate file (actually two, like the flexbox tests). Could you take another quick look at how the test is set up now?

Comment on attachment 786532 [details] [diff] [review] Part 5 v2: Handle sticky positioning in RecomputePosition. This can wait, since it depends on bug 898794.

Comment on attachment 787158 [details] [diff] [review] Part 1 v4: Support position:sticky in the CSS parser, enabled by a preference. Review of attachment 787158 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Might be good to assert that nsCSSProps::FindIndexOfKeyword returns a non-negative number?

Comment on attachment 787197 [details] [diff] [review] Part 4 v3: Compute sticky positioning offsets for getComputedStyle(). This all looks good. r=me

Better comments per dholbert. (In reply to Cameron McCormack (:heycam) from comment #39) > Might be good to assert that nsCSSProps::FindIndexOfKeyword returns a non-negative number? The following code doesn't do anything if it returns a negative number, which seems even more robust.

(In reply to Corey Ford [:corey] from comment #41) > The following code doesn't do anything if it returns a negative number, > which seems even more robust. I don't mind the robustness, but I feel like it would be good to indicate to people reading the code that we expect FindIndexOfKeyword always to return a non-negative index. I'd say also that having the assertion would make it obvious that the table has been modified incorrectly, but we'd probably find reftest failures for position:sticky in that case anyway.

One nit on the reftests: best-practice for reference cases is to have them *not* require any scripting (I think to make them as predictable / simple as possible). (Also: I'm pretty sure you don't need "reftest-wait", since it looks like your scripted changes will all take place before we fire the "load" event. Technically, you only need reftest-wait when you want to make changes *after* load fires, IIRC.)

Comment on attachment 786530 [details] [diff] [review] Part 3 v3: Compute sticky positioning offsets. >diff --git a/layout/generic/nsHTMLReflowState.cpp b/layout/generic/nsHTMLReflowState.cpp >+/* static */ >+void nsHTMLReflowState::ComputeStickyOffsets(nsIFrame* aFrame) >+{ >+ nsIFrame* scrollContainer = nsLayoutUtils::GetNearestScrollableFrame( >+ aFrame->GetParent(), nsLayoutUtils::SCROLLABLE_SAME_DOC | >+ nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN)-> >+ GetScrolledFrame(); Two things: (1) AFAICT, GetNearestScrollableFrame is allowed to return null, so we need to null-check its result before dereferencing it. (And it looks like all the other callers have a null-check.) Alternately, if you're *sure* it can't return null here, then add a MOZ_ASSERT to that effect. Also: this style is a bit hard to read -- it's a bit odd (for mozilla style at least) to have the function-args indented ~25 characters to the left of where the function-name started. I think this would be cleaner & more style-conforming (with the flags shifted leftwards from where they'd normally be to stay under the 80-char limit): nsIScrollableFrame* scrollableFrame = nsLayoutUtils::GetNearestScrollableFrame(aFrame->GetParent(), nsLayoutUtils::SCROLLABLE_SAME_DOC | nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN); (...and then you can null-check scrollableFrame, and then invoke GetScrolledFrame() if it's non-null) >+ nsSize scrollContainerSize = scrollContainer-> >+ GetContentRectRelativeToSelf().Size(); style nit: only add a newline after a "->" if you have to, to keep things under 80 characters. There's no pressing need for it here -- this fits just fine: nsSize scrollContainerSize = scrollContainer->GetContentRectRelativeToSelf().Size(); >+ if (eStyleUnit_Auto == position->mOffset.GetLeftUnit()) { >+ computedOffsets.left = NS_AUTOOFFSET; >+ } else { >+ computedOffsets.left = nsLayoutUtils:: >+ ComputeCBDependentValue(scrollContainerSize.width, >+ position->mOffset.GetLeft()); >+ } We've got four mostly-identical copies of this code (one copy for each side). Could we factor it out into a single function-call (maybe defined as a static helper-function right above this one)? (You can use "eSideLeft", "eSideRight", etc. as arguments to mOffset::GetUnit() / Get(), to avoid the need to specify "GetLeftUnit" as well as "GetLeft" -- that removes some of the redundancy.) I'm imagining something like: computedOffsets.left = ResolveStickyOffset(position->mOffset, eSideLeft, scrollContainerSize.width); computedOffsets.right = ResolveStickyOffset(position->mOffset, eSideRight, scrollContainerSize.width); If you need to make ResolveStickyOffset call ComputeCBDependentValue for horizontal vs. ComputeHeightDependentValue for vertical, you can just condition that off of the "side" argument. BUT: do you actually need to distinguish between ComputeCBDependentValue and ComputeHeightDependentValue? It looks to me like you could just use ComputeCBDependentValue() (but maybe I'm missing something). (It looks like there's a separation in the existing rel-pos code -- that appears to be because ComputeCBDependentValue used to be "ComputeWidthDependentValue", but I generified it in the first patch on bug 851379, and it should now be axis-independent. So you should be able to just use ComputeCBDependentValue regardless here, I think.) >+ // Store the offset >+ FrameProperties props = aFrame->Properties(); >+ nsMargin* offsets = static_cast<nsMargin*> >+ (props.Get(nsIFrame::ComputedStickyOffsetProperty())); >+ if (offsets) { >+ *offsets = computedOffsets; >+ } else { >+ props.Set(nsIFrame::ComputedStickyOffsetProperty(), >+ new nsMargin(computedOffsets)); >+ } >+} >+ >@@ -1944,18 +2002,22 @@ nsHTMLReflowState::InitConstraints(nsPre > if (mStyleDisplay->IsRelativelyPositioned(frame)) { > uint8_t direction = NS_STYLE_DIRECTION_LTR; > if (cbrs && NS_STYLE_DIRECTION_RTL == cbrs->mStyleVisibility->mDirection) { > direction = NS_STYLE_DIRECTION_RTL; > } >- ComputeRelativeOffsets(direction, frame, aContainingBlockWidth, >- aContainingBlockHeight, mComputedOffsets); >+ if (NS_STYLE_POSITION_STICKY == mStyleDisplay->mPosition) { >+ ComputeStickyOffsets(frame); >+ } else { >+ ComputeRelativeOffsets(direction, frame, aContainingBlockWidth, >+ aContainingBlockHeight, mComputedOffsets); >+ } This could be a bit cleaner. The current patch makes us do a redundant check for mStyleDisplay == NS_STYLE_POSITION_STICKY (one in the IsRelativelyPositioned call, and then another explicit check), which is undesirable. Also, it sets up |direction| when it's not needed, in the sticky case. I'd prefer that you replace the old IsRelativelyPositioned check with an explicit if (NS_STYLE_POSITION_RELATIVE == mStyleDisplay->mPosition) { ...and then add an "else if" for sticky, before the final "else" clause that sets the offsets to 0. ALSO: I think your Patch 5 (RecomputePosition) conceptually might want to be folded into this patch. In my mind, this patch is about adding ComputeStickyOffsets and calling it in the right places, but right now it just hits one of those places & not the other, and the split seems arbitrary.

(Sorry, I forgot to delete that "// Store the offset" chunk before posting; no comments on that part.)

Here's a MOZ_ASSERT on the result on FindIndexOfKeyword.

Rebased onto this morning's m-c (a small context change), and renumbering this patch more sensibly (from 4 to 3).

Comment on attachment 787891 [details] [diff] [review] Part 3 v4: Compute sticky positioning offsets for getComputedStyle(). Keeping heycam's r+ from "Part 4 v3".

Renumbering and folding, so this contains changes from the patches formerly known as Part 3 and Part 5. (In reply to Daniel Holbert [:dholbert] from comment #45) > Comment on attachment 786530 [details] [diff] [review] > Part 3 v3: Compute sticky positioning offsets. > > I think this would be cleaner & more style-conforming (with the flags > shifted leftwards from where they'd normally be to stay under the 80-char > limit): > nsIScrollableFrame* scrollableFrame = > nsLayoutUtils::GetNearestScrollableFrame(aFrame->GetParent(), > nsLayoutUtils::SCROLLABLE_SAME_DOC | > nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN); Much better, thanks. > We've got four mostly-identical copies of this code (one copy for each side). > > Could we factor it out into a single function-call (maybe defined as a > static helper-function right above this one)? Done (and bug 903173 filed to get rid of ComputeHeightDependentValue altogether). > >@@ -1944,18 +2002,22 @@ nsHTMLReflowState::InitConstraints(nsPre > > if (mStyleDisplay->IsRelativelyPositioned(frame)) { > > uint8_t direction = NS_STYLE_DIRECTION_LTR; > > if (cbrs && NS_STYLE_DIRECTION_RTL == cbrs->mStyleVisibility->mDirection) { > > direction = NS_STYLE_DIRECTION_RTL; > > } > >- ComputeRelativeOffsets(direction, frame, aContainingBlockWidth, > >- aContainingBlockHeight, mComputedOffsets); > >+ if (NS_STYLE_POSITION_STICKY == mStyleDisplay->mPosition) { > >+ ComputeStickyOffsets(frame); > >+ } else { > >+ ComputeRelativeOffsets(direction, frame, aContainingBlockWidth, > >+ aContainingBlockHeight, mComputedOffsets); > >+ } > > This could be a bit cleaner. The current patch makes us do a redundant check > for mStyleDisplay == NS_STYLE_POSITION_STICKY (one in the > IsRelativelyPositioned call, and then another explicit check), which is > undesirable. Also, it sets up |direction| when it's not needed, in the > sticky case. Yeah, that structure was left over from when I thought I would use |direction| in ComputeStickyOffsets. > ALSO: I think your Patch 5 (RecomputePosition) conceptually might want to be > folded into this patch. In my mind, this patch is about adding > ComputeStickyOffsets and calling it in the right places, but right now it > just hits one of those places & not the other, and the split seems arbitrary. Good idea, folded it in here. Now that GetNormalPosition is settled, that part is ready for review (and there, I think there's enough common logic between relative/sticky to justify the control flow).

Renumbering this one too (from 6).

Here's (finally) the main patch that actually makes cool things happen, using a StickyScrollContainer object attached to each scrollable frame that has sticky descendants.

And here's a pile of tests, primarily for the code in part 6.

The code style and null checks from ComputeStickyOffsets are relevant here too.

Comment on attachment 787896 [details] [diff] [review] Part 4 v4: Compute sticky positioning offsets. >+++ b/layout/base/RestyleManager.cpp >+ if (display->mPosition == NS_STYLE_POSITION_STICKY) { >+ nsHTMLReflowState::ComputeStickyOffsets(aFrame); >+ } else { >+ const nsSize size = cb->GetContentRectRelativeToSelf().Size(); Might be worth asserting (or at least commenting) to indicate that display->mPosition == NS_STYLE_POSITION_RELATIVE in that "else" clause, both for clarity and also to help us in the future if we add a new "IsRelativelyPositioned()"-flavored position-type but forget to check for it in this code. >+ nsPoint position = aFrame->GetNormalPosition(); > nsHTMLReflowState::ApplyRelativePositioning(aFrame, newOffsets, &position); > aFrame->SetPosition(position); So, in a later patch, ApplyRelativePositioning is now going to handle both "relative" and "sticky", but its name & arguments still seem very much geared towards "relative" (its name and its |newOffsets| param, in particular)... Could you add a comment here to reassure the reader that it handles *both* position:relative *and* position:sticky? (I wonder if it'd be worth changing the signature, too, so that we don't have to bother with the unnecessary "newOffsets" in the sticky case? Not a big deal, & probably a more relevant comment for the patch that tweaks ApplyRelativePositioning, anyway.) >+static nscoord ComputeStickySideOffset(mozilla::css::Side aSide, >+ const nsStyleSides& aOffset, >+ nscoord aPercentBasis) Add "using namespace mozilla::css" at the top of this file, so that you can just directly refer to "Side" without the prefix, and so that (in ComputeStickyOffsets) you can pass in "eSideTop" etc. without the prefix. >+/* static */ void >+nsHTMLReflowState::ComputeStickyOffsets(nsIFrame* aFrame) >+{ >+ nsIScrollableFrame* scrollableFrame = >+ nsLayoutUtils::GetNearestScrollableFrame(aFrame->GetParent(), >+ nsLayoutUtils::SCROLLABLE_SAME_DOC | >+ nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN); >+ >+ if (!scrollableFrame) { >+ // Not sure this would happen, but bail if it does. >+ return; >+ } s/Not sure/Not sure how/ (?) Also: It'd probably be worth putting a NS_ERROR in that early-return clause, so that we'll find out if & how it can happen. :) (Once Jesse sics his fuzzers on position:sticky.) If it turns out it's easy (and presumably not-scary) to trigger that NS_ERROR, we can always remove it or soften it to a warning later on, and update your "// Not sure" comment with whatever knowledge we've gained from seeing a testcase.

Comment on attachment 787908 [details] [diff] [review] Part 7: Reftests for sticky positioning. Looks like you've got lots of tests here - that's great! I mostly just noticed minor stuff. My comments: You need to update layout/reftests/reftest.list to point to sticky-pos/reftest.list -- otherwise, your reftests won't get tested, during a full reftest urn. Also, it looks like your patch is missing these reference cases: bottom-3-ref.html bottom-4-ref.html (You probably forgot to 'hg add' them) Also, you have a few tests that don't have a "-1" suffix. (e.g. "initial.html") I think the general rule is to prefer using a "-1" suffix, even if there's only one test, so that we can trivially add variants down the line if we end up needing to. (That should be fixable by opening your patch file in an editor and doing a search-and-replace on the file names.) Also: when you're adding a suite of feature-tests like this, it's great if you can add a short (one or two line) comment at the top of each test file (not as important for the -ref files), briefly describing the scenario that you're trying to test. (after the publicdomain comment) A reviewer / test-reader can figure this out from reading the test's HTML/JS, but it's easier to just briefly read the comment, and then skim the test to sanity-check that it makes sense, rather than trying to reverse-engineer the intent based off of the filename and the test content. If you don't end up getting to adding these comments, it's probably not a huge deal, but ideally it'd be nice. >diff --git a/layout/reftests/sticky-pos/left-right-1-ref.html b/layout/reftests/sticky-pos/left-right-1-ref.html >+ <div id="scroll"> >+ <div class="fill"></div >+ ><div id="sticky"></div >+ ><div class="fill"> >+ </div> Nit: you're missing the closing tag for <div class="fill"> here. (This goes for most of the "left-right" test files, overconstrained-* test files, and probably others) >diff --git a/layout/reftests/sticky-pos/overcontain-ref.html b/layout/reftests/sticky-pos/overcontain-ref.html >new file mode 100644 I don't understand what "overcontain" means in the filename here. (A comment in the testcase would probably help elucidate that) >+++ b/layout/reftests/sticky-pos/scrollframe-reflow-1.html >+ #sticky { >+ position: -webkit-sticky; >+ position: sticky; This is the only test where you've got the -webkit prefixed version. Probably remove it? (unless you want to add it in all the other testcases, too)

(In reply to Daniel Holbert [:dholbert] from comment #56) > Also, it looks like your patch is missing these reference cases: > bottom-3-ref.html > bottom-4-ref.html > (You probably forgot to 'hg add' them) Actually, bottom-{2,3,4} should all render identically, so I have > == bottom-2.html bottom-2-ref.html > == bottom-3.html bottom-2-ref.html > == bottom-4.html bottom-2-ref.html There might be a clearer way to name the files, though.

Oh, sorry - I clearly didn't read closely enough. :) In cases like that, I like to name the testcases bottom-2a.html, bottom-2b.html, etc. and then name the reference bottom-2-ref.html (That way, you can infer what the reference is, without needing to read the manifest.)

Updated per dholbert's comments. (Thanks!) The test file comments will hopefully make review easier.

Comment on attachment 787905 [details] [diff] [review] Part 6: Implement sticky positioning, calculated on reflow and scroll. Review of attachment 787905 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/StickyScrollContainer.cpp @@ +41,5 @@ > +StickyScrollContainer::StickyScrollContainerForFrame(nsIFrame* aFrame) > +{ > + nsIScrollableFrame* scrollFrame = nsLayoutUtils:: > + GetNearestScrollableFrame(aFrame, nsLayoutUtils::SCROLLABLE_SAME_DOC | > + nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN); Oops, this should use aFrame->GetParent() in case the sticky frame itself is scrollable. (Glad I had the ancestor assertion later on to catch this.) I'll post an update to this patch and to the nsComputedDOMStyle patch (but it appears I did this correctly in ComputeStickyOffsets)

Fixed the GetNearestScrollableFrame calls, and updated the test to catch that error.

Fixed the GetNearestScrollableFrame call and rebased on this morning's m-c.

Comment on attachment 787901 [details] [diff] [review] Part 5 v2: Always build a stacking context for sticky positioned elements. The code looks fine to me (although maybe mattwoodrow is a better reviewer of this?); I'm a little curious about the motivation. Does it match WebKit? (I'd say we wouldn't want to do this if it didn't match WebKit; I'm not sure that matching WebKit is alone a reason to do this.) Has there been discussion of it on www-style? Is it described in your spec draft at https://etherpad.mozilla.org/yqbijwrHI6 ? (It might be worth getting that onto http://wiki.csswg.org/ , actually.)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #63) > I'm a little curious about the motivation. Does it match WebKit? > (I'd say we wouldn't want to do this if it didn't match WebKit; I'm not sure > that matching WebKit is alone a reason to do this.) Has there been > discussion of it on www-style? IIRC this was mainly roc's idea (care to elaborate?); I mentioned it at [1]. According to [2], "Compare position:fixed to the new position:sticky attribute: for reference, position:sticky always creates a new stacking context." WebKit nightly passes my stacking-context reftest (with -webkit- added, of course). 1: http://lists.w3.org/Archives/Public/www-style/2013Jul/0359.html 2: http://updates.html5rocks.com/2012/09/Stacking-Changes-Coming-to-position-fixed-elements

So the patch from bug 883568 causes an ###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /Users/cford/code/mozilla-central/layout/base/nsPresShell.cpp, line 2459 in my scrollframe-reflow-1.html test. What I think is happening is: 1. JS changes the overflow:hidden frame's height 2. We start reflow from the root scroll frame 3. We reflow the overflow:hidden frame, calling UpdateSticky at the end 4. The sticky frame gets added to my OverflowChangedTracker, which is then Flush()'d 5. OverflowChangedTracker now walks all the way up to the root scroll frame 6. We call UpdateOverflow on the root scroll frame, which NeedsReflow's the root scroll frame 7. But we're still not done with the reflow started in step 2.

(In reply to Corey Ford [:corey] from comment #64) > IIRC this was mainly roc's idea (care to elaborate?); I mentioned it at [1]. > According to [2], "Compare position:fixed to the new position:sticky > attribute: for reference, position:sticky always creates a new stacking > context." WebKit nightly passes my stacking-context reftest (with -webkit- > added, of course). Creating a new stacking context for position:sticky elements ensures that all the contents can be placed in a single layer, which simplifies integration with APZC and the layer system.

(In reply to Corey Ford [:corey] from comment #65) > So the patch from bug 883568 causes an > > ###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', > file /Users/cford/code/mozilla-central/layout/base/nsPresShell.cpp, line 2459 > > in my scrollframe-reflow-1.html test. What I think is happening is: > 1. JS changes the overflow:hidden frame's height > 2. We start reflow from the root scroll frame > 3. We reflow the overflow:hidden frame, calling UpdateSticky at the end > 4. The sticky frame gets added to my OverflowChangedTracker, which is then > Flush()'d > 5. OverflowChangedTracker now walks all the way up to the root scroll frame > 6. We call UpdateOverflow on the root scroll frame, which NeedsReflow's the > root scroll frame > 7. But we're still not done with the reflow started in step 2. It sounds like you'll need to extend OverflowChangedTracker to let you specify a "subtree root" frame where it stops calling UpdateOverflow.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #67) > It sounds like you'll need to extend OverflowChangedTracker to let you > specify a "subtree root" frame where it stops calling UpdateOverflow. But not because we know overflow areas outside of the scroll frame can't change, right? (If they didn't change, OverflowChangedTracker would stop walking up...) Rather because we know in this situation that the overflow areas of the scroll frame's ancestors will be updated soon enough anyway?

Or maybe I should instead be scheduling a nsChangeHint_UpdateOverflow restyle (though I guess it's not really a restyle happening here).

Added OverflowChangedTracker::mSubtreeRoot so that my overflow updates only walk up as far as necessary.

Comment on attachment 789839 [details] [diff] [review] Part 6 v3: Implement sticky positioning, calculated on reflow and scroll. Here are comments so far; I've looked at everything so far except for going through all of the code in StickyScrollContainer.cpp from StickyScrollContainer::ComputePosition to the end. I'll try to get to that either later this evening or tomorrow. > /** >+ * Set the subtree root to limit overflow updates. >+ */ >+ void SetSubtreeRoot(nsIFrame* aSubtreeRoot) { >+ mSubtreeRoot = aSubtreeRoot; >+ } I think this needs to be a little more dramatic. In particular, it's only ok to call this when we're *currently reflowing* aSubtreeRoot; otherwise it could cause overflow changes to fail to propagate. (And it's also required if we're currently reflowing aSubtreeRoot.) I also wonder if you really want to call UpdateOverflow on the subtree root itself. Maybe you do, though? I guess I'll see later in the patch. I tend to think mOverflowChangedTracker shouldn't be a member variable of StickyScrollContainer, but instead a local variable in UpdatePositions. It seems like data that we don't need to keep around. Maybe I'm missing a good reason, though? nsHTMLReflowState: > if (NS_STYLE_POSITION_RELATIVE == display->mPosition) { > *aPosition += nsPoint(aComputedOffsets.left, aComputedOffsets.top); > } >+ if (NS_STYLE_POSITION_STICKY == display->mPosition) { >+ *aPosition = StickyScrollContainer::StickyScrollContainerForFrame(aFrame)-> >+ ComputePosition(aFrame); >+ } Make this an "} else if (...) {" for clarity (and to make it less likely somebody puts something else in-between). nsGfxScrollFrame: >+nsGfxScrollFrameInner::UpdateSticky() >+{ >+ StickyScrollContainer* s = StickyScrollContainer:: >+ StickyScrollContainerForScrollFrame(mOuter); "s" seems unnecessarily brief. Maybe sc or ssc? StickyScrollContainer.h: >+ /** >+ * Find the StickyScrollContainer associated with the scroll container of >+ * the given frame, creating it if necessary. >+ */ >+ static StickyScrollContainer* StickyScrollContainerForFrame(nsIFrame* aFrame); >+ static StickyScrollContainer* StickyScrollContainerForScrollFrame(nsIFrame* aFrame); So only the first of these creates if necessary. That's actually sensible, but you should document it. And probably also rename the argument of the second to aScrollFrame, and put "Get" at the beginning of the name of the second to indicate it might return null. I'm inclined to say you should make the destructor private as well, and make DestroyStickyScrollContainer a friend, just for clarity. Also, use private rather than protected in a MOZ_FINAL class. nsStickyScrollContainer.cpp: StickyScrollContainer::UpdatePositions should probably assert that aSubtreeRoot == do_QueryFrame(mScrollFrame) (though that might need a cast) You should definitely define STICKY_DEBUG to no-op for landing. Maybe even remove it if you don't think it'll be useful in the future.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #71) > Comment on attachment 789839 [details] [diff] [review] > Part 6 v3: Implement sticky positioning, calculated on reflow and scroll. > > > /** > >+ * Set the subtree root to limit overflow updates. > >+ */ > >+ void SetSubtreeRoot(nsIFrame* aSubtreeRoot) { > >+ mSubtreeRoot = aSubtreeRoot; > >+ } > > I think this needs to be a little more dramatic. In particular, it's > only ok to call this when we're *currently reflowing* aSubtreeRoot; > otherwise it could cause overflow changes to fail to propagate. How can I check whether we're currently reflowing it (either here or in Flush)? > (And it's also required if we're currently reflowing aSubtreeRoot.) That seems harder to ensure, though I guess Flush can check as it goes. > I also wonder if you really want to call UpdateOverflow on the subtree > root itself. Maybe you do, though? I guess I'll see later in the > patch. I think I've placed the calls to UpdateSticky after the scroll frame's overflow areas get updated -- probably I should move those, really, and then use the scrolled frame as the subtree root. > I tend to think mOverflowChangedTracker shouldn't be a member variable > of StickyScrollContainer, but instead a local variable in > UpdatePositions. It seems like data that we don't need to keep around. > Maybe I'm missing a good reason, though? Could go either way. On the other hand, it's data (um, three pointers in total) we don't need to bother recreating on every scroll event either. > StickyScrollContainer::UpdatePositions should probably assert that > aSubtreeRoot == do_QueryFrame(mScrollFrame) > (though that might need a cast) Sure, except when aSubtreeRoot == nullptr (as I do in ScrollPositionDidChange). > You should definitely define STICKY_DEBUG to no-op for landing. Certainly.

(In reply to Corey Ford [:corey] from comment #72) > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #71) > > Comment on attachment 789839 [details] [diff] [review] > > I think this needs to be a little more dramatic. In particular, it's > > only ok to call this when we're *currently reflowing* aSubtreeRoot; > > otherwise it could cause overflow changes to fail to propagate. > > How can I check whether we're currently reflowing it (either here or in > Flush)? > > > (And it's also required if we're currently reflowing aSubtreeRoot.) > > That seems harder to ensure, though I guess Flush can check as it goes. Sorry, I meant the *comment* needs to be a little more dramatic; I don't think you need to check anything. > > I also wonder if you really want to call UpdateOverflow on the subtree > > root itself. Maybe you do, though? I guess I'll see later in the > > patch. > > I think I've placed the calls to UpdateSticky after the scroll frame's > overflow areas get updated -- probably I should move those, really, and then > use the scrolled frame as the subtree root. May as well, since it seems very slightly nicer. > > I tend to think mOverflowChangedTracker shouldn't be a member variable > > of StickyScrollContainer, but instead a local variable in > > UpdatePositions. It seems like data that we don't need to keep around. > > Maybe I'm missing a good reason, though? > > Could go either way. On the other hand, it's data (um, three pointers in > total) we don't need to bother recreating on every scroll event either. But there isn't anything expensive about recreating it either, is there? > > StickyScrollContainer::UpdatePositions should probably assert that > > aSubtreeRoot == do_QueryFrame(mScrollFrame) > > (though that might need a cast) > > Sure, except when aSubtreeRoot == nullptr (as I do in > ScrollPositionDidChange). Ah, right, so "!aSubtreeRoot || ". And document that aSubtreeRoot should be null when not in reflow, and how it relates to what's being reflowed when in reflow.

Many pages on nbcnews.com scroll very choppy at the top of the page. Would what's being said here apply to these pages? IE10 is choppy too but less so than Fx25. Example: http://www.nbcnews.com/science/treasure-found-ancient-byzantine-garbage-pit-6C10908517

No. This bug is about implementing a new feature that might be used to write pages with effects like the ones on the page you point to, but it would have no effects on existing pages (such as that page) that don't use that feature.

Comment on attachment 789839 [details] [diff] [review] Part 6 v3: Implement sticky positioning, calculated on reflow and scroll. >+ if (!computedOffsets) { >+ // Reflow hasn't computed the offsets yet. Bail. >+ return normalPosition; >+ } This seems odd. It seems like we should assert that this doesn't happen... except that there's one edge case (interruptible reflow) where I could imagine it happening. I wonder if that case is easily detectable so that we can assert. >+ temp = mScrollPosition.y + sfPadding.top + computedOffsets->top - >+ sfOffset.y; Local style probably prefers lining up sfOffset.y with mScrollPosition.y. (4 times) >+ uint8_t direction = cbFrame->StyleVisibility()->mDirection; Have you checked that this is the correct frame's direction to use? Does it match WebKit? And does your current spec say to use that direction in particular? And do you think it makes sense? (And, more generally, is everything that this function does described by your spec draft?) Finally, please don't use |nscoord temp|. Use variable whose names describe what they are, and scope them to the minimum scope they can have. This means replacing temp with 8 variables, but perhaps only two names for those variables. (And I'm inclined to suggest just removing the STICKY_DEBUG lines.) Please document the aSubtreeRoot argument to UpdatePositions better -- explain that it's the root of the subtree to which overflow should be propagated, and therefore it should be non-null when UpdatePositions is used during reflow and null when it's not. r=dbaron with those things and comment 71 / comment 73

Comment on attachment 788399 [details] [diff] [review] Part 7 v2: Reftests for sticky positioning. The descriptions of what you're testing are great. I'm inclined to say the directory name should be position-sticky rather than sticky-pos. This is really easy to change by editing the patch (while it's not applied), which is easy to do in a version-controlled patch queue (but commit before and after just in case). If you're willing to put that metadata in the format described in http://wiki.csswg.org/test/format that would be even better -- don't worry about the link rel="help" for now, but adding the meta name="author", making the comment a meta name="assert", and adding meta name="flags" for all the tests that have script in them, and adding a title element would be useful, in that it would make the tests easier to contribute to a CSS WG test suite in the future. r=dbaron

Comment on attachment 787901 [details] [diff] [review] Part 5 v2: Always build a stacking context for sticky positioned elements. r=dbaron Please make sure the spec draft says this should happen.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #76) > Comment on attachment 789839 [details] [diff] [review] > Part 6 v3: Implement sticky positioning, calculated on reflow and scroll. > > >+ if (!computedOffsets) { > >+ // Reflow hasn't computed the offsets yet. Bail. > >+ return normalPosition; > >+ } > > This seems odd. It seems like we should assert that this doesn't happen... > except that there's one edge case (interruptible reflow) where I could > imagine it happening. I wonder if that case is easily detectable so > that we can assert. As I recall, this happens when we get a ScrollPositionDidChange when the page is loaded, before reflow gets to the sticky frame. Possibly I should avoid doing anything at all in that situation. > > >+ uint8_t direction = cbFrame->StyleVisibility()->mDirection; > > Have you checked that this is the correct frame's direction to use? Does > it match WebKit? No, WebKit does something different in this situation, the mechanics of which I still don't understand. http://lists.w3.org/Archives/Public/www-style/2013Jun/0675.html https://bugs.webkit.org/show_bug.cgi?id=118161 > And does your current spec say to use that direction in particular? Yes. "... except that if the 'direction' property of the containing block is 'ltr', 'right' is ignored, and if it is 'rtl', 'left' is ignored." > And do you think it makes sense? Yes; it parallels the rule for over-constrained relative positioning (CSS2.1 9.4.3). > (And, more generally, is everything that this function does described > by your spec draft?) I'll make sure of that; the only part I can think of is the step 3 logic, but that's more an artifact of the way I've done min/max in steps 1 and 2. > Finally, please don't use |nscoord temp|. Got it. By this point, I'm inclined to perhaps factor out the similar structure for each sticky direction into a helper function...

Addressed dholbert's comments on this.

Not foreseeing any especially substantive changes, then, let's see what Try thinks. https://tbpl.mozilla.org/?tree=Try&rev=d1fa3347b5ac

CSSWG test format. (Not all the <title>s are unique, but I also suspect some of the same tests would be too redundant for a W3C test suite).

Comment on attachment 791001 [details] [diff] [review] Part 7 v3: Reftests for sticky positioning. ... and carrying forward r+

Added fuzzy equality to the mochitest to fix the Windows test errors.

Changed back to using IsRelativelyPositioned to fix the reftests/svg/text/ignore-position.svg failure, and added position:sticky to that test case.

Addressed dbaron's review comments, and reformulated the algorithm to be a bit simpler (less nesting of ifs). (In reply to Corey Ford [:corey] from comment #79) > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from > comment #76) > > Comment on attachment 789839 [details] [diff] [review] > > Part 6 v3: Implement sticky positioning, calculated on reflow and scroll. > > > > >+ if (!computedOffsets) { > > >+ // Reflow hasn't computed the offsets yet. Bail. > > >+ return normalPosition; > > >+ } > > > > This seems odd. It seems like we should assert that this doesn't happen... > > except that there's one edge case (interruptible reflow) where I could > > imagine it happening. I wonder if that case is easily detectable so > > that we can assert. > > As I recall, this happens when we get a ScrollPositionDidChange when the > page is loaded, before reflow gets to the sticky frame. Possibly I should > avoid doing anything at all in that situation. Well, I added an NS_ERROR there, and I don't seem to be hitting it after all.

And a new Try push: https://tbpl.mozilla.org/?tree=Try&rev=67fb6f825eb4

Between the Android failures of my reftests (hmm...) and a silly oversight I just noticed in the calculations, I'll wait until next week to polish up and land this.

So the Android reftest failure turned out to just be another of those situations where I was trying to get a frame's size while reflowing it for the first time. I think I need to get the size out of the scroll frame's reflow state instead -- how does this look (hopefully interdiff will work)?

Added fuzzy-if(Android) annotations, plus borders+padding on more tests to better test the stay-inside-containing-block logic. And changed the padding-1 test such that it failed on desktop before v7 of part 4.

The containing block logic (the equations for |limit|) was varying degrees of wrong.

And let's Try again: https://tbpl.mozilla.org/?tree=Try&rev=4a6a2085af9a

Comment on attachment 792502 [details] [diff] [review] Part 4 v7: Compute sticky positioning offsets. >@@ -1940,22 +1981,52 @@ nsHTMLReflowState::InitConstraints(nsPre >+ if (NS_STYLE_POSITION_STICKY == mStyleDisplay->mPosition) { >+ nsIScrollableFrame* scrollableFrame = >+ nsLayoutUtils::GetNearestScrollableFrame(frame->GetParent(), >+ nsLayoutUtils::SCROLLABLE_SAME_DOC | >+ nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN); >+ >+ if (!scrollableFrame) { >+ // Not sure how this would happen, but bail if it does. >+ NS_ERROR("Couldn't find a scrollable frame"); >+ return; >+ } >+ >+ // Find the associated reflow state It'd be worth adding a comment here explaining why we can't just grab the size from the frame, like RestyleManager::RecomputePosition does. (because the frame might not know its up-to-date size yet, particularly if this is its first reflow) >+ NS_ASSERTION(sfReflowState, >+ "Couldn't find a matching reflow state"); >+ >+ nscoord scrollContainerWidth = sfReflowState->ComputedWidth(); Make this a MOZ_ASSERT (not NS_ASSERTION) -- if it fails, we're going to crash on the next line's pointer-deref anyway, and we might as well crash sooner & atomically with the assertion. >+ nscoord scrollContainerHeight = sfReflowState->ComputedHeight(); >+ ComputeStickyOffsets(frame, scrollContainerWidth, scrollContainerHeight); ComputedHeight() is fully allowed to be NS_AUTOHEIGHT, if the scrolled block is "height:auto". If you get that value here, and you have bottom-based sticky-positioning, you'll probably end up doing bogus arithmetic with NS_AUTOHEIGHT (where previously you were using the actual frame size), right? This might be tricky to solve, since you can't position the sticky frame until the ancestor (scrolled frame) knows its size, but on the other hand the scrolled frame won't know its height until it's reflowed all of its descendants & completed its own reflow.... We must have a way of handling that for abspos and fixedpos elements, since it seems like they can hit the same problem. (They get reflowed in a somewhat different codepath, though.)

(In reply to Daniel Holbert [:dholbert] from comment #93) > ComputedHeight() is fully allowed to be NS_AUTOHEIGHT, if the scrolled block > is "height:auto". If you get that value here, and you have bottom-based > sticky-positioning, you'll probably end up doing bogus arithmetic with > NS_AUTOHEIGHT (where previously you were using the actual frame size), right? Probably it will be caught by whatever sort of assertion we end up putting in ComputeCBDependentValue, but yes, I do need to handle that somehow. > This might be tricky to solve, since you can't position the sticky frame > until the ancestor (scrolled frame) knows its size, but on the other hand > the scrolled frame won't know its height until it's reflowed all of its > descendants & completed its own reflow.... We must have a way of handling > that for abspos and fixedpos elements, since it seems like they can hit the > same problem. (They get reflowed in a somewhat different codepath, though.) I think we reflow abspos elements after their containing block or something along those lines? (I do *position* sticky elements after their scroll frame is reflowed, but here we're just trying to compute the offsets earlier). (In practice, sticky positioning inside an auto-sized scrollframe isn't terribly useful since the scrollframe shouldn't scroll in that case. Would it be at all reasonable to just treat percentage offsets as 0 here, then?)

(In reply to Corey Ford [:corey] from comment #94) > Probably it will be caught by whatever sort of assertion we end up putting > in ComputeCBDependentValue, but yes, I do need to handle that somehow. True :) > I think we reflow abspos elements after their containing block or something > along those lines? Ah, yes -- that sounds right. > (I do *position* sticky elements after their scroll frame > is reflowed, but here we're just trying to compute the offsets earlier). Hmm. Maybe we can delay the offset-computation to when you position them, in this case? > (In practice, sticky positioning inside an auto-sized scrollframe isn't > terribly useful since the scrollframe shouldn't scroll in that case. Would > it be at all reasonable to just treat percentage offsets as 0 here, then?) I'm not sure. I agree it's a not-super-useful use-case. I don't have strong feelings RE just treating percentages as 0 in this situation, though I lean slightly against it... It'd probably be worth checking what webkit does, too.

(In reply to Daniel Holbert [:dholbert] from comment #95) > > (I do *position* sticky elements after their scroll frame > > is reflowed, but here we're just trying to compute the offsets earlier). > > Hmm. Maybe we can delay the offset-computation to when you position them, in > this case? Perhaps. I was about to say "but we don't need to do it every time we reflow the scroll frame" -- actually, we do need to recompute the offsets when its size changes. And the first time the result of the positioning algorithm is meaningful isn't until we reflow the scroll frame for the first time anyway. I would end up trying to position sticky elements during reflow before the offsets are computed, then, but I can just bail on the algorithm in that situation. > > (In practice, sticky positioning inside an auto-sized scrollframe isn't > > terribly useful since the scrollframe shouldn't scroll in that case. Would > > it be at all reasonable to just treat percentage offsets as 0 here, then?) > > I'm not sure. I agree it's a not-super-useful use-case. I don't have strong > feelings RE just treating percentages as 0 in this situation, though I lean > slightly against it... It'd probably be worth checking what webkit does, > too. No, it wouldn't be a great solution. And in a quick test WebKit seems to do the right thing (and we do bogus arithmetic).

Don't compute offsets in nsHTMLReflowState::InitConstraints.

Compute offsets in UpdatePositions when reflowing the scrollframe. These two patches now make offsets be computed at a better point. Possibly ComputeStickyOffsets should move into StickyScrollContainer.

Comment on attachment 793095 [details] [diff] [review] Part 4 v8: Compute sticky positioning offsets. Looks like this updated patch doesn't do anything in nsHTMLReflowState::InitConstraints. It should at least change this existing chunk... 1943 1944 // Compute our offsets if the element is relatively positioned. We need 1945 // the correct containing block width and height here, which is why we need 1946 // to do it after all the quirks-n-such above. 1947 if (mStyleDisplay->IsRelativelyPositioned(frame)) { 1948 uint8_t direction = NS_STYLE_DIRECTION_LTR; 1949 if (cbrs && NS_STYLE_DIRECTION_RTL == cbrs->mStyleVisibility->mDirection) { 1950 direction = NS_STYLE_DIRECTION_RTL; 1951 } 1952 ComputeRelativeOffsets(direction, frame, aContainingBlockWidth, 1953 aContainingBlockHeight, mComputedOffsets); http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1944 ...to check "position == POSITION_RELATIVE", inside the IsRelativelyPositioned check. And then maybe add a comment after that new "if", along the lines of: // Else, we're position:sticky. We can't compute our // sticky offsets here, because our scroll container // might not know its size yet. So, we'll compute those // in StickyScrollContainer::UpdatePositions(). >diff --git a/layout/reftests/svg/text/ignore-position-ref.svg b/layout/reftests/svg/text/ignore-position-ref.svg >--- a/layout/reftests/svg/text/ignore-position-ref.svg >+++ b/layout/reftests/svg/text/ignore-position-ref.svg You probably wanted to make these one-line reftest tweaks changes in the reftest patch, right? (Also: speaking of reftests -- if you haven't already, could you add a test to ensure we don't do "the right thing" instead of bogus arithmetic with NS_AUTOHEIGHT, per the end of comment 96?) (In reply to Corey Ford [:corey] from comment #98) > Possibly ComputeStickyOffsets should move into StickyScrollContainer. Yeah, I think so. (ComputeStickySideOffset, too.) It made sense in nsHTMLReflowState when it was invoked by code in nsHTMLReflowState, but now that's no longer true. That means this patch will probably need to be split up a bit, since StickyScrollContainer.cpp doesn't exist at this point in your patch-stack. :) Maybe this whole patch wants to be folded into part 6? At least the RestyleManager::RecomputePosition chunk does, to keep your ComputeStickyOffsets() invocations together in the same patch. If you like, you could leave the ComputeStickyOffsets() impl out of part 6, and replace its invocations with: // XXXcorey invoke ComputeStickyOffsets() here and then have part 6.5 (or whatever) add the ComputeStickyOffsets() impl, and the frame-property, and replace those XXX comments with actual invocations.

(In reply to Daniel Holbert [:dholbert] from comment #99) > (Also: speaking of reftests -- if you haven't already, could you add a test > to ensure we don't do "the right thing" instead of bogus arithmetic er 'to ensure we _do_ "the right thing"' :)

(In reply to Daniel Holbert [:dholbert] from comment #99) > Comment on attachment 793095 [details] [diff] [review] > Part 4 v8: Compute sticky positioning offsets. > > Looks like this updated patch doesn't do anything in > nsHTMLReflowState::InitConstraints. It should at least change this existing > chunk... It looks to me like that's in this patch. But the comment is a good idea. > > >diff --git a/layout/reftests/svg/text/ignore-position-ref.svg b/layout/reftests/svg/text/ignore-position-ref.svg > >--- a/layout/reftests/svg/text/ignore-position-ref.svg > >+++ b/layout/reftests/svg/text/ignore-position-ref.svg > > You probably wanted to make these one-line reftest tweaks changes in the > reftest patch, right? Either way -- they're tests I originally broke with an earlier version of this patch, so that's why they got added here. > (Also: speaking of reftests -- if you haven't already, could you add a test > to ensure we don't do "the right thing" instead of bogus arithmetic with > NS_AUTOHEIGHT, per the end of comment 96?) Yes, I have one of those.

(In reply to Corey Ford [:corey] (offline until ~08-28) from comment #101) > > Looks like this updated patch doesn't do anything in > > nsHTMLReflowState::InitConstraints. It should at least change this existing > > chunk... > > It looks to me like that's in this patch. But the comment is a good idea. Ah, you're right, sorry. But I think it's not quite right -- per comment 85, you still need to keep the IsRelativelyPositioned() check, to avoid breaking the SVG reftests. And then inside of that clause (or subsequently in the same "if" check), you can test for POSITION_RELATIVE. > > You probably wanted to make these one-line reftest tweaks changes in the > > reftest patch, right? > > Either way -- they're tests I originally broke with an earlier version of > this patch, so that's why they got added here. (Ah, right -- sorry, I didn't look closely enough at the tests). These are the tests that require you to keep the IsRelativelyPositioned() check, IIRC.

> > (Also: speaking of reftests -- if you haven't already, could you add a test > > to ensure we don't do "the right thing" instead of bogus arithmetic with > > NS_AUTOHEIGHT, per the end of comment 96?) > > Yes, I have one of those. Except now I can't find it. Reminder to self to actually write a test for auto-sized scroll containers.

Folded part 4 in here, since offsets computation is now in StickyScrollContainer.cpp. I also factored out most of the heavy lifting from the positioning calculations into a ComputeStickyLimits method, which will help avoid duplicating it in bug 897105. dbaron, would you look over that refactoring, and also the part of UpdatePositions where I compute offsets? (I just reassured myself that the latter does work as intended; I'll get to the reftest in a few days.) dholbert, would you double-check that all the offsets stuff looks fine now (RestyleManager::RecomputePosition, nsHTMLReflowState::InitConstraints, StickyScrollContainer::UpdatePositions)?

Rebased this to account for changes from bug 825771.

Added a test for auto-height scroll containers and one for recomputing offsets when the scroll container reflows. Also moved the SVG reftest changes into this patch.

And yet another Try push: https://tbpl.mozilla.org/?tree=Try&rev=b5bb7e1168c2

Comment on attachment 798356 [details] [diff] [review] Part 6 v7: Implement sticky positioning, calculated on reflow and scroll. (In reply to Corey Ford [:corey] from comment #104) > dholbert, would you double-check that all the offsets stuff looks fine now > (RestyleManager::RecomputePosition, nsHTMLReflowState::InitConstraints, > StickyScrollContainer::UpdatePositions)? Looks good to me! r=me on the offsets stuff. (One nit that I noticed in other code in this patch -- it looks like mSubtreeRoot is only used for equality-checks (we don't invoke anything on it), so it presumably could be a "const nsIFrame*", right?)

Comment on attachment 798356 [details] [diff] [review] Part 6 v7: Implement sticky positioning, calculated on reflow and scroll. >+ aStick->SetRect(nscoord_MIN/2, nscoord_MIN/2, >+ nscoord_MAX - nscoord_MIN/2, nscoord_MAX - nscoord_MIN/2); >+ aContain->SetRect(nscoord_MIN/2, nscoord_MIN/2, >+ nscoord_MAX - nscoord_MIN/2, nscoord_MAX - nscoord_MIN/2); The width/height params should just be nscoord_MAX; you're currently making them bigger than nscoord_MAX. Then you'll have a nscoord_MAX-sized rect centered on 0,0. Or maybe you want an (nscoord_MAX * 2 - 1) sized rect instead, that is, a rect whose x and y are nscoord_MIN, so its XMost() and YMost() are nscoord_MAX ? I think that would be preferable, though it's a little odd. It would simplify things a little if you assigned: nsPoint sfOffset = aFrame->GetParent()->GetOffsetTo(scrolledFrame) - nsPoint(sfPadding.left, sfPadding.top); and then removed all further uses of sfPadding. Likewise, you could assign: nsPoint cbOffset = nsPoint(cbBorderPadding.left, cbBorderPadding.top) - aFrame->GetParent()->GetOffsetTo(cbFrame); and then remove all further uses of cbBorderPadding, and **drop the minus sign before the use of cbOffset**. >+ // For each sticky direction (top, bottom, left, right), move the frame along >+ // the appropriate axis, based on the scroll position, but limit this to keep >+ // the element's margin box within the containing block. >+ position.y = std::max(position.y, std::min(stick.y, contain.YMost())); >+ position.y = std::min(position.y, std::max(stick.YMost(), contain.y)); >+ position.x = std::max(position.x, std::min(stick.x, contain.XMost())); >+ position.x = std::min(position.x, std::max(stick.XMost(), contain.x)); Hmmm. This makes me wonder if the order of the last two statements needs to be flipped for RTL. It's probably ok to save clearing that up to a followup bug, though. (Or maybe it's ok because of the way ComputeStickyLimits accounts for the frame's size?) >+ /** >+ * Compute two rectangles that determine sticky positioning: |aStick|, based >+ * on the scroll container, and |aContain|, based on the containing block. >+ * Sticky positioning keeps the frame position always within |aContain| and >+ * secondarily within |aStick|. >+ */ It doesn't quite seem true that it's always within aContain, since it could be bigger than aContain. Anyway, r=dbaron on the ComputeStickyLimits refactoring with those comments.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #109) > Comment on attachment 798356 [details] [diff] [review] > Part 6 v7: Implement sticky positioning, calculated on reflow and scroll. > > >+ aStick->SetRect(nscoord_MIN/2, nscoord_MIN/2, > >+ nscoord_MAX - nscoord_MIN/2, nscoord_MAX - nscoord_MIN/2); > >+ aContain->SetRect(nscoord_MIN/2, nscoord_MIN/2, > >+ nscoord_MAX - nscoord_MIN/2, nscoord_MAX - nscoord_MIN/2); > > The width/height params should just be nscoord_MAX; you're currently > making them bigger than nscoord_MAX. Then you'll have a nscoord_MAX-sized > rect centered on 0,0. That sounds good. (Though FWIW, I borrowed this from nsGfxScrollFrameInner::GetScrollRangeForClamping.) > Or maybe you want an (nscoord_MAX * 2 - 1) sized rect instead, that is, > a rect whose x and y are nscoord_MIN, so its XMost() and YMost() are > nscoord_MAX ? I think that would be preferable, though it's a little > odd. (Not quite; nscoord_MIN is defined as (-nscoord_MAX) so the size would need to be (nscoord_MAX * 2), but of course that overflows.) > >+ // For each sticky direction (top, bottom, left, right), move the frame along > >+ // the appropriate axis, based on the scroll position, but limit this to keep > >+ // the element's margin box within the containing block. > >+ position.y = std::max(position.y, std::min(stick.y, contain.YMost())); > >+ position.y = std::min(position.y, std::max(stick.YMost(), contain.y)); > >+ position.x = std::max(position.x, std::min(stick.x, contain.XMost())); > >+ position.x = std::min(position.x, std::max(stick.XMost(), contain.x)); > > Hmmm. This makes me wonder if the order of the last two statements > needs to be flipped for RTL. It's probably ok to save clearing that up > to a followup bug, though. > > (Or maybe it's ok because of the way ComputeStickyLimits accounts for > the frame's size?) Yes. The only way the order of those statements would matter is with left+right sticky where the element is wider than the scroll container. ComputeStickyLimits ignores one or the other (based on direction) in that situation, and I have tests for these various "overconstrained" cases. > >+ /** > >+ * Compute two rectangles that determine sticky positioning: |aStick|, based > >+ * on the scroll container, and |aContain|, based on the containing block. > >+ * Sticky positioning keeps the frame position always within |aContain| and > >+ * secondarily within |aStick|. > >+ */ > > It doesn't quite seem true that it's always within aContain, since > it could be bigger than aContain. The frame *position* should always stay within aContain. Thanks!

(In reply to David Baron [:dbaron] (needinfo? me) from comment #109) > It would simplify things a little if you assigned: > nsPoint sfOffset = aFrame->GetParent()->GetOffsetTo(scrolledFrame) - > nsPoint(sfPadding.left, sfPadding.top); > and then removed all further uses of sfPadding. True, but that's conceptually a bit harder; "- sfOffset" in the calculations currently can be read as "convert from scrolledframe coordinate space to parent coordinate space". > Likewise, you could assign: > nsPoint cbOffset = nsPoint(cbBorderPadding.left, cbBorderPadding.top) - > aFrame->GetParent()->GetOffsetTo(cbFrame); > and then remove all further uses of cbBorderPadding, and **drop the > minus sign before the use of cbOffset**. Actually, here I can now move this all inside the aContain computation block and drop some of the variables.

Addressed dholbert's and dbaron's review comments.

Aaand that should do it. Another try build from yesterday turned out looking fine: https://tbpl.mozilla.org/?tree=Try&rev=a3eee86de2d8 (Parts 2 and 5 need r=heycam and r=dbaron added to their respective commit messages before checking in. And yes, there is no longer a part 4.)

(In reply to Corey Ford [:corey] from comment #110) > (In reply to David Baron [:dbaron] (needinfo? me) from comment #109) > > Comment on attachment 798356 [details] [diff] [review] > > Or maybe you want an (nscoord_MAX * 2 - 1) sized rect instead, that is, > > a rect whose x and y are nscoord_MIN, so its XMost() and YMost() are > > nscoord_MAX ? I think that would be preferable, though it's a little > > odd. > > (Not quite; nscoord_MIN is defined as (-nscoord_MAX) so the size would need > to be (nscoord_MAX * 2), but of course that overflows.) right, but nscoord_MAX * 2 - 1 does not overflow. (But the XMost() and YMost() would be nscoord_MAX - 1, though -- I was wrong on that the first time.) > > >+ /** > > >+ * Compute two rectangles that determine sticky positioning: |aStick|, based > > >+ * on the scroll container, and |aContain|, based on the containing block. > > >+ * Sticky positioning keeps the frame position always within |aContain| and > > >+ * secondarily within |aStick|. > > >+ */ > > > > It doesn't quite seem true that it's always within aContain, since > > it could be bigger than aContain. > > The frame *position* should always stay within aContain. I think you should make it clearer that you mean the frame's top-left corner. (Although for RTL is it the top-left or the top-right? It *ought* to be the top right.) (In reply to Corey Ford [:corey] from comment #111) > (In reply to David Baron [:dbaron] (needinfo? me) from comment #109) > > It would simplify things a little if you assigned: > > nsPoint sfOffset = aFrame->GetParent()->GetOffsetTo(scrolledFrame) - > > nsPoint(sfPadding.left, sfPadding.top); > > and then removed all further uses of sfPadding. > > True, but that's conceptually a bit harder; "- sfOffset" in the calculations > currently can be read as "convert from scrolledframe coordinate space to > parent coordinate space". You could think of it as converting to the coordinate space of the scrollframe's padding box.

(In reply to Corey Ford [:corey] from comment #113) > (Parts 2 and 5 need r=heycam and r=dbaron added to their respective commit > messages before checking in. And yes, there is no longer a part 4.) You should probably repost the patches for that.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #114) > right, but nscoord_MAX * 2 - 1 does not overflow. (But the XMost() and > YMost() would be nscoord_MAX - 1, though -- I was wrong on that the first > time.) Right, but if you don't mind I'll just stick with the "nscoord_MAX sized rectangle centered on 0, 0". > > The frame *position* should always stay within aContain. > > I think you should make it clearer that you mean the frame's top-left > corner. (Although for RTL is it the top-left or the top-right? It *ought* > to be the top right.) Yeah, that could be a bit more explicit. It shouldn't change for RTL, though -- the ComputeStickyLimits add in the frame's size as part of the bottom/right calculations (but yes, that might want to change eventually if we're wanting to avoid ever thinking about things in a top-left-centric coordinate system.) > > > It would simplify things a little if you assigned: > > > nsPoint sfOffset = aFrame->GetParent()->GetOffsetTo(scrolledFrame) - > > > nsPoint(sfPadding.left, sfPadding.top); > > > and then removed all further uses of sfPadding. > > > > True, but that's conceptually a bit harder; "- sfOffset" in the calculations > > currently can be read as "convert from scrolledframe coordinate space to > > parent coordinate space". > > You could think of it as converting to the coordinate space of the > scrollframe's padding box. I don't think it's quite that either -- wouldn't it be converting (from) the coordinate space of the scrollframe's padding box *inflated* by padding?

Very well. Re-uploading 2 and 5 with r=

Okay, improved that comment a bit. Now really ready for checkin.

Rebased this yet again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc50ede32239 https://hg.mozilla.org/integration/mozilla-inbound/rev/d65d4436d3a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/90d700eaeac0 https://hg.mozilla.org/integration/mozilla-inbound/rev/05c2da42c84e https://hg.mozilla.org/integration/mozilla-inbound/rev/63a55ac51f7f https://hg.mozilla.org/integration/mozilla-inbound/rev/bb399c38e3b6

per irc discussion w/ corey & RyanVM, I pushed a followup to tweak one of the fuzzy-if annotations (since it sporadically needs slightly more fuzziness than its initial annotation allowed for): https://hg.mozilla.org/integration/mozilla-inbound/rev/d75c702f3a43

https://hg.mozilla.org/mozilla-central/rev/cc50ede32239 https://hg.mozilla.org/mozilla-central/rev/d65d4436d3a6 https://hg.mozilla.org/mozilla-central/rev/90d700eaeac0 https://hg.mozilla.org/mozilla-central/rev/05c2da42c84e https://hg.mozilla.org/mozilla-central/rev/63a55ac51f7f https://hg.mozilla.org/mozilla-central/rev/bb399c38e3b6 (Will get the fuzzy follow-up on the next merge)

If this uplifts to Aurora, we'll probably want to release note it. Could someone who is familiar with this write up a one or two sentence description for the release notes? Is this worth a blog post as well?

Note this in Aurora only - shouldn't carry to Beta notes.

Not least because per bug 902992 this will be disabled by default in Beta for now. Simply "Implemented CSS sticky positioning"? Or maybe "Experimental implementation of"? My intern presentation is a decent source of extra information: https://air.mozilla.org/intern-presentation-ford/