Closed Bug 904197 Opened 11 years ago Closed 11 years ago

Handle sticky positioning where the element or its containing-block element have multiple frames

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: coyotebush, Assigned: coyotebush)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

Use the union of continuations' rects in sticky positioning calculations.
22.81 KB, patch
Details | Diff | Splinter Review 
My implementation of sticky positioning in bug 886646 computes position on a frame-by-frame basis. The result is that multiple frames of, for instance, sticky positioned inline elements won't always be in the same position relative to each other.

While this might be sensible behavior, WebKit appears to use the bounding box/union of an element's frames in sticky positioning calculations. I haven't investigated what it does when the nearest block-level ancestor (which establishes the containing block) has multiple frames itself (e.g. with columns), but that might also need special handling.
So, I think WebKit's behavior probably makes more sense here, and it shouldn't be hard to implement (find the bounding box, sticky position that, position the frame relative to the bounding box).

I briefly tested what WebKit does with a (left-)sticky element inside a multi-column element: it kept the element stuck, though then rendered the second column on top of it. With CSS Regions also in mind, I'm inclined to say my behavior (which keeps it within the first column/fragment) might be more reasonable, though it wouldn't be too hard to take the bounding box there too.

But I'd appreciate more opinions.
Flags: needinfo?(roc)
Flags: needinfo?(dbaron)
I would take the bounding box in all cases.
Flags: needinfo?(roc)
Yeah, using the bounding box makes sense.  If the sticky calculation is happening at the end of the scrolling container's reflow, that should be relatively straightforward since all the fragments should have their layout calculated by that point.  (I think it's probably safe to assume that scrollable containers can't fragment.)
Flags: needinfo?(dbaron)
Oh, but I can't quite just take the union of all continuations' rects as part of the positioning algorithm, because once I ApplyRelativePositioning to one frame (or at least, the topmost or leftmost frame) later frames will use its new position and get the bounding box wrong.

I can more or less get around that by using GetNormalPosition everywhere, though that gets messy. Ideally, it would be nice to compute the bounding box once before sticky positioning any of the element's frames, but given how we call ApplyRelativePositioning on frames independently it's not clear how to do that.
Tossed around ideas about this for a while with dholbert on IRC. Here's one that's sounding attractive, though perhaps ambitious...

If we can safely assume (1) that a sticky frame's nearest reflow root is no closer than its scroll container, then we can avoid applying sticky positioning during reflow of the frame itself (currently, via ApplyRelativePositioning) and leave it until scrollframe reflow, at which point it would be straightforward to do element-wise.

Then, if we could also assume (2) that whenever we move a frame as contemplated in bug 911786 comment 12, we move all its continuations by the same amount, we could route all those code paths through, say, a new nsIFrame::MovePosition(nsPoint aDistance). This would be a no-op on any sticky frame with a previous continuation, but on first continuations it would update normal positions of all continuations by the given amount, then apply sticky positioning to the lot. (We could optionally follow this for relative positioning too.)

Given the small number of things that are reflow roots, assumption (1) looks like it might be okay. I'll see whether assumption (2) looks reasonable as I look through the relevant code for bug 911786.

Does that approach (and the assumptions) sound reasonable?
Flags: needinfo?(dbaron)
(The approach I was initially working on went like this, but it would need GetNormalPosition in place of every GetPosition call (including, I believe, the ones involved in GetOffsetTo).)
(In reply to Corey Ford [:corey] from comment #5)
> we can avoid applying sticky positioning during reflow of the frame itself (currently, via ApplyRelativePositioning)

Though I note that several of the current callers of ApplyRelativePositioning (such as nsLineLayout and nsBlockReflowState::FlowAndPlaceFloat) do a significant amount of work after calling it. RestyleManager::RecomputePosition seems to show that it's okay to adjust relative positioning effects later without worrying about those bits, though.
(In reply to Corey Ford [:corey] from comment #5)
> Tossed around ideas about this for a while with dholbert on IRC. Here's one
> that's sounding attractive, though perhaps ambitious...
> 
> If we can safely assume (1) that a sticky frame's nearest reflow root is no
> closer than its scroll container, then we can avoid applying sticky
> positioning during reflow of the frame itself (currently, via
> ApplyRelativePositioning) and leave it until scrollframe reflow, at which
> point it would be straightforward to do element-wise.

I think this assumption is fine.

> Then, if we could also assume (2) that whenever we move a frame as
> contemplated in bug 911786 comment 12, we move all its continuations by the
> same amount, we could route all those code paths through, say, a new
> nsIFrame::MovePosition(nsPoint aDistance). This would be a no-op on any
> sticky frame with a previous continuation, but on first continuations it
> would update normal positions of all continuations by the given amount, then
> apply sticky positioning to the lot. (We could optionally follow this for
> relative positioning too.)

I don't think this assumption is correct, but I also don't see why you need it.  In the cases where this stuff happens, I presume you'd also still be reflowing the scroll container, given that I think all of these cases happen inside of reflow code.

> Does that approach (and the assumptions) sound reasonable?

I think the approach sounds fine.
Flags: needinfo?(dbaron)
(In reply to Corey Ford [:corey] from comment #7)
> (In reply to Corey Ford [:corey] from comment #5)
> > we can avoid applying sticky positioning during reflow of the frame itself (currently, via ApplyRelativePositioning)
> 
> Though I note that several of the current callers of
> ApplyRelativePositioning (such as nsLineLayout and
> nsBlockReflowState::FlowAndPlaceFloat) do a significant amount of work after
> calling it. RestyleManager::RecomputePosition seems to show that it's okay
> to adjust relative positioning effects later without worrying about those
> bits, though.

Er, actually, that is a problem.

RecomputePosition also does updating of overflow areas.  If you took this approach, you'd need to do after-the-fact updates of overflow areas, which is rather inefficient.

Maybe it's best to do the sticky positioning *both* during reflow of the sticky element and during reflow of its scroll container?  Then you do the (somewhat inefficient, though not horrible) work to update overflow areas only if the second pass actually changes something, which should be rare.
(In reply to David Baron [:dbaron] (needinfo? me) (busy through Sept. 14) from comment #8)
> I don't think this assumption is correct, but I also don't see why you need
> it.  In the cases where this stuff happens, I presume you'd also still be
> reflowing the scroll container, given that I think all of these cases happen
> inside of reflow code.

Oh, we took 

> there are cases where we move frames (knowing how much to move them) without doing reflow.

to mean we might do this outside of reflow entirely. But if not, maybe I can not bother with sticky positioning in these situations at all.

(In reply to David Baron [:dbaron] (needinfo? me) (busy through Sept. 14) from comment #9)
> RecomputePosition also does updating of overflow areas.  If you took this
> approach, you'd need to do after-the-fact updates of overflow areas, which
> is rather inefficient.

Ah, right.

> Maybe it's best to do the sticky positioning *both* during reflow of the
> sticky element and during reflow of its scroll container?  Then you do the
> (somewhat inefficient, though not horrible) work to update overflow areas
> only if the second pass actually changes something, which should be rare.

That's what I currently do; the challenge is adapting reflow positioning to operate on an entire sticky element at once.

One more uncertain assumption that came up yesterday: can I count on the frame-moving codepaths, and reflow in general, touching the first continuation before any others, or the last continuation after any others, or at least always reaching all continuations? (Although given the above, I'm not currently sure how that would help much...)
Ultimately, I think the problem is that to sticky position the element as a whole, it has to happen after *all* the frames have been (re)positioned, in which case an extra round of overflow updates seems pretty unavoidable.
(In reply to Corey Ford [:corey] from comment #10)
> (In reply to David Baron [:dbaron] (needinfo? me) (busy through Sept. 14)
> from comment #8)
> > I don't think this assumption is correct, but I also don't see why you need
> > it.  In the cases where this stuff happens, I presume you'd also still be
> > reflowing the scroll container, given that I think all of these cases happen
> > inside of reflow code.
> 
> Oh, we took 
> 
> > there are cases where we move frames (knowing how much to move them) without doing reflow.
> 
> to mean we might do this outside of reflow entirely. But if not, maybe I can
> not bother with sticky positioning in these situations at all.

I think what I meant was without doing reflow of the frame being moved, but still within reflow of an ancestor (typically parent).  I think RecomputePosition and scrolling are the only cases that happen outside reflow, though I might be missing something.

> One more uncertain assumption that came up yesterday: can I count on the
> frame-moving codepaths, and reflow in general, touching the first
> continuation before any others, or the last continuation after any others,
> or at least always reaching all continuations? (Although given the above,
> I'm not currently sure how that would help much...)

Hmmm.  You probably can count on it, but I'm not sure I'd want to count on it unless there's a particularly strong reason to want to (i.e., a lot of complexity that would result from not doing so).

(In reply to Corey Ford [:corey] from comment #11)
> Ultimately, I think the problem is that to sticky position the element as a
> whole, it has to happen after *all* the frames have been (re)positioned, in
> which case an extra round of overflow updates seems pretty unavoidable.

Having multiple frames is an unusual case; with only one frame it's not needed.
(In reply to David Baron [:dbaron] (needinfo? me) (busy through Sept. 14) from comment #12)
> (In reply to Corey Ford [:corey] from comment #11)
> > Ultimately, I think the problem is that to sticky position the element as a
> > whole, it has to happen after *all* the frames have been (re)positioned, in
> > which case an extra round of overflow updates seems pretty unavoidable.
> 
> Having multiple frames is an unusual case; with only one frame it's not
> needed.

True. So maybe ApplyRelativePositioning (during reflow) can be a no-op for sticky frames *with continuations*. That, plus modifying RecomputePosition, might do it.
(I would use my continuations-bounding-box method in place of a nearly identical inline implementation from bug 435293, but that's in currently #ifdef'd off code and I'm not up for fixing the transform code's own problems with doing that during initial reflow.)
https://hg.mozilla.org/mozilla-central/file/f73bed2856a8/layout/base/nsDisplayList.cpp#l3531
So this is as far as I got before realizing I need to tweak some of the calculations to use, e.g. the bounding box of the margin boxes of all continuations, which is no longer the same as just the box inflated by the margin. Maybe GetContinuations{Margin,Content}BoundingRect too?
Attachment #801930 - Attachment is obsolete: true
Attachment #803932 - Flags: feedback?(dholbert)
(for instance, the inline-3.html test doesn't pass with this code -- but it's OK in WebKit)
Comment on attachment 803932 [details] [diff] [review]
Use the bounding box of a sticky positioned element's frames in calculations.

>+nsRect
>+nsLayoutUtils::GetContinuationsBoundingRect(nsIFrame* aFrame)
>+{
>+  nsRect result;
>+
>+  for (nsIFrame* f = aFrame->GetFirstContinuation(); f;
>+       f = f->GetNextContinuation()) {
>+    result.UnionRect(result, nsRect(f->GetOffsetTo(aFrame), f->GetSize()));
>+  }
>+
>+  return result;

Several optimizations/tweaks that I think would be worth making here:
 (1) Assert that aFrame->GetPrevContinuation is null, and mandate that this function be called on the first frame in a continuation chain? (That matches your usage of it right now.)
 (2) Add a fast-path for when aFrame has no next-continuation (the 99% situation), in which case we don't need any looping or GetOffsetTo() calls.
 (3) Invoke GetOffsetTo() with respect to the root frame, instead of with respect to aFrame; this avoids having to re-walk aFrame's parent chain (and build up all the offsets) for every single continuation.

(root frame = aFrame->PresContext()->GetPresShell()->GetRootFrame())

>+++ b/layout/reftests/position-sticky/inline-2.html
[...]
>+    <link rel="match" href="inline-1-ref.html">
That needs s/1-ref/2-ref/

(same for inline-3.html, with 3-ref)

(In reply to Corey Ford [:corey] from comment #15)
> Maybe GetContinuations{Margin,Content}BoundingRect too?

Do you actually need all of those, or would GetContinuationsMarginBoundingRect be sufficient?
Attachment #803932 - Flags: feedback?(dholbert) → feedback+
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 803932 [details] [diff] [review]
> Use the bounding box of a sticky positioned element's frames in calculations.
> Several optimizations/tweaks that I think would be worth making here:

Thanks.

> Do you actually need all of those, or would
> GetContinuationsMarginBoundingRect be sufficient?

The border box of the sticky element needs to stay inside the scrollframe, but the margin box of the sticky element needs to stay inside the content box of the containing-block element, so yes, it feels like I need all three of margin/border/content. :|
Maybe we could use one function and make it take an enum that tells us which box to calculate?
(In reply to Daniel Holbert [:dholbert] from comment #17)
>  (3) Invoke GetOffsetTo() with respect to the root frame, instead of with
> respect to aFrame; this avoids having to re-walk aFrame's parent chain (and
> build up all the offsets) for every single continuation.
> 
> (root frame = aFrame->PresContext()->GetPresShell()->GetRootFrame())

It would avoid that work, yes. However, if we're asserting that aFrame has no previous continuation we could just have:

>  nsRect result = aFrame->GetRect();
>  for (nsIFrame* f = aFrame->GetNextContinuation(); f;
>       f = f->GetNextContinuation()) {
>    result.UnionRect(result, nsRect(f->GetOffsetTo(aFrame), f->GetSize()));
>  }
>  return result;

but optimizing for GetOffsetTo while keeping the one-continuation case fast would complicate that a fair bit. Plus the function of the code would be less obvious, plus we have to worry at least a bit about having a null pres shell. So, I'm inclined to stick with the above formulation (plus logic for the different types of boxes).
Sounds good to me. We can always optimize the GetOffsetTo stuff later (at a possible cost of readability) if we need to.
This adds an nsIFrame::* parameter to GetAllInFlowRects and GetAllInFlowRectsUnion, replacing BoxToRect::GetRectFromFrameFun, and also adds a few more rect-getting methods to nsIFrame.

(Hmm, that went in an interesting direction. Will see whether this still looks sane in the morning.)
Comment on attachment 804269 [details] [diff] [review]
Part 1: Generalize nsLayoutUtils::GetAllInFlowRects to use arbitrary rects from a frame.

Let's save getting this precisely right for a followup.
Attachment #804269 - Attachment is obsolete: true
This should basically resemble the previous patch, except that I'm not redefining GetAllInFlowRectsUnion.

Reftest inline-3.html fails because handling the sticky element's margin correctly will take more work; similarly, border/padding on containing-block continuations won't be handled quite right. Reftest column-contain-1a fails because some of the anonymous blocks inside an nsColumnSetFrame have 0 height.
Attachment #804694 - Flags: review?(dholbert)
Attachment #803932 - Attachment is obsolete: true
Blocks: 916302
Comment on attachment 804694 [details] [diff] [review]
Use the union of continuations' rects in sticky positioning calculations.


>diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp
>     // Move the frame
>     if (display->mPosition == NS_STYLE_POSITION_STICKY) {
>-      StickyScrollContainer::ComputeStickyOffsets(aFrame);
>+      if (!aFrame->GetPrevContinuation()) {
>+        StickyScrollContainer::ComputeStickyOffsets(aFrame);
>+        StickyScrollContainer* ssc =
>+          StickyScrollContainer::GetStickyScrollContainerForFrame(aFrame);
>+        if (ssc) {
>+          ssc->PositionContinuations(aFrame);
>+        }
>+      }

An explanatory comment would be handy here.

E.g. "For fragmented frames, we update sticky positioning for the whole continuation chain in one go, during the RecomputePosition() call for the first frame in the chain. So, we can skip this code for later frames in the continuation chain."
>     } else {
>       MOZ_ASSERT(NS_STYLE_POSITION_RELATIVE == display->mPosition,
>                  "Unexpected type of positioning");
[...]
>+      aFrame->SetPosition(aFrame->GetNormalPosition() +
>+                          nsPoint(newOffsets.left, newOffsets.top));
>     }
> 
>-    nsPoint position = aFrame->GetNormalPosition();
>-
>-    // This handles both relative and sticky positioning.
>-    nsHTMLReflowState::ApplyRelativePositioning(aFrame, newOffsets, &position);
>-    aFrame->SetPosition(position);

Rather than condensing to a "SetPosition()" all-in-one call, we should just shift the removed code up.  In particular, we need the ApplyRelativePositioning call; otherwise, we won't keep the Normal Position up to date for relatively positioned stuff.  (ApplyRelativePositioning takes care of that.)

>+    aContain->MoveBy(-aFrame->GetParent()->GetOffsetTo(cbFrame));
>+    aContain->Deflate(cbFrame->GetUsedBorderAndPadding() +
>+                      aFrame->GetUsedMargin() +
>+                      nsMargin(0, rect.width, rect.height, 0));

I think this would be marginally (forgive the pun) more readable as 3 separate "Deflate()" calls.

> void
>+StickyScrollContainer::PositionContinuations(nsIFrame* aFrame)
>+{

Two things:
 (a) Assert that aFrame->GetPrevContinuation() is null, to make it less likely that someone adds another caller down the line that accidentally invokes this on an intermediate continuation.
 (b) This doesn't update the NormalPosition of these frames (where previously, we would, for at least the first continuation, via ApplyRelativePositioning). Is that a problem?

>+void
> StickyScrollContainer::UpdatePositions(nsPoint aScrollPosition,
>                                        nsIFrame* aSubtreeRoot)
> {
[...]
>+      oct.AddFrame(f);
>+
>+      for (nsIFrame* cont = f->GetNextContinuation(); cont;
>+           cont = cont->GetNextContinuation()) {
>+        oct.AddFrame(cont);
>+      }

Just collapse the first AddFrame into the loop, starting "cont" at f instead of f->GetNextContinuation().

>+fails == inline-3.html inline-3-ref.html 

As noted in IRC, add # bug 916302 at the end of this line.

Probably r=me with that, though I'll hold off for the moment until I understand the NormalPosition situation better, per "(b)" above.
(In reply to Daniel Holbert [:dholbert] from comment #25)
> > void
> >+StickyScrollContainer::PositionContinuations(nsIFrame* aFrame)
> >+{
> 
> Two things:
>  (a) Assert that aFrame->GetPrevContinuation() is null, to make it less
> likely that someone adds another caller down the line that accidentally
> invokes this on an intermediate continuation.

PositionContinuations calls ComputePosition calls ComputeStickyLimits, which asserts that, so it would be redundant here.

>  (b) This doesn't update the NormalPosition of these frames (where
> previously, we would, for at least the first continuation, via
> ApplyRelativePositioning). Is that a problem?

As we just discussed, no, because calling ApplyRelativePositioning from RecomputePosition doesn't change the stored NormalPosition; I'll add a comment to justify my optimization of RecomputePosition in the position:relative case.
Comment on attachment 804694 [details] [diff] [review]
Use the union of continuations' rects in sticky positioning calculations.

OK, r=me with comment 25 & with a comment in RestyleManager explaining what you just told me, regarding why we don't bother calling ApplyRelativePositioning in RescomputePosition (& updating normal position) anymore.

(because we only hit that code if something like e.g. "top" has been tweaked, which will affect the actual position but won't affect the normal position; so we can skip the NormalPosition updates)
Attachment #804694 - Flags: review?(dholbert) → review+
Attachment #804694 - Attachment is obsolete: true
Thanks for addressing those! Sorry, I didn't see comment 26 when posting comment 27 -- one other point:

(In reply to Corey Ford [:corey] from comment #26)
> >  (a) Assert that aFrame->GetPrevContinuation() is null, to make it less
> > likely that someone adds another caller down the line that accidentally
> > invokes this on an intermediate continuation.
> 
> PositionContinuations calls ComputePosition calls ComputeStickyLimits, which
> asserts that, so it would be redundant here.

That's OK -- it's still worth adding the assertion.

It may be functionally redundant right now, but that might change (e.g. if we tweak how it works), and even if that doesn't happen, it's useful for documentation purposes.  (So that you don't have to read all of the functions that PositionContinuations calls, and all functions that they call, to find out if we ever sanity-check PositionContinuations's argument. And so that someone adding a new call to PositionContinuations will know about this assumption just from glancing at the impl.)

Assertions don't slow down release builds, and it won't clutter up the code readability-wise (makes the function easier to reason about, if anything), so please do add it.
Attachment #804766 - Attachment is obsolete: true
Attachment #804769 - Attachment description: Use the union of continuations' rects in sticky positioning calculations. inline-3.html fails because handling the sticky element's margin correctly will take more work; similarly, border/padding on containing-block continuations won't be handled quite r → Use the union of continuations' rects in sticky positioning calculations.
Attachment #804769 - Flags: review+
Thanks! Corey triggered Try run including this patch: https://tbpl.mozilla.org/?tree=Try&rev=c29ac5d62b81

[needinfo=me to make sure this gets landed after the Try run completes.]
Flags: needinfo?(dholbert)
Comment on attachment 804769 [details] [diff] [review]
Use the union of continuations' rects in sticky positioning calculations.

>+void
> StickyScrollContainer::UpdatePositions(nsPoint aScrollPosition,
>                                        nsIFrame* aSubtreeRoot)
[...]

>   for (nsTArray<nsIFrame*>::size_type i = 0; i < mFrames.Length(); i++) {
>     nsIFrame* f = mFrames[i];
[...]
>+    // Only process first continuations here. (Filtering others out as we add
>+    // them to the list doesn't work correctly, because we do that during frame
>+    // construction).
>+    if (!f->GetPrevContinuation()) {

Sorry, one more followup question here:  Why can't we filter at frame-construction time? It looks like nsFrame::Init() is what adds to our list, and that has an argument "aPrevInFlow", and I'd bet we can just check that and *only* call ssc->AddFrame if it's null.

(Sorry for the late-breaking comment; I was just skimming nsFrame::Init() for a different bug and this comment came to mind.
Attachment #804769 - Attachment is obsolete: true
Lingering issues with those inline tests, unfortunately.
Try run, with a followup to make the inline-* tests use the Ahem font (which fixes the pixel-alignment issue locally for me): https://tbpl.mozilla.org/?tree=Try&rev=4e4b09d884ce
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
Landed with ahem-font followup to make the tests pass:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a532a78c5671
 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3db70c3f1e
Flags: needinfo?(dholbert) → in-testsuite+
Depends on: 918994
FWIW, this bitrotted my patches in bug 828312, and as a result I noticed that this doesn't handle block-in-inline splits correctly, and finishing bug 828312 requires fixing that.  So I'll fix it there.
Depends on: 926728
Depends on: 1250249
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: