David Baron :dbaron: Reporter
	Description • 11 years ago

In patch 9a of bug 828312, I note some FIXMEs that are followups to bug 904197.  We should probably fix these issue before enabling position:sticky, or at least investigate them further.

	David Baron :dbaron: Reporter
	Comment 1 • 11 years ago

The comments are added in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba2dc63f1bf

	Corey Ford [:coyotebush]
	Comment 2 • 11 years ago

The FIXME for GetUsedBorderAndPadding / GetUsedMargin is bug 916302.

	David Baron :dbaron: Reporter
	Comment 3 • 11 years ago

Indeed it is.  Thanks.

	:kip (Kearwood Gilbert) Assignee
	Comment 4 • 11 years ago

The remaining FIXME is within StickyScrollContainer::ComputeStickyLimits:

  // FIXME (Bug 920688):  cbFrame isn't quite right if we're dealing
  // with a block-in-inline split whose first part is a block.  We
  // probably want the first in flow of the containing block of the
  // first inline part.  (Or maybe those block-in-inline split pieces
  // are never a containing block, and we're ok?)
  nsIFrame* cbFrame = aFrame->GetContainingBlock();

	:kip (Kearwood Gilbert) Assignee
	Comment 5 • 11 years ago

(In reply to :kip (Kearwood Gilbert) from comment #4)
> The remaining FIXME is within StickyScrollContainer::ComputeStickyLimits:
> 
>   // FIXME (Bug 920688):  cbFrame isn't quite right if we're dealing
>   // with a block-in-inline split whose first part is a block.  We
>   // probably want the first in flow of the containing block of the
>   // first inline part.  (Or maybe those block-in-inline split pieces
>   // are never a containing block, and we're ok?)
>   nsIFrame* cbFrame = aFrame->GetContainingBlock();

As GetNearestBlockContainer (nsFrame.cpp) iterates upwards through the parents to find the block container, it is skipping frames that could not be a block container.  Among those conditions is that the frame does not have a type of nsIFrame::eLineParticipant.  Would it be safe to assume that this would prevent a block-in-inline split piece from being returned as a containing block?

	:kip (Kearwood Gilbert) Assignee
	Comment 6 • 11 years ago

This patch applies the correction as described in the FIXME comment; however, I have not yet found a test case that results in a different value for cbFrame than the original aFrame->GetcontainingBlock() call on its own.

If my hypothesis in Comment 5 is true, then perhaps this patch will not be required and the FIXME comment needs to be updated instead.

	David Baron :dbaron: Reporter
	Comment 7 • 11 years ago

It looks like the function GetNearestBlockContainer in nsFrame.cpp (helper for nsIFrame::GetContainingBlock) already handles this, so the FIXME can just be removed.

(Note that the patch isn't quite implementing what the comment suggests, since it's getting the first part rather than the first inline part, but it seems like we don't need to worry about that.)

	:kip (Kearwood Gilbert) Assignee
	Comment 8 • 11 years ago

This patch removes the FIXME comment from StickyScrollContainer::ComputeStickyLimits, as the condition described in the FIXME is already handled by GetNearestBlockContainer in nsFrame.cpp

	Carsten Book [:Tomcat]
	Comment 9 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/20eb244eae3b

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/20eb244eae3b