async scrolling should be aware of position:sticky elements and keep them in a fixed position as the user scrolls.

From a brief discussion with mattwoodrow several weeks ago: <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: I probably won't look at async quite yet, but if you have pointers handy I can be reading things. <mattwoodrow> corey: Good choice :) Take a look at the callers of nsDisplayListBuilder::IsFixedItem i guess <mattwoodrow> but beware, it returns true for everything on platforms where we don't have async scrolling enabled yet (like all of desktop)

Discussion of the position:fixed work in bug 607417 and related.

As I recall, we want sticky positioned elements to always create their own layers. So, as much as I've figured out so far is that I probably want to extend nsDisplayFixedPosition (from bug 758620), then set some additional properties alongside Layer::mIsFixedPosition to indicate where to fix vs. scroll the layer. On the compositing side, I assume I can start by looking at the uses of Layer::GetIsFixedPosition. Anything else I should be thinking about here? :)

nsDisplayFixedPosition is more complex than you need, because position:fixed can't just create a single stacking context. Let's not use that. Instead clone nsDisplayOpacity, which forces everything into a single stacking context and builds a layer for that content, which is exactly what you want to do. Then you'll want to add a new layer property, an optional struct containing the data you need. (What is that data?) Then you'll need to pipe that data through the layers system until it reaches the AsyncPanZoomController.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4) > (What is that data?) I think this will be, for each direction the element is sticky (up to all four), a single scroll range where the layer should be fixed, and at what position relative to the scroll container. To also enable smooth async zooming, the positions would need to be relative to the corresponding edge, and would probably also need units information attached.

We shouldn't need units information. The offsets and scroll ranges should be in layer pixels.

I was thinking of percentage offsets, which for position:fixed we interpret in terms of the visible area of the page (displayport?) such that the offset in real-world units doesn't change while zooming. But that isn't handled smoothly during async zooming, so I suppose I won't worry about it here either.

I imagine I might want similar building logic to that from bug 769541? Nested, independently moving stickypos elements are completely valid, though (e.g. sticky-bottom inside sticky-top).

AsyncCompositionManager::AlignFixedLayersForAnchorPoint appears to be the important place to which to get that data.

So for the most part I think it's reasonable to treat sticky like fixed, then add in that extra data (which on further thought can probably just be scroll ranges, without the offsets). Which of these (that define each other in turn) ought to return true with regard to sticky items/layers? 1. Layer::GetIsFixedPosition 2. nsDisplayListBuilder::IsFixedItem 3. nsLayoutUtils::IsScrolledByRootContentDocumentDisplayportScrolling I'd say probably #1 and not #3, but then I'm not sure about #2.

1 and 2 but not 3 I think.

You'll want to rename things to IsFixedOrSticky

After some IRC discussion, it sounds like the way to go is to add Layer::mIsStickyPosition and just set that from nsDisplayStickyPosition::BuildLayer, and that I'll need to indicate what the stickiness is relative to (perhaps using nsLayoutUtils::FindIDFor).

This adds Layer::mIsStickyPosition, accepts it in AsyncCompositionManager::AlignFixedLayersForAnchorPoint and TransformShadowTree, and sets it in (the new) nsDisplayStickyPosition::BuildLayer. I *think* I'll want to keep the FixedLayerMargins and FixedPositionAnchor stuff, so factored out that part from nsDisplayFixedPosition. Next, working on calculating and storing those scroll ranges.

At the moment the only effect of this is to make sticky positioned elements act like position:static on mobile :( so I may have missed something. Poking at RenderTrace a bit.

Erm, wait, that was that I hadn't turned on my new preference. Now they do something interesting :D

Comment on attachment 788424 [details] [diff] [review] Implement a display item class and layer flag for position:sticky Review of attachment 788424 [details] [diff] [review]: ----------------------------------------------------------------- basically looks good. ::: gfx/layers/Layers.h @@ +843,5 @@ > + /** > + * CONSTRUCTION PHASE ONLY > + * A layer is "sticky position" when ??? it draws content from a content > + * (not chrome) document, the topmost content document has a root scrollframe > + * with a displayport, but the layer does not move when that displayport scrolls. Fix this comment!

I also want to figure out why I see "stuck" elements flickering on what seems to be each content update (it coincides with the "unstuck" ones updating their position, which is now what happens).

Since the |translation| we end up with in AlignFixedLayersForAnchorPoint is only temporary (in the sense that the transforms only deal with panning since the last content update), I'd need to access the actual scroll position to figure out whether sticky positioning applies. Maybe a better approach would be to adjust the sticky scroll ranges on each content update so they're relative to the current scroll position -- how could I update layer data from the StickyScrollContainer algorithm? (I'm also not certain a pair of rectangles will turn out to be the best way to represent the scroll ranges, but I can work with that for now.)

Just have BuildLayer build and modify the layer --- that gets run every time we paint/update the layer tree.

Oh, so it does :) And it looks like it ends up getting called after scroll listeners, so it may be simpler to just compute and hold onto the scroll ranges while I'm recomputing the position from my scroll listener, then move that over to the layer in BuildLayer.

I think I've worked out the arithmetic for those rectangles, and presumably I can just tweak |translation| here appropriately (am I working in layer pixels here?) http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#266 This similar bit of code doesn't seem necessary for Android. Is it used on B2G or something (so should I get a device to test that on)? http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#294 Speaking of testing, is there much potential for automated testing of this stuff?

Could either of you help with those questions? :)

(In reply to Corey Ford [:corey] from comment #22) > I think I've worked out the arithmetic for those rectangles, and presumably > I can just tweak |translation| here appropriately (am I working in layer > pixels here?) > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ > AsyncCompositionManager.cpp#266 I'm not sure exactly how you're going about your transformations, but that translation is in async transformed layer space, I think. If you want to apply an offset, you'll ultimately want to modify that translation, but you need to be careful what coordinate space you're in - that translation applies directly to the layer, so you need to take into account any transformations on that layer (which is what all the code above it is doing). I find the best thing to do is just to experiment with the values and see what changes. Not just this, but I find another useful thing to do is to bypass this function entirely and see what the layers would look like untransformed, to get a better picture of what the function is doing. > This similar bit of code doesn't seem necessary for Android. Is it used on > B2G or something (so should I get a device to test that on)? > http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent. > cpp#294 I'm not actually sure that anything at all uses that code anymore... If anything did, it'd be b2g, but I didn't think it did anymore. Possibly xul-fennec or metro uses it... It'd be great if you could test that, it'd be nice to rip out some unused code :) > Speaking of testing, is there much potential for automated testing of this > stuff? There is an independent gfx testing framework that you may be able to use for this - :BenWa would have more answers here, I'm afraid I don't know much about it.

(In reply to Chris Lord [:cwiiis] from comment #24) > (In reply to Corey Ford [:corey] from comment #22) > > I think I've worked out the arithmetic for those rectangles, and presumably > > I can just tweak |translation| here appropriately (am I working in layer > > pixels here?) > > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ > > AsyncCompositionManager.cpp#266 > > I'm not sure exactly how you're going about your transformations, but that > translation is in async transformed layer space, I think. If you want to > apply an offset, you'll ultimately want to modify that translation, but you > need to be careful what coordinate space you're in - that translation > applies directly to the layer, so you need to take into account any > transformations on that layer (which is what all the code above it is doing). So my thinking here was that given some transformations to the page that would cause a fixed position layer to move by a certain amount, I should be able to more or less intersect that translation vector with areas where a sticky positioned layer should "stick" to get the appropriate translation for it. I'll keep experimenting; that should reveal what units I need to be working in.

(In reply to Corey Ford [:corey] from comment #25) > (In reply to Chris Lord [:cwiiis] from comment #24) > > (In reply to Corey Ford [:corey] from comment #22) > > > I think I've worked out the arithmetic for those rectangles, and presumably > > > I can just tweak |translation| here appropriately (am I working in layer > > > pixels here?) > > > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ > > > AsyncCompositionManager.cpp#266 > > > > I'm not sure exactly how you're going about your transformations, but that > > translation is in async transformed layer space, I think. If you want to > > apply an offset, you'll ultimately want to modify that translation, but you > > need to be careful what coordinate space you're in - that translation > > applies directly to the layer, so you need to take into account any > > transformations on that layer (which is what all the code above it is doing). > > So my thinking here was that given some transformations to the page that > would cause a fixed position layer to move by a certain amount, I should be > able to more or less intersect that translation vector with areas where a > sticky positioned layer should "stick" to get the appropriate translation > for it. I'll keep experimenting; that should reveal what units I need to be > working in. This sounds good to me - I guess if you can work out the maximum and minimum translation relative to the original render, you should be able to clamp the translation due to async scrolling(?)

Basically yes -- there will be two regions (because an element can be both top+bottom sticky) in each dimension through which the sticky thing should stay fixed wrt the displayport. And if the element isn't "stuck" on the initial render it won't necessarily be inside either of those regions. The arithmetic there is taking a while to get right.

Cool, this basically does the trick! :D Still a fair bit of cleanup to do, and the calculations could probably be formulated a bit better. But I'll be on vacation and out of contact for the next week or so, so any feedback in the meantime would be great. Much of the computation in ComputeScrollRanges mirrors ComputePosition (see bug 886646 part 6), including all those sizes of frames as well as the topLimit, topStick, etc. equations, though I'm still not sure of a great way to get rid of that repetition. (I do still see some sort of flickering as I scroll the html5rocks demo page... http://html5-demos.appspot.com/static/css/sticky.html )

Comment on attachment 793076 [details] [diff] [review] Add layer data indicating exactly where a sticky element should act fixed Review of attachment 793076 [details] [diff] [review]: ----------------------------------------------------------------- Please split the patch up into -- Rect.h change -- Layers changes -- Layout changes ::: gfx/layers/Layers.h @@ +845,5 @@ > + * CONSTRUCTION PHASE ONLY > + * A layer is "sticky position" when it draws content from a content > + * (not chrome) document, the topmost content document has a root scrollframe > + * with a displayport, and, for certain ranges of scroll positions, the layer > + * does not move when that displayport scrolls. Seems like you copied most of this from the fixed-pos setter, and it doesn't apply anywmore. @@ +909,5 @@ > + * CONSTRUCTION PHASE ONLY > + * If a layer is "sticky position", the difference between these rectangles > + * determines an area within which the layer should remain in the same > + * position when compositing the layer tree with a transformation (such as > + * when asynchronously scrolling and zooming). The exact behavior should be explained in detail. Which layer are these rectangles relative to? The layer's parent, by the look of it, but that isn't going to be correct in general, e.g. if we have a position:sticky element inside an element with animated opacity inside a scrollable element --- the middle element will be the sticky element's parent layer. I think we probably need to indicate somehow which layer these coordinates are relative to, probably using the scrollID of the scrollable element's layer (assuming there is one; if the scrollable element doesn't have a layer, the position:sticky descendants shouldn't have one either becuase the scrollable element isn't being "actively scrolled"). @@ +911,5 @@ > + * determines an area within which the layer should remain in the same > + * position when compositing the layer tree with a transformation (such as > + * when asynchronously scrolling and zooming). > + */ > + void SetStickyScrollRanges(LayerRect aOuter, LayerRect aInner) const& ::: layout/base/nsDisplayList.cpp @@ +3198,5 @@ > + GetScrollPositionClampingScrollPortSize(); > + } > + > + SetFixedPositionLayerData(layer, scrollFrame, scrollFrameSize, presContext, > + aContainerParameters); Why do we need to set this? ::: layout/generic/StickyScrollContainer.h @@ +22,5 @@ > > namespace mozilla { > > +struct StickyScrollRanges { > + nsRect mOuter, mInner; Needs detailed documentation

(In reply to Corey Ford [:corey] (offline until ~08-28) from comment #28) > Much of the computation in ComputeScrollRanges mirrors ComputePosition (see > bug 886646 part 6), including all those sizes of frames as well as the > topLimit, topStick, etc. equations, though I'm still not sure of a great way > to get rid of that repetition. It occurred to me that I can probably do those |limit| and |stick| calculations all in a helper function.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Please split the patch up into > -- Rect.h change > -- Layers changes > -- Layout changes Will do. > ::: gfx/layers/Layers.h > @@ +845,5 @@ > > + * CONSTRUCTION PHASE ONLY > > + * A layer is "sticky position" when it draws content from a content > > + * (not chrome) document, the topmost content document has a root scrollframe > > + * with a displayport, and, for certain ranges of scroll positions, the layer > > + * does not move when that displayport scrolls. > > Seems like you copied most of this from the fixed-pos setter, and it doesn't > apply anywmore. Because the scrollable thing isn't necessarily the root scrollframe and displayport, right? Or is there more of this that doesn't apply for sticky? > @@ +909,5 @@ > > + * CONSTRUCTION PHASE ONLY > > + * If a layer is "sticky position", the difference between these rectangles > > + * determines an area within which the layer should remain in the same > > + * position when compositing the layer tree with a transformation (such as > > + * when asynchronously scrolling and zooming). > > The exact behavior should be explained in detail. I think this is a pretty succinct explanation, minus the way I treat dimensions independently, and the origin of the rectangles. > Which layer are these rectangles relative to? The layer's parent, by the > look of it, but that isn't going to be correct in general, e.g. if we have a > position:sticky element inside an element with animated opacity inside a > scrollable element --- the middle element will be the sticky element's > parent layer. > > I think we probably need to indicate somehow which layer these coordinates > are relative to, probably using the scrollID of the scrollable element's > layer (assuming there is one; if the scrollable element doesn't have a > layer, the position:sticky descendants shouldn't have one either becuase the > scrollable element isn't being "actively scrolled"). So these are relative to the layer's current position, in that they directly affect the translations that AlignFixedLayersForAnchorPoint might otherwise apply to that layer. I certainly haven't considered scrollable elements thoroughly yet, and I probably will need an indication of which scrollable thing. I think I tested with an overflow:scroll container, though, and it looked okay... Do I understand correctly that I should be seeing async scrolling in effect on overflow:scroll elements in Fennec? > > + SetFixedPositionLayerData(layer, scrollFrame, scrollFrameSize, presContext, > > + aContainerParameters); > > Why do we need to set this? Fixed position margins (for browser chrome and such) should affect sticky layers too, no? The fixed position anchor concept might not be useful for sticky layers, although at least it makes it easier to reuse existing calculations. > > +struct StickyScrollRanges { > > + nsRect mOuter, mInner; > > Needs detailed documentation I'm hesitant to write very similar documentation of the behavior of these rectangles in too many places. And since I only use this struct once (in passing data from layout to layers), perhaps I should replace it with a pair of outparams or something.

(In reply to Corey Ford [:corey] from comment #31) > I think I tested with an overflow:scroll container, though, and it looked > okay... Do I understand correctly that I should be seeing async scrolling in > effect on overflow:scroll elements in Fennec? No, async scrolling doesn't currently work anywhere for overflow:scroll elements, but work is in progress on b2g (and will be unified with fennec soon after). :kats and :botond are working on this. > > > > + SetFixedPositionLayerData(layer, scrollFrame, scrollFrameSize, presContext, > > > + aContainerParameters); > > > > Why do we need to set this? > > Fixed position margins (for browser chrome and such) should affect sticky > layers too, no? The fixed position anchor concept might not be useful for > sticky layers, although at least it makes it easier to reuse existing > calculations. They should, yes.

(In reply to Corey Ford [:corey] from comment #31) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > > ::: gfx/layers/Layers.h > > @@ +845,5 @@ > > > + * CONSTRUCTION PHASE ONLY > > > + * A layer is "sticky position" when it draws content from a content > > > + * (not chrome) document, the topmost content document has a root scrollframe > > > + * with a displayport, and, for certain ranges of scroll positions, the layer > > > + * does not move when that displayport scrolls. > > > > Seems like you copied most of this from the fixed-pos setter, and it doesn't > > apply anywmore. > > Because the scrollable thing isn't necessarily the root scrollframe and > displayport, right? Or is there more of this that doesn't apply for sticky? Right. Any scrollable element can be the scrollable container for position:sticky. > > @@ +909,5 @@ > > > + * CONSTRUCTION PHASE ONLY > > > + * If a layer is "sticky position", the difference between these rectangles > > > + * determines an area within which the layer should remain in the same > > > + * position when compositing the layer tree with a transformation (such as > > > + * when asynchronously scrolling and zooming). > > > > The exact behavior should be explained in detail. > > I think this is a pretty succinct explanation, minus the way I treat > dimensions independently, and the origin of the rectangles. Yes, that, plus what "determines an area within which" means. Does it mean if the top-left of the layer is in the difference, or if the entire layer outline lies in that difference, or something else? > I think I tested with an overflow:scroll container, though, and it looked > okay... Do I understand correctly that I should be seeing async scrolling in > effect on overflow:scroll elements in Fennec? Yes but I'm not sure if that's using APZC yet. See bug 775452. I think you should focus on position:sticky in the viewport case, since AFAIK APZC for scrollable elements is not working yet. > > > + SetFixedPositionLayerData(layer, scrollFrame, scrollFrameSize, presContext, > > > + aContainerParameters); > > > > Why do we need to set this? > > Fixed position margins (for browser chrome and such) should affect sticky > layers too, no? The fixed position anchor concept might not be useful for > sticky layers, although at least it makes it easier to reuse existing > calculations. I guess so. > > > +struct StickyScrollRanges { > > > + nsRect mOuter, mInner; > > > > Needs detailed documentation > > I'm hesitant to write very similar documentation of the behavior of these > rectangles in too many places. And since I only use this struct once (in > passing data from layout to layers), perhaps I should replace it with a pair > of outparams or something. Sure.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > > I think I tested with an overflow:scroll container, though, and it looked > > okay... Do I understand correctly that I should be seeing async scrolling in > > effect on overflow:scroll elements in Fennec? > > Yes but I'm not sure if that's using APZC yet. See bug 775452. > > I think you should focus on position:sticky in the viewport case, since > AFAIK APZC for scrollable elements is not working yet. Okay. Looking again at some of the code in AsyncCompositionManager, the fixed/sticky layer calculations are just based on a transform applied to the layer tree, so it might work more or less correctly with async subframe panning (perhaps just minus the fixedmargins part). But in a quick test sticky within an iframe doesn't quite work right currently.

Oh, I see, the problem is that fixed/sticky layers will try to stay in place when other ancestor scrollframes scroll. I think what you're suggesting is to tag each sticky layer with the ID of its scrollframe, then pass that ID along with async-pan transforms. Alternatively, how about having AlignFixedLayersForAnchorPoint stop recursing when it reaches another scrollframe? And I do need to make fixed margins only come into consideration for scrolling on the root displayport.

(In reply to Corey Ford [:corey] from comment #35) > Oh, I see, the problem is that fixed/sticky layers will try to stay in place > when other ancestor scrollframes scroll. I think what you're suggesting is > to tag each sticky layer with the ID of its scrollframe, then pass that ID > along with async-pan transforms. Yes. > Alternatively, how about having > AlignFixedLayersForAnchorPoint stop recursing when it reaches another > scrollframe? I suppose that would work too. And it would be simpler. OK. > And I do need to make fixed margins only come into consideration for > scrolling on the root displayport. OK.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > > > @@ +909,5 @@ > > > > + * CONSTRUCTION PHASE ONLY > > > > + * If a layer is "sticky position", the difference between these rectangles > > > > + * determines an area within which the layer should remain in the same > > > > + * position when compositing the layer tree with a transformation (such as > > > > + * when asynchronously scrolling and zooming). > > > > > > The exact behavior should be explained in detail. > > > > I think this is a pretty succinct explanation, minus the way I treat > > dimensions independently, and the origin of the rectangles. > > Yes, that, plus what "determines an area within which" means. Does it mean > if the top-left of the layer is in the difference, or if the entire layer > outline lies in that difference, or something else? So... that comment is proving tricky. Here's one possibility that's a bit improved on the above (though I haven't worked in the issue of *what* is in those intervals): * If a layer is "sticky position", the difference between these rectangles * defines two intervals in each dimension, relative to the layer's current * position, across which the layer should not move relative to its scroll * container when compositing the layer tree with a transformation (such as * when asynchronously scrolling and zooming) Or one that more literally describes how the rectangles are used: * If a layer is "sticky position", the difference between these rectangles * defines two intervals in each dimension. When compositing the layer tree * with a transformation (such as when asynchronously scrolling and zooming), * the intersection of these intervals with the respective translation * component of the inverse of the transformation determines a translation * applied to the layer itself, such that the layer does not move relative * to its scroll container where appropriate.

(In reply to Corey Ford [:corey] from comment #37) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > > > > @@ +909,5 @@ > > > > > + * CONSTRUCTION PHASE ONLY > > > > > + * If a layer is "sticky position", the difference between these rectangles > > > > > + * determines an area within which the layer should remain in the same > > > > > + * position when compositing the layer tree with a transformation (such as > > > > > + * when asynchronously scrolling and zooming). > > > > > > > > The exact behavior should be explained in detail. > > > > > > I think this is a pretty succinct explanation, minus the way I treat > > > dimensions independently, and the origin of the rectangles. > > > > Yes, that, plus what "determines an area within which" means. Does it mean > > if the top-left of the layer is in the difference, or if the entire layer > > outline lies in that difference, or something else? > > So... that comment is proving tricky. That's usually a good sign that the concept isn't well-defined or we don't fully understand it yet or it's too complicated. Or all of the above :-). > Here's one possibility that's a bit > improved on the above (though I haven't worked in the issue of *what* is in > those intervals): > > * If a layer is "sticky position", the difference between these rectangles > * defines two intervals in each dimension, relative to the layer's current > * position, across which the layer should not move relative to its scroll > * container when compositing the layer tree with a transformation (such as > * when asynchronously scrolling and zooming) > > Or one that more literally describes how the rectangles are used: > > * If a layer is "sticky position", the difference between these rectangles > * defines two intervals in each dimension. When compositing the layer tree > * with a transformation (such as when asynchronously scrolling and > zooming), > * the intersection of these intervals with the respective translation > * component of the inverse of the transformation determines a translation > * applied to the layer itself, such that the layer does not move relative > * to its scroll container where appropriate. Yes, neither of those are easy to understand :-). How about taking the first one and formalizing it with math?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > > Alternatively, how about having > > AlignFixedLayersForAnchorPoint stop recursing when it reaches another > > scrollframe? > > I suppose that would work too. And it would be simpler. OK. Oh, that looks like it wouldn't work because the fix for bug 818575 means we call TransformScrollableLayer on (and thus AlignFixedLayers starting with) every scrollframe anyway. And it also wouldn't do the right thing when a sub-scrollframe *doesn't* have its own layer (which seems to be happening in some tests I've constructed, but I'll keep experimenting with that).

Ah, right. Scrollframes that aren't for the viewport and haven't been scrolled recently won't get layers created for them. Then again, position:sticky doesn't need APZC support in that case. Maybe we should only create layers for position:sticky elements when their nearest scrollable frame is active? nsIScrollableFrame::IsScrollingActive.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > Ah, right. Scrollframes that aren't for the viewport and haven't been > scrolled recently won't get layers created for them. I see. Though I'm having trouble getting a scrollable layer created (in a simple iframe test). Layer trees tend to look like http://pastebin.mozilla.org/2973759 -- neither of the frames with scrollIds looks like the iframe. > Then again, > position:sticky doesn't need APZC support in that case. Maybe we should only > create layers for position:sticky elements when their nearest scrollable > frame is active? nsIScrollableFrame::IsScrollingActive. Yeah, that makes sense and was an easy enough tweak.

I think scroll position is probably the easiest way to think about the StickyScrollRanges; let me try the comment again. * If a layer is "sticky position", the difference between these rectangles * is treated as two intervals in each dimension. While the change to each * component of the scroll position lies within either of the respective * intervals, the layer should not move relative to its scrolling container, * when compositing the layer tree with a transformation (such as when * asynchronously scrolling and zooming). Does the idea make enough sense to you by now, at least, that you could point out specific parts of that description that are unclear?

I assume that conceptually the "change to the component of the scroll position" is a continuous function over time. Does that mean that the "change to the component of the scroll position" would normally progress smoothly while we're outside both of those intervals, then we'd hit one end of one of the intervals and halt there?

(In reply to Corey Ford [:corey] from comment #41) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > > Ah, right. Scrollframes that aren't for the viewport and haven't been > > scrolled recently won't get layers created for them. > > I see. Though I'm having trouble getting a scrollable layer created (in a > simple iframe test). Layer trees tend to look like > http://pastebin.mozilla.org/2973759 -- neither of the frames with scrollIds > looks like the iframe. The IFRAME doesn't seem to have a layer there. I'm not surprised, since APZC doesn't support scrolling of non-root documents on B2G yet AFAIK. Let's focus on position:sticky elements whose scrolling container is the root document viewport.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43) > I assume that conceptually the "change to the component of the scroll > position" is a continuous function over time. Does that mean that the > "change to the component of the scroll position" would normally progress > smoothly while we're outside both of those intervals, then we'd hit one end > of one of the intervals and halt there? Well, of course APZC is changing the scroll position (conceptually). But yes, the change to the sticky element's position wrt the scrollframe would progress smoothly while we're outside both those intervals. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44) > The IFRAME doesn't seem to have a layer there. I'm not surprised, since APZC > doesn't support scrolling of non-root documents on B2G yet AFAIK. Let's > focus on position:sticky elements whose scrolling container is the root > document viewport. Oh, I thought we had at least some kind of async subframe panning. If not, I guess I can't do a lot about supporting sticky layers inside other scrolling containers (which I thought you had been asking for). (In reply to Corey Ford [:corey] from comment #41) > > Then again, > > position:sticky doesn't need APZC support in that case. Maybe we should only > > create layers for position:sticky elements when their nearest scrollable > > frame is active? nsIScrollableFrame::IsScrollingActive. > > Yeah, that makes sense and was an easy enough tweak. Except that to make that example work, I guess I still need to tag things with scrollIds. (In that testcase, and with that change to whether layers are created, once I scroll the iframe the sticky element gets its own layer and starts behaving badly, even though as we see the iframe doesn't get APZC.)

Okay, cleared that up: 09:11 <kats> on Android APZC is not enabled, and subframe scrolling is entirely synchronous 09:11 <kats> on B2G you should be seeing a scrollable layer for iframes 09:11 <kats> for overflow:scroll elements on B2G if things work you should also see a scrollable layer, but it's broken - bug 898444 09:12 <kats> not that for "scrollable layer" i'm referring to either a nsDisplayScrollLayer or a nsDisplayScrollInfoLayer 09:12 <kats> s/not/note/

One significant issue still remains, which I see on Android but not B2G: sticky layers "flicker" on each content update (apparently). I might guess it has something to do with the asynchronous and synchronous parts of scrolling being out of sync with each other somehow. I'll try to make a recording of it tomorrow.

(as I noted in comment 18)

I put isStickyPosition above fixedAnchor/Margins since sticky positioned layers use those too.

This is the interesting part.

I assume BuildDisplayListForStackingContext is the right place in nsIFrame, though I'm not at all sure about the order within that method (relative to the opacity & transforms bits).

Leaving this here because I think it's probably correct, but as we don't currently seem to have any platforms with both async scrollable subframes and changing fixed position margins, I haven't tested.

Comment on attachment 801314 [details] [diff] [review] Part 3: Keep sticky position layers fixed during async panning, within certain scroll ranges. Review of attachment 801314 [details] [diff] [review]: ----------------------------------------------------------------- The AsyncCompositionManager parts look fine to me, but I don't think I know enough of this code/context to officially review it. ::: gfx/layers/composite/AsyncCompositionManager.cpp @@ +235,5 @@ > + !aLayer->GetParent()->GetIsFixedPosition()) || > + (aLayer->GetIsStickyPosition() && > + aTransformedSubtreeRoot->AsContainerLayer() && > + aLayer->GetStickyScrollContainerId() == aTransformedSubtreeRoot-> > + AsContainerLayer()->GetFrameMetrics().mScrollId))) { I would prefer to see this giant if condition broken up into smaller named conditions. e.g. bool isRootFixedPosition = (aLayer->GetIsFixedPosition() && !aLayer->GetParent()->GetIsFixedPosition()); bool isStickyForSubtree = aLayer->GetIsStickyPosition() && .... if (aLayer != aTransformedSubtreeRoot && (isRootFixedPosition || isStickyForSubtree)) {

Recordings and selected still frames of the flickering problem (evident on http://html5-demos.appspot.com/static/css/sticky.html on Android 4.3): http://people.mozilla.org/~cford/sticky-flicker/ If anyone has insights into what might be going on, that would be great.

Comment on attachment 801313 [details] [diff] [review] Part 2: Add layer fields for sticky positioning. Review of attachment 801313 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.h @@ +1338,5 @@ > LayerPoint mAnchor; > LayerMargin mMargins; > + FrameMetrics::ViewID mStickyScrollId; > + LayerRect mStickyOuter; > + LayerRect mStickyInner; Let's slim this down a bitby putting mStickyOuter/mStickyInner/mStickyScrollId into a struct that's only allocated for sticky-position layers (managed by nsAutoPtr). Then we can get rid of mIsStickyPosition too.

Comment on attachment 801314 [details] [diff] [review] Part 3: Keep sticky position layers fixed during async panning, within certain scroll ranges. Review of attachment 801314 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/StickyScrollContainer.cpp @@ +221,5 @@ > + nsRect* aInner) const > +{ > + nsRect stick; > + nsRect contain; > + ComputeStickyLimits(aFrame, &stick, &contain); The comment for ComputeStickyLimits makes lots of sense to me. Why don't we get rid of GetScrollRanges and just use ComputeStickyLimits instead of defining a new way to represent the sticky region with two rectangles?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56) > Let's slim this down a bitby putting > mStickyOuter/mStickyInner/mStickyScrollId into a struct that's only > allocated for sticky-position layers (managed by nsAutoPtr). Then we can get > rid of mIsStickyPosition too. Okay. Can that pointer go through IPDL-world directly, or do I need to copy the fields on one end or the other?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57) > The comment for ComputeStickyLimits makes lots of sense to me. > > Why don't we get rid of GetScrollRanges and just use ComputeStickyLimits > instead of defining a new way to represent the sticky region with two > rectangles? Hmm. I like the thought, but they're fairly different pairs of rectangles -- StickyLimits are in the sticky frame's parent's coordinate space and push the sticky frame's position around, while StickyScrollRanges are a function of the scroll position that determine whether the frame is acting fixed or not. It's definitely not obvious how to use the StickyLimits directly in AsyncCompositionManager, given that it knows nothing about frame coordinate spaces and the current scroll position.

(In reply to Corey Ford [:corey] from comment #58) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56) > > Let's slim this down a bitby putting > > mStickyOuter/mStickyInner/mStickyScrollId into a struct that's only > > allocated for sticky-position layers (managed by nsAutoPtr). Then we can get > > rid of mIsStickyPosition too. > > Okay. Can that pointer go through IPDL-world directly, or do I need to copy > the fields on one end or the other? You'll have to do some marshaling yourself.

Comment on attachment 801315 [details] [diff] [review] Part 4: Build display items and layers for sticky positioned elements. Review of attachment 801315 [details] [diff] [review]: ----------------------------------------------------------------- Excellent! ::: layout/base/nsDisplayList.cpp @@ +3237,5 @@ > + > + StickyScrollContainer* stickyScrollContainer = StickyScrollContainer:: > + StickyScrollContainerForFrame(mFrame); > + nsIFrame* scrollFrame = do_QueryFrame(stickyScrollContainer->ScrollFrame()); > + nsPresContext *presContext = scrollFrame->PresContext(); * after nsPresContext @@ +3268,5 @@ > + aContainerParameters.mXScale, > + NSAppUnitsToFloatPixels(outer.width, factor) * > + aContainerParameters.mYScale, > + NSAppUnitsToFloatPixels(outer.height, factor) * > + aContainerParameters.mXScale); We really need a shorthand for this, don't we? Let's do that in another bug.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #61) > @@ +3268,5 @@ > > + aContainerParameters.mXScale, > > + NSAppUnitsToFloatPixels(outer.width, factor) * > > + aContainerParameters.mYScale, > > + NSAppUnitsToFloatPixels(outer.height, factor) * > > + aContainerParameters.mXScale); > > We really need a shorthand for this, don't we? Let's do that in another bug. Yeah, especially since I just noticed I got the X/Y scales backwards there.

er, wrong bug.

I had thought nsIPresShell::MarkFixedFramesForReflow might need to look at sticky frames, but now I think not, because sticky elements' sizes don't depend on the viewport like fixed frames' do.

Adding r=roc

Something more like this?

Reformulated that unwieldy if condition per comment 54.

Some try runs from the last day: https://tbpl.mozilla.org/?tree=Try&rev=4878197fb4fe https://tbpl.mozilla.org/?tree=Try&rev=4c56eaa8c61b

Updated to reflect the changes in part 2 and from bug 914891, plus added Android fuzziness to a few of my sticky reftests.

I think that's it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b69bc8e40dd https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb106e846bb https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf8a3e61e3f https://hg.mozilla.org/integration/mozilla-inbound/rev/cd56b1bd18e3

Landed a followup to increase cd56b1bd18e3's fuzz-tweak slightly, for a recent inbound orange: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4a80f8e1df

https://hg.mozilla.org/mozilla-central/rev/6b69bc8e40dd https://hg.mozilla.org/mozilla-central/rev/7bb106e846bb https://hg.mozilla.org/mozilla-central/rev/fdf8a3e61e3f https://hg.mozilla.org/mozilla-central/rev/cd56b1bd18e3 https://hg.mozilla.org/mozilla-central/rev/0d4a80f8e1df