Closed Bug 886646 Opened 11 years ago Closed 11 years ago

implement position:sticky

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
relnote-firefox --- 26+

People

(Reporter: dbaron, Assigned: coyotebush)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(7 files, 43 obsolete files)

6.18 KB, text/html
Details
10.03 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
94.85 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
1.04 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
1.34 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
28.85 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
11.02 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
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.
Attached patch Basic reftests (obsolete) — Splinter Review
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.
Attached file Updated reftest suite (obsolete) —
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.
Attachment #768440 - Attachment is obsolete: true
Attached patch Current progress (obsolete) — Splinter Review
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.
Attachment #773700 - Flags: feedback?(dbaron)
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.
Attachment #773700 - Flags: feedback?(dbaron) → feedback+
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
Attachment #774360 - Flags: review?(dbaron)
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...)
Attachment #774360 - Flags: review?(dbaron) → review-
(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.
Attachment #774360 - Attachment is obsolete: true
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.
Attachment #778154 - Flags: feedback?(dbaron)
Attachment #773700 - Attachment is obsolete: true
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?
Attachment #778212 - Flags: feedback?(dbaron)
Attachment #778212 - Flags: feedback?(roc)
Attached file simple testcase
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.
Attachment #778212 - Flags: feedback?(roc) → feedback+
Depends on: 896548
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.
Attachment #778154 - Flags: feedback?(dbaron) → feedback+
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.
Attachment #778212 - Flags: feedback?(dbaron) → feedback+
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.
Blocks: 897105
Depends on: 893962
Depends on: 898794
Depends on: 898797
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.
Attachment #778154 - Attachment is obsolete: true
A similar update to these computations, plus a mochitest (currently in test_computed_style.html, but could go in its own file instead).
Attachment #778581 - Attachment is obsolete: true
Attachment #784130 - Attachment description: Part 4: Compute sticky positioning offsets for getComputedStyle(). → Part 4 v2: Compute sticky positioning offsets for getComputedStyle().
Rebased to account for nsStyleConsts.h move.
Attachment #769750 - Attachment is obsolete: true
Attachment #785844 - Attachment description: Part 1: Support position:sticky in the CSS parser. → Part 1 v2: Support position:sticky in the CSS parser.
Depends on: 901610
And another revision to include sticky in the property_database.js test file.
Attachment #785844 - Attachment is obsolete: true
Attachment #786115 - Flags: review?(cam)
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.
Attachment #786530 - Flags: review?(dholbert)
Attachment #784071 - Attachment is obsolete: true
Attachment #786530 - Attachment description: Part 3: Compute sticky positioning offsets. → Part 3 v3: Compute sticky positioning offsets.
Attachment #786532 - Flags: review?(dholbert)
Attachment #786368 - Attachment is obsolete: true
Attachment #770887 - Flags: review?(cam)
Attachment #784130 - Flags: review?(cam)
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.
Attachment #786115 - Flags: review?(cam)
Comment on attachment 770887 [details] [diff] [review]
Part 2: Include sticky positioning in nsStyleDisplay::IsRelativelyPositionedStyle

Review of attachment 770887 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #770887 - Flags: review?(cam) → review+
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?
Attachment #784130 - Flags: review?(cam) → review+
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.)
Attachment #787158 - Flags: review?(cam)
Attachment #786115 - Attachment is obsolete: true
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?
Attachment #787197 - Flags: review?(cam)
Attachment #784130 - Attachment is obsolete: true
Attachment #787197 - Attachment description: Part 4: Compute sticky positioning offsets for getComputedStyle(). → Part 4 v3: Compute sticky positioning offsets for getComputedStyle().
Comment on attachment 786532 [details] [diff] [review]
Part 5 v2: Handle sticky positioning in RecomputePosition.

This can wait, since it depends on bug 898794.
Attachment #786532 - Flags: review?(dholbert)
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?
Attachment #787158 - Flags: review?(cam) → review+
Comment on attachment 787197 [details] [diff] [review]
Part 4 v3: Compute sticky positioning offsets for getComputedStyle().

This all looks good.  r=me
Attachment #787197 - Flags: review?(cam) → review+
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.
Attachment #787158 - Attachment is obsolete: true
Attachment #787263 - Flags: review+
(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.
Blocks: 902992
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.)
Attachment #773622 - Attachment is obsolete: true
Attachment #778212 - Attachment is obsolete: true
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.
Attachment #787263 - Attachment is obsolete: true
Attachment #787869 - Flags: review+
Rebased onto this morning's m-c (a small context change), and renumbering this patch more sensibly (from 4 to 3).
Attachment #787197 - Attachment is obsolete: true
Comment on attachment 787891 [details] [diff] [review]
Part 3 v4: Compute sticky positioning offsets for getComputedStyle().

Keeping heycam's r+ from "Part 4 v3".
Attachment #787891 - Flags: review+
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).
Attachment #786530 - Attachment is obsolete: true
Attachment #786532 - Attachment is obsolete: true
Attachment #786530 - Flags: review?(dholbert)
Attachment #787896 - Flags: review?(dholbert)
Renumbering this one too (from 6).
Attachment #786577 - Attachment is obsolete: true
Attachment #787901 - Flags: review?(dbaron)
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.
Attachment #787905 - Flags: review?(dbaron)
And here's a pile of tests, primarily for the code in part 6.
Attachment #787908 - Flags: review?(dbaron)
Attachment #787908 - Flags: feedback?(dholbert)
The code style and null checks from ComputeStickyOffsets are relevant here too.
Attachment #787891 - Attachment is obsolete: true
Attachment #787931 - Attachment description: Part 3: Compute sticky positioning offsets for getComputedStyle(). → Part 3 v5: Compute sticky positioning offsets for getComputedStyle().
Attachment #787931 - Flags: review+
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.
Attachment #787896 - Flags: review?(dholbert) → review+
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)
Attachment #787908 - Flags: feedback?(dholbert) → feedback+
(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.
Attachment #787908 - Attachment is obsolete: true
Attachment #787908 - Flags: review?(dbaron)
Attachment #788399 - Attachment description: Part 7: Reftests for sticky positioning. → Part 7 v2: Reftests for sticky positioning.
Attachment #788399 - Flags: review?(dbaron)
Blocks: 904197
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.
Attachment #787931 - Attachment is obsolete: true
Attachment #789224 - Flags: review+
Fixed the GetNearestScrollableFrame call and rebased on this morning's m-c.
Attachment #789231 - Flags: review?(dbaron)
Attachment #787905 - Attachment is obsolete: true
Attachment #787905 - Flags: review?(dbaron)
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
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
(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.
Attachment #789839 - Flags: review?(dbaron)
Attachment #789231 - Attachment is obsolete: true
Attachment #789231 - Flags: review?(dbaron)
Attachment #789839 - Attachment description: Part 6: Implement sticky positioning, calculated on reflow and scroll. → Part 6 v3: Implement sticky positioning, calculated on reflow and scroll.
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
Attachment #789839 - Flags: review?(dbaron) → review+
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
Attachment #788399 - Flags: review?(dbaron) → review+
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.
Attachment #787901 - Flags: review?(dbaron) → review+
(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.
Attachment #787896 - Attachment is obsolete: true
Attachment #790898 - Flags: review+
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).
Attachment #788399 - Attachment is obsolete: true
Comment on attachment 791001 [details] [diff] [review]
Part 7 v3: Reftests for sticky positioning.

... and carrying forward r+
Attachment #791001 - Flags: review+
Added fuzzy equality to the mochitest to fix the Windows test errors.
Attachment #789224 - Attachment is obsolete: true
Changed back to using IsRelativelyPositioned to fix the reftests/svg/text/ignore-position.svg failure, and added position:sticky to that test case.
Attachment #790898 - Attachment is obsolete: true
Attachment #791059 - Flags: review+
Attachment #791060 - Attachment description: Part 4: Compute sticky positioning offsets. → Part 4 v6: Compute sticky positioning offsets.
Attachment #791060 - Flags: review+
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.
Attachment #789839 - Attachment is obsolete: true
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)?
Attachment #792502 - Flags: review?(dholbert)
Attachment #791060 - Attachment is obsolete: true
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.
Attachment #791001 - Attachment is obsolete: true
The containing block logic (the equations for |limit|) was varying degrees of wrong.
Attachment #791063 - Attachment is obsolete: true
Attachment #792508 - Flags: review+
Attachment #792506 - Flags: review+
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.
Attachment #792502 - Attachment is obsolete: true
Attachment #792502 - Flags: review?(dholbert)
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.
Attachment #792508 - Attachment is obsolete: true
Attachment #793095 - Attachment description: Part 4: Compute sticky positioning offsets. → Part 4 v8: Compute sticky positioning offsets.
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)?
Attachment #793095 - Attachment is obsolete: true
Attachment #793099 - Attachment is obsolete: true
Attachment #798356 - Flags: review?(dholbert)
Attachment #798356 - Flags: review?(dbaron)
Rebased this to account for changes from bug 825771.
Attachment #787869 - Attachment is obsolete: true
Attachment #798899 - Attachment description: Part 1: Support position:sticky in the CSS parser, enabled by a preference. → Part 1 v7: Support position:sticky in the CSS parser, enabled by a preference.
Attachment #798899 - Flags: review+
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.
Attachment #792506 - Attachment is obsolete: true
Attachment #798910 - Flags: review+
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?)
Attachment #798356 - Flags: review?(dholbert) → review+
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.
Attachment #798356 - Flags: review?(dbaron) → review+
(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.
Attachment #798356 - Attachment is obsolete: true
Attachment #800276 - Flags: review+
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.)
Keywords: checkin-needed
(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=
Attachment #770887 - Attachment is obsolete: true
Attachment #800369 - Flags: review+
Okay, improved that comment a bit. Now really ready for checkin.
Attachment #800276 - Attachment is obsolete: true
Attachment #800373 - Flags: review+
Attachment #798899 - Attachment is obsolete: true
Attachment #800433 - Attachment description: Part 1: Support position:sticky in the CSS parser, enabled by a preference. → Part 1 v8: Support position:sticky in the CSS parser, enabled by a preference.
Attachment #800433 - Flags: review+
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
Depends on: 914891
Depends on: 914919
Blocks: 915302
Depends on: 915475
Depends on: 916115
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/
Depends on: 918994
Depends on: 919156
Depends on: 919434
Depends on: 925259
Blocks: 916315
Blocks: 930419
No longer blocks: 930419
Whiteboard: [DocArea=CSS]
The relnote shouldn't be in the beta notes, right?
See bug 902992 and bug 916315
Flags: needinfo?(lsblakk)
(In reply to sjw from comment #129)
> The relnote shouldn't be in the beta notes, right?
> See bug 902992 and bug 916315

Thanks for catching that, missed it sticking around from older notes.
Flags: needinfo?(lsblakk)
Blocks: 975644
Depends on: 926155
Depends on: 1001994
I believe the documentation on this is already good enough now.

Sebastian
Depends on: 1131944
Depends on: 1199991
You need to log in before you can comment on or make changes to this bug.