Last Comment Bug 886646 - implement position:sticky
: implement position:sticky
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla26
Assigned To: Corey Ford [:coyotebush]
:
:
Mentors:
http://dev.w3.org/csswg/css-position/...
: 719141 (view as bug list)
Depends on: 914919 1131944 893962 896548 898794 898797 901610 914891 915475 916115 918994 919156 919434 925259 926155 1001994 1199991
Blocks: 915302 975644 897105 902992 904197 916315
  Show dependency treegraph
 
Reported: 2013-06-24 19:04 PDT by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2015-08-30 07:38 PDT (History)
20 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
26+


Attachments
Basic reftests (10.57 KB, patch)
2013-06-27 11:00 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 1: Add position: sticky to the CSS parser (2.55 KB, patch)
2013-07-01 10:42 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 2: Include sticky positioning in nsStyleDisplay::IsRelativelyPositionedStyle (1.03 KB, patch)
2013-07-03 10:27 PDT, Corey Ford [:coyotebush]
cam: review+
Details | Diff | Splinter Review
Updated reftest suite (8.66 KB, application/zip)
2013-07-10 15:13 PDT, Corey Ford [:coyotebush]
no flags Details
Current progress (12.41 KB, patch)
2013-07-10 16:43 PDT, Corey Ford [:coyotebush]
dbaron: feedback+
Details | Diff | Splinter Review
Refactor the application of CSS relative positioning. (8.03 KB, patch)
2013-07-11 17:04 PDT, Corey Ford [:coyotebush]
dbaron: review-
Details | Diff | Splinter Review
Part 3: Compute sticky positioning offsets. (6.51 KB, patch)
2013-07-18 16:38 PDT, Corey Ford [:coyotebush]
dbaron: feedback+
Details | Diff | Splinter Review
Sticky positioning with nsStickyScrollingContainer object (11.54 KB, patch)
2013-07-18 18:42 PDT, Corey Ford [:coyotebush]
dbaron: feedback+
roc: feedback+
Details | Diff | Splinter Review
simple testcase (6.18 KB, text/html)
2013-07-18 18:50 PDT, Corey Ford [:coyotebush]
no flags Details
Part 4: Compute sticky positioning offsets for getComputedStyle(). (5.13 KB, patch)
2013-07-19 11:21 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 3 v2: Compute sticky positioning offsets. (6.52 KB, patch)
2013-07-31 16:09 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 4 v2: Compute sticky positioning offsets for getComputedStyle(). (7.33 KB, patch)
2013-07-31 18:20 PDT, Corey Ford [:coyotebush]
cam: review+
Details | Diff | Splinter Review
Part 1 v2: Support position:sticky in the CSS parser. (2.67 KB, patch)
2013-08-05 10:38 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 1 v3: Support position:sticky in the CSS parser. (3.42 KB, patch)
2013-08-05 21:02 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 5: Handle sticky positioning in RecomputePosition. (2.96 KB, patch)
2013-08-06 10:27 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 3 v3: Compute sticky positioning offsets. (6.38 KB, patch)
2013-08-06 15:10 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 5 v2: Handle sticky positioning in RecomputePosition. (2.88 KB, patch)
2013-08-06 15:12 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 6: Always build a stacking context for sticky positioned elements. (1.44 KB, patch)
2013-08-06 16:00 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 1 v4: Support position:sticky in the CSS parser, enabled by a preference. (10.47 KB, patch)
2013-08-07 15:19 PDT, Corey Ford [:coyotebush]
cam: review+
Details | Diff | Splinter Review
Part 4 v3: Compute sticky positioning offsets for getComputedStyle(). (9.76 KB, patch)
2013-08-07 16:20 PDT, Corey Ford [:coyotebush]
cam: review+
Details | Diff | Splinter Review
Part 1 v5: Support position:sticky in the CSS parser, enabled by a preference. (10.51 KB, patch)
2013-08-07 18:47 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 1 v6: Support position:sticky in the CSS parser, enabled by a preference. (11.00 KB, patch)
2013-08-08 17:39 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 3 v4: Compute sticky positioning offsets for getComputedStyle(). (9.76 KB, patch)
2013-08-08 18:42 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 4 v4: Compute sticky positioning offsets. (9.13 KB, patch)
2013-08-08 18:54 PDT, Corey Ford [:coyotebush]
dholbert: review+
Details | Diff | Splinter Review
Part 5 v2: Always build a stacking context for sticky positioned elements. (1.33 KB, patch)
2013-08-08 19:02 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Splinter Review
Part 6: Implement sticky positioning, calculated on reflow and scroll. (17.90 KB, patch)
2013-08-08 19:07 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 7: Reftests for sticky positioning. (69.40 KB, patch)
2013-08-08 19:09 PDT, Corey Ford [:coyotebush]
dholbert: feedback+
Details | Diff | Splinter Review
Part 3 v5: Compute sticky positioning offsets for getComputedStyle(). (9.83 KB, patch)
2013-08-08 20:02 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 7 v2: Reftests for sticky positioning. (73.78 KB, patch)
2013-08-09 15:24 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Splinter Review
Part 3 v6: Compute sticky positioning offsets for getComputedStyle(). (9.92 KB, patch)
2013-08-12 15:54 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 6 v2: Implement sticky positioning, calculated on reflow and scroll. (17.83 KB, patch)
2013-08-12 15:59 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 6 v3: Implement sticky positioning, calculated on reflow and scroll. (20.50 KB, patch)
2013-08-13 14:53 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Splinter Review
Part 4 v5: Compute sticky positioning offsets. (9.60 KB, patch)
2013-08-15 12:27 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 7 v3: Reftests for sticky positioning. (86.11 KB, patch)
2013-08-15 15:39 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 3 v7: Compute sticky positioning offsets for getComputedStyle(). (10.03 KB, patch)
2013-08-15 18:28 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 4 v6: Compute sticky positioning offsets. (11.57 KB, patch)
2013-08-15 18:30 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 6 v4: Implement sticky positioning, calculated on reflow and scroll. (20.41 KB, patch)
2013-08-15 18:39 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 4 v7: Compute sticky positioning offsets. (12.91 KB, patch)
2013-08-19 16:46 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 7 v4: Reftests for sticky positioning. (89.01 KB, patch)
2013-08-19 16:50 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 6 v5: Implement sticky positioning, calculated on reflow and scroll. (20.48 KB, patch)
2013-08-19 16:57 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 4 v8: Compute sticky positioning offsets. (10.84 KB, patch)
2013-08-20 14:13 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 6 v6: Implement sticky positioning, calculated on reflow and scroll. (20.73 KB, patch)
2013-08-20 14:17 PDT, Corey Ford [:coyotebush]
no flags Details | Diff | Splinter Review
Part 6 v7: Implement sticky positioning, calculated on reflow and scroll. (28.92 KB, patch)
2013-09-01 18:55 PDT, Corey Ford [:coyotebush]
dbaron: review+
dholbert: review+
Details | Diff | Splinter Review
Part 1 v7: Support position:sticky in the CSS parser, enabled by a preference. (11.05 KB, patch)
2013-09-03 09:16 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 7 v5: Reftests for sticky positioning. (94.85 KB, patch)
2013-09-03 09:32 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 6 v8: Implement sticky positioning, calculated on reflow and scroll. (28.82 KB, patch)
2013-09-05 11:20 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 2 v2: Include sticky positioning in nsStyleDisplay::IsRelativelyPositionedStyle. (1.04 KB, patch)
2013-09-05 13:49 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 5 v3: Always build a stacking context for sticky positioned elements. (1.34 KB, patch)
2013-09-05 13:50 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 6 v9: Implement sticky positioning, calculated on reflow and scroll. (28.85 KB, patch)
2013-09-05 13:55 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Part 1 v8: Support position:sticky in the CSS parser, enabled by a preference. (11.02 KB, patch)
2013-09-05 15:47 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-06-24 19:04:54 PDT
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
Comment 1 Corey Ford [:coyotebush] 2013-06-24 21:01:05 PDT
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.
Comment 2 Corey Ford [:coyotebush] 2013-06-27 11:00:13 PDT
Created attachment 768440 [details] [diff] [review]
Basic reftests

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.
Comment 3 Corey Ford [:coyotebush] 2013-07-01 10:42:56 PDT
Created attachment 769750 [details] [diff] [review]
Part 1: Add position: sticky to the CSS parser
Comment 4 Corey Ford [:coyotebush] 2013-07-03 10:27:58 PDT
Created attachment 770887 [details] [diff] [review]
Part 2: Include sticky positioning in nsStyleDisplay::IsRelativelyPositionedStyle
Comment 5 Corey Ford [:coyotebush] 2013-07-10 15:13:41 PDT
Created attachment 773622 [details]
Updated reftest suite

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.
Comment 6 Corey Ford [:coyotebush] 2013-07-10 16:43:23 PDT
Created attachment 773700 [details] [diff] [review]
Current progress

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 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-07-11 14:30:29 PDT
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.
Comment 8 Corey Ford [:coyotebush] 2013-07-11 16:00:59 PDT
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
Comment 9 Corey Ford [:coyotebush] 2013-07-11 17:04:26 PDT
Created attachment 774360 [details] [diff] [review]
Refactor the application of CSS relative positioning.

Good idea, I can separate out this part of the patch.

On try: https://tbpl.mozilla.org/?tree=Try&rev=21cbc17b0d24
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-07-12 11:57:02 PDT
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...)
Comment 11 Corey Ford [:coyotebush] 2013-07-12 17:08:15 PDT
(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
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-07-12 17:27:44 PDT
(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.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-07-12 17:33:26 PDT
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 14 Corey Ford [:coyotebush] 2013-07-15 12:12:35 PDT
Comment on attachment 774360 [details] [diff] [review]
Refactor the application of CSS relative positioning.

Continuing work on this part in bug 893962.
Comment 15 Corey Ford [:coyotebush] 2013-07-18 16:38:16 PDT
Created attachment 778154 [details] [diff] [review]
Part 3: Compute sticky positioning offsets.

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.
Comment 16 Corey Ford [:coyotebush] 2013-07-18 18:42:08 PDT
Created attachment 778212 [details] [diff] [review]
Sticky positioning with nsStickyScrollingContainer object

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?
Comment 17 Corey Ford [:coyotebush] 2013-07-18 18:50:47 PDT
Created attachment 778216 [details]
simple testcase

This is what I've been using to manually test sticky positioning.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-07-18 19:06:22 PDT
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 19 Corey Ford [:coyotebush] 2013-07-19 11:21:15 PDT
Created attachment 778581 [details] [diff] [review]
Part 4: Compute sticky positioning offsets for getComputedStyle().
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-07-22 13:47:51 PDT
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 21 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-07-22 14:15:55 PDT
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.
Comment 22 Corey Ford [:coyotebush] 2013-07-22 18:13:51 PDT
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 ;)
Comment 23 Corey Ford [:coyotebush] 2013-07-22 18:19:16 PDT
(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.
Comment 24 Corey Ford [:coyotebush] 2013-07-31 16:09:04 PDT
Created attachment 784071 [details] [diff] [review]
Part 3 v2: Compute sticky positioning offsets.

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.
Comment 25 Corey Ford [:coyotebush] 2013-07-31 18:20:57 PDT
Created attachment 784130 [details] [diff] [review]
Part 4 v2: Compute sticky positioning offsets for getComputedStyle().

A similar update to these computations, plus a mochitest (currently in test_computed_style.html, but could go in its own file instead).
Comment 26 Corey Ford [:coyotebush] 2013-08-05 10:38:08 PDT
Created attachment 785844 [details] [diff] [review]
Part 1 v2: Support position:sticky in the CSS parser.

Rebased to account for nsStyleConsts.h move.
Comment 27 Corey Ford [:coyotebush] 2013-08-05 21:02:47 PDT
Created attachment 786115 [details] [diff] [review]
Part 1 v3: Support position:sticky in the CSS parser.

And another revision to include sticky in the property_database.js test file.
Comment 28 Corey Ford [:coyotebush] 2013-08-06 10:27:06 PDT
Created attachment 786368 [details] [diff] [review]
Part 5: Handle sticky positioning in RecomputePosition.
Comment 29 Corey Ford [:coyotebush] 2013-08-06 15:10:21 PDT
Created attachment 786530 [details] [diff] [review]
Part 3 v3: Compute sticky positioning offsets.

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 30 Corey Ford [:coyotebush] 2013-08-06 15:12:06 PDT
Created attachment 786532 [details] [diff] [review]
Part 5 v2: Handle sticky positioning in RecomputePosition.
Comment 31 Corey Ford [:coyotebush] 2013-08-06 16:00:54 PDT
Created attachment 786577 [details] [diff] [review]
Part 6: Always build a stacking context for sticky positioned elements.
Comment 32 Cameron McCormack (:heycam) (away Sep 28) 2013-08-06 17:25:36 PDT
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 33 Cameron McCormack (:heycam) (away Sep 28) 2013-08-06 17:25:54 PDT
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 34 Cameron McCormack (:heycam) (away Sep 28) 2013-08-06 18:18:43 PDT
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?
Comment 35 Corey Ford [:coyotebush] 2013-08-07 15:19:28 PDT
Created attachment 787158 [details] [diff] [review]
Part 1 v4: Support position:sticky in the CSS parser, enabled by a preference.

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 36 Daniel Holbert [:dholbert] 2013-08-07 15:27:57 PDT
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.]
Comment 37 Corey Ford [:coyotebush] 2013-08-07 16:20:41 PDT
Created attachment 787197 [details] [diff] [review]
Part 4 v3: Compute sticky positioning offsets for getComputedStyle().

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 38 Corey Ford [:coyotebush] 2013-08-07 17:09:31 PDT
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 39 Cameron McCormack (:heycam) (away Sep 28) 2013-08-07 18:30:20 PDT
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 40 Cameron McCormack (:heycam) (away Sep 28) 2013-08-07 18:41:39 PDT
Comment on attachment 787197 [details] [diff] [review]
Part 4 v3: Compute sticky positioning offsets for getComputedStyle().

This all looks good.  r=me
Comment 41 Corey Ford [:coyotebush] 2013-08-07 18:47:45 PDT
Created attachment 787263 [details] [diff] [review]
Part 1 v5: Support position:sticky in the CSS parser, enabled by a preference.

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.
Comment 42 Cameron McCormack (:heycam) (away Sep 28) 2013-08-07 18:57:11 PDT
(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.
Comment 43 Corey Ford [:coyotebush] 2013-08-08 09:52:02 PDT
*** Bug 719141 has been marked as a duplicate of this bug. ***
Comment 44 Daniel Holbert [:dholbert] 2013-08-08 13:34:53 PDT
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 45 Daniel Holbert [:dholbert] 2013-08-08 15:11:46 PDT
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.
Comment 46 Daniel Holbert [:dholbert] 2013-08-08 15:14:02 PDT
(Sorry, I forgot to delete that "// Store the offset" chunk before posting; no comments on that part.)
Comment 47 Corey Ford [:coyotebush] 2013-08-08 17:39:19 PDT
Created attachment 787869 [details] [diff] [review]
Part 1 v6: Support position:sticky in the CSS parser, enabled by a preference.

Here's a MOZ_ASSERT on the result on FindIndexOfKeyword.
Comment 48 Corey Ford [:coyotebush] 2013-08-08 18:42:08 PDT
Created attachment 787891 [details] [diff] [review]
Part 3 v4: Compute sticky positioning offsets for getComputedStyle().

Rebased onto this morning's m-c (a small context change), and renumbering this patch more sensibly (from 4 to 3).
Comment 49 Corey Ford [:coyotebush] 2013-08-08 18:43:53 PDT
Comment on attachment 787891 [details] [diff] [review]
Part 3 v4: Compute sticky positioning offsets for getComputedStyle().

Keeping heycam's r+ from "Part 4 v3".
Comment 50 Corey Ford [:coyotebush] 2013-08-08 18:54:56 PDT
Created attachment 787896 [details] [diff] [review]
Part 4 v4: Compute sticky positioning offsets.

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).
Comment 51 Corey Ford [:coyotebush] 2013-08-08 19:02:37 PDT
Created attachment 787901 [details] [diff] [review]
Part 5 v2: Always build a stacking context for sticky positioned elements.

Renumbering this one too (from 6).
Comment 52 Corey Ford [:coyotebush] 2013-08-08 19:07:19 PDT
Created attachment 787905 [details] [diff] [review]
Part 6: Implement sticky positioning, calculated on reflow and scroll.

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.
Comment 53 Corey Ford [:coyotebush] 2013-08-08 19:09:10 PDT
Created attachment 787908 [details] [diff] [review]
Part 7: Reftests for sticky positioning.

And here's a pile of tests, primarily for the code in part 6.
Comment 54 Corey Ford [:coyotebush] 2013-08-08 20:02:19 PDT
Created attachment 787931 [details] [diff] [review]
Part 3 v5: Compute sticky positioning offsets for getComputedStyle().

The code style and null checks from ComputeStickyOffsets are relevant here too.
Comment 55 Daniel Holbert [:dholbert] 2013-08-09 11:12:30 PDT
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 56 Daniel Holbert [:dholbert] 2013-08-09 11:46:35 PDT
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)
Comment 57 Corey Ford [:coyotebush] 2013-08-09 12:00:28 PDT
(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.
Comment 58 Daniel Holbert [:dholbert] 2013-08-09 12:20:32 PDT
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.)
Comment 59 Corey Ford [:coyotebush] 2013-08-09 15:24:46 PDT
Created attachment 788399 [details] [diff] [review]
Part 7 v2: Reftests for sticky positioning.

Updated per dholbert's comments. (Thanks!) The test file comments will hopefully make review easier.
Comment 60 Corey Ford [:coyotebush] 2013-08-12 14:27:13 PDT
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)
Comment 61 Corey Ford [:coyotebush] 2013-08-12 15:54:30 PDT
Created attachment 789224 [details] [diff] [review]
Part 3 v6: Compute sticky positioning offsets for getComputedStyle().

Fixed the GetNearestScrollableFrame calls, and updated the test to catch that error.
Comment 62 Corey Ford [:coyotebush] 2013-08-12 15:59:30 PDT
Created attachment 789231 [details] [diff] [review]
Part 6 v2: Implement sticky positioning, calculated on reflow and scroll.

Fixed the GetNearestScrollableFrame call and rebased on this morning's m-c.
Comment 63 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-12 16:45:36 PDT
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.)
Comment 64 Corey Ford [:coyotebush] 2013-08-12 16:59:41 PDT
(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
Comment 65 Corey Ford [:coyotebush] 2013-08-12 17:27:12 PDT
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.
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-08-12 22:51:58 PDT
(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.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-08-12 22:54:05 PDT
(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.
Comment 68 Corey Ford [:coyotebush] 2013-08-13 11:06:19 PDT
(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?
Comment 69 Corey Ford [:coyotebush] 2013-08-13 13:23:18 PDT
Or maybe I should instead be scheduling a nsChangeHint_UpdateOverflow restyle (though I guess it's not really a restyle happening here).
Comment 70 Corey Ford [:coyotebush] 2013-08-13 14:53:17 PDT
Created attachment 789839 [details] [diff] [review]
Part 6 v3: Implement sticky positioning, calculated on reflow and scroll.

Added OverflowChangedTracker::mSubtreeRoot so that my overflow updates only walk up as far as necessary.
Comment 71 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-13 18:37:14 PDT
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.
Comment 72 Corey Ford [:coyotebush] 2013-08-13 20:07:14 PDT
(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.
Comment 73 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-13 21:33:39 PDT
(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.
Comment 74 Gary [:streetwolf] 2013-08-14 12:52:11 PDT
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
Comment 75 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-14 13:53:58 PDT
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 76 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-15 11:16:40 PDT
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 77 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-15 11:25:03 PDT
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 78 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-08-15 11:26:31 PDT
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.
Comment 79 Corey Ford [:coyotebush] 2013-08-15 11:45:32 PDT
(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...
Comment 80 Corey Ford [:coyotebush] 2013-08-15 12:27:44 PDT
Created attachment 790898 [details] [diff] [review]
Part 4 v5: Compute sticky positioning offsets.

Addressed dholbert's comments on this.
Comment 81 Corey Ford [:coyotebush] 2013-08-15 12:39:16 PDT
Not foreseeing any especially substantive changes, then, let's see what Try thinks.
https://tbpl.mozilla.org/?tree=Try&rev=d1fa3347b5ac
Comment 82 Corey Ford [:coyotebush] 2013-08-15 15:39:08 PDT
Created attachment 791001 [details] [diff] [review]
Part 7 v3: Reftests for sticky positioning.

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 83 Corey Ford [:coyotebush] 2013-08-15 18:04:45 PDT
Comment on attachment 791001 [details] [diff] [review]
Part 7 v3: Reftests for sticky positioning.

... and carrying forward r+
Comment 84 Corey Ford [:coyotebush] 2013-08-15 18:28:30 PDT
Created attachment 791059 [details] [diff] [review]
Part 3 v7: Compute sticky positioning offsets for getComputedStyle().

Added fuzzy equality to the mochitest to fix the Windows test errors.
Comment 85 Corey Ford [:coyotebush] 2013-08-15 18:30:34 PDT
Created attachment 791060 [details] [diff] [review]
Part 4 v6: Compute sticky positioning offsets.

Changed back to using IsRelativelyPositioned to fix the reftests/svg/text/ignore-position.svg failure, and added position:sticky to that test case.
Comment 86 Corey Ford [:coyotebush] 2013-08-15 18:39:17 PDT
Created attachment 791063 [details] [diff] [review]
Part 6 v4: Implement sticky positioning, calculated on reflow and scroll.

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.
Comment 87 Corey Ford [:coyotebush] 2013-08-15 18:42:28 PDT
And a new Try push:
https://tbpl.mozilla.org/?tree=Try&rev=67fb6f825eb4
Comment 88 Corey Ford [:coyotebush] 2013-08-16 09:43:19 PDT
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.
Comment 89 Corey Ford [:coyotebush] 2013-08-19 16:46:50 PDT
Created attachment 792502 [details] [diff] [review]
Part 4 v7: Compute sticky positioning offsets.

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)?
Comment 90 Corey Ford [:coyotebush] 2013-08-19 16:50:21 PDT
Created attachment 792506 [details] [diff] [review]
Part 7 v4: Reftests for sticky positioning.

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.
Comment 91 Corey Ford [:coyotebush] 2013-08-19 16:57:16 PDT
Created attachment 792508 [details] [diff] [review]
Part 6 v5: Implement sticky positioning, calculated on reflow and scroll.

The containing block logic (the equations for |limit|) was varying degrees of wrong.
Comment 92 Corey Ford [:coyotebush] 2013-08-19 17:03:53 PDT
And let's Try again: https://tbpl.mozilla.org/?tree=Try&rev=4a6a2085af9a
Comment 93 Daniel Holbert [:dholbert] 2013-08-19 18:26:00 PDT
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.)
Comment 94 Corey Ford [:coyotebush] 2013-08-19 19:21:13 PDT
(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?)
Comment 95 Daniel Holbert [:dholbert] 2013-08-19 20:02:50 PDT
(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.
Comment 96 Corey Ford [:coyotebush] 2013-08-19 21:10:21 PDT
(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).
Comment 97 Corey Ford [:coyotebush] 2013-08-20 14:13:51 PDT
Created attachment 793095 [details] [diff] [review]
Part 4 v8: Compute sticky positioning offsets.

Don't compute offsets in nsHTMLReflowState::InitConstraints.
Comment 98 Corey Ford [:coyotebush] 2013-08-20 14:17:00 PDT
Created attachment 793099 [details] [diff] [review]
Part 6 v6: Implement sticky positioning, calculated on reflow and scroll.

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 99 Daniel Holbert [:dholbert] 2013-08-20 14:39:38 PDT
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.
Comment 100 Daniel Holbert [:dholbert] 2013-08-20 14:40:57 PDT
(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"' :)
Comment 101 Corey Ford [:coyotebush] 2013-08-20 14:58:00 PDT
(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.
Comment 102 Daniel Holbert [:dholbert] 2013-08-20 15:05:20 PDT
(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.
Comment 103 Corey Ford [:coyotebush] 2013-09-01 18:21:58 PDT
> > (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.
Comment 104 Corey Ford [:coyotebush] 2013-09-01 18:55:10 PDT
Created attachment 798356 [details] [diff] [review]
Part 6 v7: Implement sticky positioning, calculated on reflow and scroll.

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)?
Comment 105 Corey Ford [:coyotebush] 2013-09-03 09:16:35 PDT
Created attachment 798899 [details] [diff] [review]
Part 1 v7: Support position:sticky in the CSS parser, enabled by a preference.

Rebased this to account for changes from bug 825771.
Comment 106 Corey Ford [:coyotebush] 2013-09-03 09:32:52 PDT
Created attachment 798910 [details] [diff] [review]
Part 7 v5: Reftests for sticky positioning.

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.
Comment 107 Corey Ford [:coyotebush] 2013-09-03 11:34:07 PDT
And yet another Try push: https://tbpl.mozilla.org/?tree=Try&rev=b5bb7e1168c2
Comment 108 Daniel Holbert [:dholbert] 2013-09-03 18:53:02 PDT
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 109 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-09-05 10:20:17 PDT
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.
Comment 110 Corey Ford [:coyotebush] 2013-09-05 10:29:21 PDT
(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!
Comment 111 Corey Ford [:coyotebush] 2013-09-05 10:44:07 PDT
(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.
Comment 112 Corey Ford [:coyotebush] 2013-09-05 11:20:00 PDT
Created attachment 800276 [details] [diff] [review]
Part 6 v8: Implement sticky positioning, calculated on reflow and scroll.

Addressed dholbert's and dbaron's review comments.
Comment 113 Corey Ford [:coyotebush] 2013-09-05 11:25:40 PDT
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.)
Comment 114 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-09-05 12:45:18 PDT
(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.
Comment 115 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-09-05 12:46:10 PDT
(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.
Comment 116 Corey Ford [:coyotebush] 2013-09-05 13:30:02 PDT
(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?
Comment 117 Corey Ford [:coyotebush] 2013-09-05 13:49:20 PDT
Created attachment 800369 [details] [diff] [review]
Part 2 v2: Include sticky positioning in nsStyleDisplay::IsRelativelyPositionedStyle.

Very well. Re-uploading 2 and 5 with r=
Comment 118 Corey Ford [:coyotebush] 2013-09-05 13:50:52 PDT
Created attachment 800371 [details] [diff] [review]
Part 5 v3: Always build a stacking context for sticky positioned elements.
Comment 119 Corey Ford [:coyotebush] 2013-09-05 13:55:51 PDT
Created attachment 800373 [details] [diff] [review]
Part 6 v9: Implement sticky positioning, calculated on reflow and scroll.

Okay, improved that comment a bit. Now really ready for checkin.
Comment 120 Corey Ford [:coyotebush] 2013-09-05 15:47:35 PDT
Created attachment 800433 [details] [diff] [review]
Part 1 v8: Support position:sticky in the CSS parser, enabled by a preference.

Rebased this yet again.
Comment 122 Daniel Holbert [:dholbert] 2013-09-06 14:07:29 PDT
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
Comment 124 Corey Ford [:coyotebush] 2013-09-06 21:34:15 PDT
https://hg.mozilla.org/mozilla-central/rev/d75c702f3a43
Comment 125 Asa Dotzler [:asa] 2013-09-13 12:43:01 PDT
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?
Comment 126 Lukas Blakk [:lsblakk] use ?needinfo 2013-09-13 13:18:11 PDT
Note this in Aurora only - shouldn't carry to Beta notes.
Comment 127 Corey Ford [:coyotebush] 2013-09-13 14:39:00 PDT
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/
Comment 128 Matthew N. [:MattN] (In Taipei until Sep. 23) 2013-10-16 16:38:11 PDT
I started some documentation at https://developer.mozilla.org/en-US/docs/Web/CSS/position and pointed to the editor's draft on this topic: http://dev.w3.org/csswg/css-position/#sticky-positioning
Comment 129 sjw 2014-02-12 11:02:16 PST
The relnote shouldn't be in the beta notes, right?
See bug 902992 and bug 916315
Comment 130 Lukas Blakk [:lsblakk] use ?needinfo 2014-02-12 13:10:07 PST
(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.
Comment 131 Sebastian Zartner [:sebo] 2014-12-01 06:02:59 PST
I believe the documentation on this is already good enough now.

Sebastian

Note You need to log in before you can comment on or make changes to this bug.