Closed Bug 916302 Opened 11 years ago Closed 11 years ago

correctly account for margin/border/padding on multiple-frame elements for sticky positioning

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: coyotebush, Assigned: coyotebush)

References

Details

Attachments

(2 files, 6 obsolete files)

8.06 KB, patch
Details | Diff | Splinter Review
10.59 KB, patch
Details | Diff | Splinter Review
In short, (union of border rects) + (one frame's margin) != (union of margin rects) in general. So the sticky positioning calculations should not use |cbFrame->GetUsedBorderAndPadding()| and |aFrame->GetUsedMargin()|.

In bug 904197 I added the test layout/reftests/position-sticky/inline-3.html as one that currently fails because of this.

This would require generalizing nsLayoutUtils::GetAllInFlowRectsUnion to handle boxes other than frame border boxes. One approach to this is attached (though moving the RectGetters to just be static methods of nsLayoutUtils is probably simpler).
Blocks: 916315
Assignee: nobody → corey
A variant of the previous patch that actually works. (And, the sticky-specific part is coming along nicely.)

This adds an nsIFrame::* parameter to GetAllInFlowRects and GetAllInFlowRectsUnion, replacing BoxToRect::GetRectFromFrameFun, and also adds a few more rect-getting methods to nsIFrame.

Does this look like a good way to handle the rect-getter functions? We could instead add the various RectRelativeToSelf methods to nsLayoutUtils, which would make it possible to specify a default for the parameter to GetAllInFlowRects. But I'm inclined to use the ones that are already in nsIFrame and just complete that suite of methods.

(I'll wrap lines where necessary on those existing calls I'm changing, naturally.)
Attachment #804698 - Attachment is obsolete: true
Attachment #829827 - Flags: feedback?(dholbert)
Attachment #829827 - Attachment is patch: true
Comment on attachment 829827 [details] [diff] [review]
Part 1: Generalize nsLayoutUtils::GetAllInFlowRects to use arbitrary rects from a frame.

This seems reasonable to me! feedback+. :)

A few notes:

>Bug 904197 - Part 1: Generalize nsLayoutUtils::GetAllInFlowRects to use arbitrary rects from a frame.

Nit: "arbitrary rects" sounds maybe a little too wild-west for what this is actually doing. :)

Maybe "...to support other types of frame rects beyond just the border rect"?

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -2445,55 +2445,49 @@ nsLayoutUtils::GetAllInFlowBoxes(nsIFram
[...]
> struct BoxToRect : public nsLayoutUtils::BoxCallback {
>   nsIFrame* mRelativeTo;
>   nsLayoutUtils::RectCallback* mCallback;
>   uint32_t mFlags;
>-  GetRectFromFrameFun mRectFromFrame;
>+  nsLayoutUtils::RectGetter mRectGetter;
> 
>   BoxToRect(nsIFrame* aRelativeTo, nsLayoutUtils::RectCallback* aCallback,
>-            uint32_t aFlags, GetRectFromFrameFun aRectFromFrame)
>+            uint32_t aFlags, nsLayoutUtils::RectGetter aRectGetter)
>     : mRelativeTo(aRelativeTo), mCallback(aCallback), mFlags(aFlags),
>-      mRectFromFrame(aRectFromFrame) {}
>+      mRectGetter(aRectGetter) {}

Nit: In this code above, mRectGetter should be listed *between* mCallback and mFlags, for consistency with the arg-order in the public GetAllInFlowRects/GetAllInFlowRectsUnion functions.
(similar for "aRectGetter" in the constructor function-signature)

(mRectFromFrame is last in the old code because it's an implementation detail, not exposed in any public function signature. But now that you're replacing it with something public, we should keep its position consistent with the order in the public function.)


Also: Please add a MOZ_ASSERT to the BoxToRect constructor (that seems to be the place that makes the most sense), along these lines...
    MOZ_ASSERT(&nsIFrame::GetMarginRectRelativeToSelf == mRectGetter ||
               &nsIFrame::GetPaddingRectRelativeToSelf == mRectGetter ||
               &nsIFrame::GetRectRelativeToSelf == mRectGetter ||
               &nsIFrame::GetContentRectRelativeToSelf == mRectGetter);

...to enforce that the rect-getter function we're passing in one of the whitelisted ones that we're expecting. (per your comment in nsLayoutUtils.h)

>+++ b/layout/generic/nsFrame.cpp
[...]
>+nsRect
>+nsIFrame::GetMarginRectRelativeToSelf() const
>+{
>+  nsMargin m = GetUsedMargin();
>+  ApplySkipSides(m);
>+  nsRect r(0, 0, mRect.width, mRect.height);

Observation, for a followup patch: this "r" declaration (as well as the equivalent line in the other similar functions) could take advantage of the GetRectRelativeToSelf() function that you're adding, to be slightly more concise and foolproof against typos. It could just be:
  nsRect r(GetRectRelativeToSelf());
Attachment #829827 - Flags: feedback?(dholbert) → feedback+
Attachment #829827 - Attachment is obsolete: true
(Looks like there's a reftest failure from some sub-pixel positioning differences.)
I'll do another run on all/most platforms to figure out the right annotations for that test.
Part 2 drops one "FIXME (Bug 920688)" comment, but leaves behind another, here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/StickyScrollContainer.cpp?rev=8ba2dc63f1bf#147

Is that part fixed by this patch, or does it need its own additional patch/bug?
Comment on attachment 831622 [details] [diff] [review]
Part 2: In sticky positioning calculations, use the union of the element's margin boxes and the union of the containing-block element's content boxes.

>+++ b/layout/generic/StickyScrollContainer.cpp
>@@ -155,26 +155,22 @@ StickyScrollContainer::ComputeStickyLimi
>   nsRect rect =
>     nsLayoutUtils::GetAllInFlowRectsUnion(aFrame, aFrame->GetParent(),
>                                           &nsIFrame::GetRectRelativeToSelf);
> 
>   // Containing block limits
>   if (cbFrame != scrolledFrame) {
>+    *aContain = nsLayoutUtils::
>+      GetAllInFlowRectsUnion(cbFrame, aFrame->GetParent(),
>+                             &nsIFrame::GetContentRectRelativeToSelf);
>+    aContain->Deflate(nsLayoutUtils::
>+      GetAllInFlowRectsUnion(aFrame, aFrame->GetParent(),
>+                             &nsIFrame::GetMarginRectRelativeToSelf) - rect);
>     aContain->Deflate(nsMargin(0, rect.width, rect.height, 0));
>   }

Could you add a code comment here to explain what's going on?

(things that'd be worth clarifying: why we're using the getter for [border-]rect vs. content-rect vs. margin-rect of these various frames, why we're using aFrame->GetParent() as our coordinate system, and just a high-level overview of what we're trying to do with these various rect-unions)
Comment on attachment 831606 [details] [diff] [review]
Part 1: Generalize nsLayoutUtils::GetAllInFlowRects to support other types of frame rects beyond border rects.

I wonder whether it'd be cleaner to make this a 

>+++ b/layout/base/nsLayoutUtils.cpp
>+    MOZ_ASSERT(&nsIFrame::GetMarginRectRelativeToSelf == mRectGetter ||
>+               &nsIFrame::GetPaddingRectRelativeToSelf == mRectGetter ||
>+               &nsIFrame::GetRectRelativeToSelf == mRectGetter ||
>+               &nsIFrame::GetContentRectRelativeToSelf == mRectGetter);
>+  }

Please swap the two middle lines there, so that the order makes more sense (margin > border > padding > content).

(That's my fault; I listed these in the wrong order in Comment 2. Sorry about that!)

r=me on part 1 with that.
Attachment #831606 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #9)
> I wonder whether it'd be cleaner to make this a 

Sorry, had a half-written thought that I forgot to complete. :)

Meant to say: I wonder if it'd be cleaner to pass in an enum value instead of a function-pointer into these public methods -- and then nsLayoutUtils-internal code could switch() over that enum value & pick the correct nsIFrame function (/ function-pointer) to use for that box type?

That way, we could more sanely provide a default value for this new (enum) parameter, and avoid modifying most of the call-sites that you had to modify.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Part 2 drops one "FIXME (Bug 920688)" comment, but leaves behind another,
> here:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> StickyScrollContainer.cpp?rev=8ba2dc63f1bf#147
> 
> Is that part fixed by this patch, or does it need its own additional
> patch/bug?

I don't fully understand that FIXME, and haven't addressed it here. I figure bug 920688 can be morphed to cover just that issue instead of also this one.

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Could you add a code comment here to explain what's going on?

Good idea.

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (In reply to Daniel Holbert [:dholbert] from comment #9)
> > I wonder whether it'd be cleaner to make this a 
> 
> Sorry, had a half-written thought that I forgot to complete. :)
> 
> Meant to say: I wonder if it'd be cleaner to pass in an enum value instead
> of a function-pointer into these public methods -- and then
> nsLayoutUtils-internal code could switch() over that enum value & pick the
> correct nsIFrame function (/ function-pointer) to use for that box type?
> 
> That way, we could more sanely provide a default value for this new (enum)
> parameter, and avoid modifying most of the call-sites that you had to modify.

Maybe. I don't see an existing enum definition that could serve for that. But it could even be handled with an extra two bits of the existing aFlags parameter.
(In reply to Corey Ford [:corey] from comment #11)
> I don't fully understand that FIXME, and haven't addressed it here. I figure
> bug 920688 can be morphed to cover just that issue instead of also this one.

Ah, right -- never mind about that. I mistakenly thought that bug 920688 *was* this bug, and I was afraid we were leaving behind a straggling FIXME comment associated with a bug that's (soon) fixed.

> Maybe. I don't see an existing enum definition that could serve for that.

Yeah, I don't either. That's too bad.

> But it could even be handled with an extra two bits of the existing aFlags
> parameter.

It could, yeah... I think I like that idea. That would conveniently avoid any need to change *any* of the existing callers, which would be great.  And given that we have exactly four types of boxes, we can make very good use of those two bits (i.e. all configurations would be meaningful).

Would you be up for changing part 1 to use aFlags like that? (I'd prefer that to landing part 1 as-is & simplifying later, since it'll avoid creating unnecessary code-churn in all of the callers.)
(In reply to Daniel Holbert [:dholbert] from comment #12)
> That would conveniently avoid
> any need to change *any* of the existing callers, which would be great.

(I'm of course assuming that the "00" configuration of those two bits would correspond to the border-box. This is slightly out of order, since the border box is an intermediate box; but it's OK, because it's more important that we keep the default-behavior aFlags value at 0 IMHO.)
(Sorry for not having thought of this back in comment 2.)
Comment on attachment 831606 [details] [diff] [review]
Part 1: Generalize nsLayoutUtils::GetAllInFlowRects to support other types of frame rects beyond border rects.

[Canceling r+ on part 1, since (per comment 10 / end of comment 12) I think we can make this cleaner.]
Attachment #831606 - Flags: review+
Attachment #831622 - Flags: review?(dholbert)
More like this, then?

(I decided against some clever content+margin=padding reasoning in favor of just putting the enum values in order :))
Attachment #8338267 - Flags: review?(dholbert)
Attachment #831606 - Attachment is obsolete: true
And this patch updated for the new parameter convention, plus with a little more explanation per comment 8.
Attachment #8338268 - Flags: review?(dholbert)
Attachment #831622 - Attachment is obsolete: true
Comment on attachment 8338267 [details] [diff] [review]
Part 1: Generalize nsLayoutUtils::GetAllInFlowRects to support other types of frame rects beyond border rects.

This looks really good! Just a few clarifying nits.

>+++ b/layout/base/nsLayoutUtils.cpp
>+      switch (mFlags & nsLayoutUtils::RECTS_USE_MARGIN_BOX) {
>+        case nsLayoutUtils::RECTS_USE_CONTENT_BOX:

So, RECTS_USE_MARGIN_BOX is serving a double-purpose here -- it's an enum value, and it's also the mask that picks out the bits that we'll use for checking these enum values.

I'd rather use a more explicitly-named #define for the latter purpose.  [More details on this below, in my response to next patch-hunk]

>+++ b/layout/base/nsLayoutUtils.h
>   enum {
>-    RECTS_ACCOUNT_FOR_TRANSFORMS = 0x01
>+    RECTS_ACCOUNT_FOR_TRANSFORMS = 0x01,
>+    RECTS_USE_CONTENT_BOX = 0x02,
>+    RECTS_USE_PADDING_BOX = 0x04,
>+    RECTS_USE_MARGIN_BOX = 0x06
>   };

This is a little magic right now -- it requires some reasoning on the reader's part to reason about which of these values are single bits vs. bit-combinations, and which of these values can be meaningfully combined in |aFlags|.

How about something more like:
  enum {
    RECTS_ACCOUNT_FOR_TRANSFORMS = 1,
    // We reserve two bits for specifying which box to look up. 
    // RECTS_USE_BORDER_BOX: neither bit set (default; no explicit enum value)
    RECTS_USE_CONTENT_BOX = 1 << 1, // first bit set
    RECTS_USE_PADDING_BOX = 1 << 2, // second bit set
    RECTS_USE_MARGIN_BOX  =
      RECTS_USE_CONTENT_BOX & RECTS_USE_PADDING_BOX // both bits set
  };

  // Bitmask for selecting out the "which box" bits from a mask using
  // the enum values defined above. (This mask happens to match the value
  // for RECTS_USE_MARGIN_BOX, since that value has both bits set.) 
  #define RECTS_USE_WHICH_BOX_MASK RECTS_USE_MARGIN_BOX

>   /**
>-   * Collect all CSS border-boxes associated with aFrame and its
>+   * Collect all CSS boxes associated with aFrame and its

"all css boxes" gives the wrong impression here -- it sounds a little all-encompassing.

Maybe "Collect all CSS boxes of a particular type (content, padding, border, or margin)"

This will require a bit of comment-rewrapping, but I think it only affects the first ~3 lines of this comment; there's sort of a new paragraph after that point.

[...]
>+   * If aFlags includes one of RECTS_USE_CONTENT_BOX, RECTS_USE_PADDING_BOX,
>+   * or RECTS_USE_MARGIN_BOX, the corresponding type of box is used
>+   * instead of the border box.
>    */

"instead of the border box" doesn't quite make sense (since we haven't mentioned the border-box yet).
Replace with something like:
  "Otherwise (by default), the border-box will be used."

r=me with tweaks along those lines.
Attachment #8338267 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #19)
> How about something more like:
>   enum {
>     RECTS_ACCOUNT_FOR_TRANSFORMS = 1,
>     // We reserve two bits for specifying which box to look up. 
>     // RECTS_USE_BORDER_BOX: neither bit set (default; no explicit enum
> value)
>     RECTS_USE_CONTENT_BOX = 1 << 1, // first bit set
>     RECTS_USE_PADDING_BOX = 1 << 2, // second bit set
>     RECTS_USE_MARGIN_BOX  =
>       RECTS_USE_CONTENT_BOX & RECTS_USE_PADDING_BOX // both bits set
>   };

I'm not sure we need to be quite that explicit about which enum values correspond to which bits. But at least I'll note that we're using two bits and that border-box is the default.

>   // Bitmask for selecting out the "which box" bits from a mask using
>   // the enum values defined above. (This mask happens to match the value
>   // for RECTS_USE_MARGIN_BOX, since that value has both bits set.) 
>   #define RECTS_USE_WHICH_BOX_MASK RECTS_USE_MARGIN_BOX

This could live in the enum too, I think.
(In reply to Corey Ford [:corey] from comment #20)
> I'm not sure we need to be quite that explicit about which enum values
> correspond to which bits. But at least I'll note that we're using two bits
> and that border-box is the default.

Yeah -- I was only suggesting the "// first bit set" etc. comments to make it clearer why RECTS_USE_MARGIN_BOX is set as RECTS_USE_CONTENT_BOX & RECTS_USE_PADDING_BOX  (In particular, the reason is *not* that "margin = content + padding", which it might otherwise look like my sample-code was saying.)

> >   // Bitmask for selecting out the "which box" bits from a mask using
> >   // the enum values defined above. (This mask happens to match the value
> >   // for RECTS_USE_MARGIN_BOX, since that value has both bits set.) 
> >   #define RECTS_USE_WHICH_BOX_MASK RECTS_USE_MARGIN_BOX
> 
> This could live in the enum too, I think.

It probably could; I marginally prefer outside-the-enum, because that makes it clearer that it's not a flag (not something client code should be passing in) -- its a mask. But I could go either way.
Comment on attachment 8338268 [details] [diff] [review]
Part 2: In sticky positioning calculations, use the union of the element's margin boxes and the union of the containing-block element's content boxes.

>+++ b/layout/generic/StickyScrollContainer.cpp
[...]
>+    aContain->Deflate(nsLayoutUtils::
>+      GetAllInFlowRectsUnion(aFrame, aFrame->GetParent(),
>+                             nsLayoutUtils::RECTS_USE_MARGIN_BOX) - rect);

There's a lot going on in this one line, and (even with the comment) I had to stare at it for a while to figure out what it's doing.

Maybe that's just me :) but I think it'd be very helpful for readability/understandability to split this up a bit & add a bit more commenting.

Maybe:
 // Calculate the offset between aFrame's margin-box and its border-box
 // (or rather, the offset between the union of those types of boxes
 // for its continuation chain, if it has continuations)
 nsMargin frameMarginOffset = GetAllInFlowRectsUnion(aFrame, aFrame->GetParent(),
                               nsLayoutUtils::RECTS_USE_MARGIN_BOX) - rect;

 // Now we deflate aContain by that ^ offset, so that by constraining aFrame within
 // aContain, we'll be keeping aFrame's margin-box inside of the containing
 // block's content-box.
 aContain->Deflate(frameMarginOffset);


>     aContain->Deflate(nsMargin(0, rect.width, rect.height, 0));

Similarly, a comment would help explain what's going on here.

e.g.
 // Now, we further deflate aContain by the sticky frame's border-box size,
 // so that it's specifically constraining the frame's upper-left position.


>   // These limits are for the bounding box of aFrame's continuations. Convert
>   // to limits for aFrame itself.
>   aStick->MoveBy(aFrame->GetPosition() - rect.TopLeft());
>+  aContain->MoveBy(aFrame->GetPosition() - rect.TopLeft());

Maybe worth caching the result of this point-subtraction in a local nsPoint, and passing that to each function here, to avoid repeating work? 

r=me with something like the above
Attachment #8338268 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #19)
>     RECTS_USE_CONTENT_BOX = 1 << 1, // first bit set
>     RECTS_USE_PADDING_BOX = 1 << 2, // second bit set
>     RECTS_USE_MARGIN_BOX  =
>       RECTS_USE_CONTENT_BOX & RECTS_USE_PADDING_BOX // both bits set

I'm actually not convinced that this is clearer than 0x02 0x04 0x06, given that, as you note, we don't want it to read like "margin = content + padding".
So just purely for assigning bits in an enum, I think 1 << 1, 1<< 2, 1 << 3, etc. is clearer than 0x1, 0x2, 0x4, 0x8 (and it's what I recall seeing more commonly elsewhere in the code). I think we should do that, at least.

In that scheme, I'm not sure what the clearest way to represent "both of the reserved bits are set" is -- we could do (1 << 1) & (1<< 2), or 0x06, or  RECTS_USE_CONTENT_BOX & RECTS_USE_PADDING_BOX.

I just think 0x2 0x4 0x6 looks superficially like a bug (e.g. someone incrementing each successive value by 2, thinking that that reserves another bit, or something). It's not a bug, but it kinda looks like it at first glance, and being a little more declarative about the bits we're working with would address that.
but I suppose I'd be ok with 0x02 0x04 0x06 as long as there's a comment about 2 reserved bits and a "both bits set" comment next to the 0x06.
Attachment #8338267 - Attachment is obsolete: true
Attachment #8338268 - Attachment is obsolete: true
Looks good to me (assuming the Try run pans out).  Thanks!
Try looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9ceffd9514da
https://hg.mozilla.org/mozilla-central/rev/920b595433d8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: