Closed Bug 856932 Opened 11 years ago Closed 11 years ago

Incorrect layer position when panning down to make the urlbar visible

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

Product:
Firefox for Android Graveyard
Component:
Toolbar
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 23

People

(Reporter: ehsan.akhgari, Assigned: cwiiis)

References

Details

(Whiteboard: [NavBar])

Attachments

(3 files, 6 obsolete files)

top:auto/bottom:auto testcase
250 bytes, text/html
Details
Fix application of fixed margins to position:fixed hierarchies v7
16.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
Fix async scrolling of auto-positioned fixed position content
4.99 KB, patch
nrc
: review+
Details | Diff | Splinter Review 
STR:

1. Load http://blog.hipset.com/boost-the-best-way-to-get-youtube-subscribers/.
2. Pan down to hide the urlbar.
3. Pan up to show the urlbar.

One of the top layers is positioned correctly, the other one is positioned by an incorrect offset down the first one.
Will take a look.
OS: Mac OS X → Android
Hardware: x86 → All
Version: unspecified → Trunk
Adding blocker so we/I don't lose track.
Blocks: dynamic-toolbar
Unfortunately, this is layout side and an issue introduced by my changes to nsAbsoluteContainerBlock... Still investigating.
The problem is that I add the fixed margins to the borders, but if you have a nested fixed-position div, it gets the borders added multiple times (for each level of nesting).

Just trying to figure out how to only add them to the top-level of nesting...
I searched for a while for a better way of doing this and came up short :( There must be a nicer way of doing this, but I couldn't figure out anything in short order...
Attachment #733424 - Flags: review?(roc)
Comment on attachment 733424 [details] [diff] [review]
Fix application of fixed margins to position:fixed hierarchies

Review of attachment 733424 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +390,5 @@
>  
> +    // Add the fixed margins to the frame's border if it's the top-level of
> +    // a position:fixed hierarchy. Descendants of position:fixed content will
> +    // receive the margins through nsHTMLReflowState.
> +    if (aKidFrame->GetContent()->GetParent()->GetPrimaryFrame()->StyleDisplay()->mPosition != NS_STYLE_POSITION_FIXED) {

why not just check whether aKidFrame's parent is the viewport? i.e. !aKidFrame->GetParent()->GetParent() ?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 733424 [details] [diff] [review]
> Fix application of fixed margins to position:fixed hierarchies
> 
> Review of attachment 733424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsAbsoluteContainingBlock.cpp
> @@ +390,5 @@
> >  
> > +    // Add the fixed margins to the frame's border if it's the top-level of
> > +    // a position:fixed hierarchy. Descendants of position:fixed content will
> > +    // receive the margins through nsHTMLReflowState.
> > +    if (aKidFrame->GetContent()->GetParent()->GetPrimaryFrame()->StyleDisplay()->mPosition != NS_STYLE_POSITION_FIXED) {
> 
> why not just check whether aKidFrame's parent is the viewport? i.e.
> !aKidFrame->GetParent()->GetParent() ?

This still doesn't catch the case that my  (horrible-looking) line catches:

<div style="position:fixed" id="a">
  <div style="position:fixed" id="b"></div>
</div>

With the current unmodified code, the margins are applied to div#a once, and div#b twice. With this patch, they get applied once to both. I (think I) need to do this as the margins alter the size of div#a, which I assume is used to size div#b. Both of the frames that represent these divs are direct descendants of the viewport.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Flags: needinfo?(roc)
Oops, right.

I guess I don't really understand the problem here. aContainingBlockWidth and aContainingBlockHeight should be the same for every fixed-pos frame that we reflow: the dimensions of the viewport. I don't see how applying the margins to the containing block for div#a eventually impacts the layout of div#b.
Flags: needinfo?(roc)
Thinking about this, I think what I do is a bit wrong (well, of course it is, because this bug exists, but bear with me);

Currently, I add the margins to the border of each child and I take away LeftRight/TopBottom from the containing block size. Nested children are inheriting the border of their parent elements, so end up getting the margins applied multiple times.

I don't think I should be doing both of these - either the margins should apply to the viewport frame, or they should apply to the first-level children of the viewport frame, but they shouldn't be applied to all children. I think altering the containing block size in addition to this will mean that any relatively-sized elements will be too small. I just tested this and it appears to be the case (height:100% ends up being height: 100% - topBottomMargins).

I think it'd be nicer to apply the margins to the viewport frame if that's possible, but otherwise, it needs to be on top-level elements on the viewport frame.
That said, I'm unconvinced my testing was correct, there appear to be other issues here too :/
(In reply to Chris Lord [:cwiiis] from comment #9)
> I don't think I should be doing both of these - either the margins should
> apply to the viewport frame, or they should apply to the first-level
> children of the viewport frame, but they shouldn't be applied to all
> children. I think altering the containing block size in addition to this
> will mean that any relatively-sized elements will be too small. I just
> tested this and it appears to be the case (height:100% ends up being height:
> 100% - topBottomMargins).

Isn't that what you want? If not, how do you expect a top:0 bottom:0 height:100% fixed-pos element to render?

I'm pretty sure making ViewportFrame::Reflow take your margins into account when it reflows all its fixed-pos children should work. And the way you've hacked it nsAbsoluteContainingBlock at least isn't far wrong. Probably we shouldn't be adding the margins to the borders of fixed-pos elements, but instead just decreasing the size of the containing block and adding an x/y offset to all the children (positioning them within the reduced area).
hmm, I actually meant 100% - 2*(topBottomMargins), but I think I was wrong...

With all this in mind, I think the current patch ought to be ok, and I'm coming up short with trying to find a more elegant way to do any of the things it does. If you have any good ideas, let me know, otherwise I'd say the currently attached r? patch is what I'm going with.
(In reply to Chris Lord [:cwiiis] from comment #12)
> hmm, I actually meant 100% - 2*(topBottomMargins), but I think I was wrong...
> 
> With all this in mind, I think the current patch ought to be ok, and I'm
> coming up short with trying to find a more elegant way to do any of the
> things it does. If you have any good ideas, let me know, otherwise I'd say
> the currently attached r? patch is what I'm going with.

Er, sorry for this churn - thinking about this some more, is it correct to add the bottom/right (or bottom/left on rtl pages) to the border of the viewport children? Would they not already effectively get this border from the containing block size change?
Comment on attachment 733424 [details] [diff] [review]
Fix application of fixed margins to position:fixed hierarchies

I've discovered more issues with this patch after testing some more... I need to figure out a better way of testing what I'm trying to test, as this seems to break some cases that worked previously.
Attachment #733424 - Flags: review?(roc)
Actually now that I think about it more, adding these margins to the borders in nsAbsoluteContainingBlock is pretty bogus. At least it's very confusing.

Instead, let's try reducing the size of the container passed into nsAbsoluteContainingBlock and also passing in an offset which is added to the top-left of the positioned children. This will allow callers of nsAbsoluteContainingBlock to have the children laid out based on any rectangle, not just the caller's border-box.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Actually now that I think about it more, adding these margins to the borders
> in nsAbsoluteContainingBlock is pretty bogus. At least it's very confusing.
> 
> Instead, let's try reducing the size of the container passed into
> nsAbsoluteContainingBlock and also passing in an offset which is added to
> the top-left of the positioned children. This will allow callers of
> nsAbsoluteContainingBlock to have the children laid out based on any
> rectangle, not just the caller's border-box.

Even doing this, the problem still exists because the offset will exist in the frame's rect. I've got a patch that does this, as it's nicer to keep the fixed margins stuff in nsViewport, alongside the scroll-position clamping scroll-port size, but I'm going to need some kind of fix for the double-offset regardless.
This is as good as I could come up with to do what you suggest. I didn't have any luck tracing how exactly descendent fixed-pos frames are related in the frame-tree to do this in some nicer way :( At least this is a bit more generic and moves the content-document-fixed-pos-margins stuff to somewhere it makes more sense...
Attachment #733424 - Attachment is obsolete: true
Attachment #734759 - Flags: review?(roc)
Here's one that doesn't break things too.
Attachment #734759 - Attachment is obsolete: true
Attachment #734759 - Flags: review?(roc)
Attachment #734767 - Flags: review?(roc)
Comment on attachment 734767 [details] [diff] [review]
Fix application of fixed margins to position:fixed hierarchies v3

Review of attachment 734767 [details] [diff] [review]:
-----------------------------------------------------------------

This is exactly what I had in mind, thanks! Except for that conditional check you've put in to work around double application of the offset.

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +462,5 @@
> +  // containing block rect origin.
> +  if ((aContainingBlockLeft != 0 || aContainingBlockTop != 0) &&
> +      !aKidFrame->GetContent()->GetParent()->GetPrimaryFrame()->IsAbsolutelyPositioned()) {
> +    rect.MoveBy(aContainingBlockLeft, aContainingBlockTop);
> +  }

This should be unconditional.

I don't understand why that causes double application of the top-left. That's something we need to figure out.

::: layout/generic/nsAbsoluteContainingBlock.h
@@ +91,2 @@
>                    nscoord                  aContainingBlockWidth,
>                    nscoord                  aContainingBlockHeight,

Let's just pass const nsRect& aContainingBlock.

@@ +108,5 @@
> +    return Reflow(aDelegatingFrame, aPresContext, aReflowState, aReflowStatus,
> +                  0, 0, aContainingBlockWidth, aContainingBlockHeight,
> +                  aConstrainHeight, aCBWidthChanged, aCBHeightChanged,
> +                  aOverflowAreas);
> +  }

I don't think we need this wrapper method --- there are only a few callers of this and changing them should be easy. Just do that.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Comment on attachment 734767 [details] [diff] [review]
> Fix application of fixed margins to position:fixed hierarchies v3
> 
> Review of attachment 734767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is exactly what I had in mind, thanks! Except for that conditional
> check you've put in to work around double application of the offset.
> 
> ::: layout/generic/nsAbsoluteContainingBlock.cpp
> @@ +462,5 @@
> > +  // containing block rect origin.
> > +  if ((aContainingBlockLeft != 0 || aContainingBlockTop != 0) &&
> > +      !aKidFrame->GetContent()->GetParent()->GetPrimaryFrame()->IsAbsolutelyPositioned()) {
> > +    rect.MoveBy(aContainingBlockLeft, aContainingBlockTop);
> > +  }
> 
> This should be unconditional.
> 
> I don't understand why that causes double application of the top-left.
> That's something we need to figure out.

I don't know enough right now (though I'll continue looking) to know if the double-application is fishy. From what happens, it just seems that in the situation of a nested, fixed-pos div, the inner children will lay out within the rects of their ancestors - that makes sense to me, but I don't know the mechanism through which they do it (seeing as the frames are only siblings, this is the bit that confuses me).

Could you give any pointers with regards to where to start looking on this? I have ideas, but perhaps you have a more enlightened one, what with knowing the code better :)

> ::: layout/generic/nsAbsoluteContainingBlock.h
> @@ +91,2 @@
> >                    nscoord                  aContainingBlockWidth,
> >                    nscoord                  aContainingBlockHeight,
> 
> Let's just pass const nsRect& aContainingBlock.

That would require we keep local copies of width/height. With that in mind, is this still what you'd prefer to see? (ReflowAbsoluteFrame calls a function that takes nscoord& and modifies them)

> @@ +108,5 @@
> > +    return Reflow(aDelegatingFrame, aPresContext, aReflowState, aReflowStatus,
> > +                  0, 0, aContainingBlockWidth, aContainingBlockHeight,
> > +                  aConstrainHeight, aCBWidthChanged, aCBHeightChanged,
> > +                  aOverflowAreas);
> > +  }
> 
> I don't think we need this wrapper method --- there are only a few callers
> of this and changing them should be easy. Just do that.

Sounds good to me.
(In reply to Chris Lord [:cwiiis] from comment #20)
> I don't know enough right now (though I'll continue looking) to know if the
> double-application is fishy. From what happens, it just seems that in the
> situation of a nested, fixed-pos div, the inner children will lay out within
> the rects of their ancestors - that makes sense to me, but I don't know the
> mechanism through which they do it (seeing as the frames are only siblings,
> this is the bit that confuses me).

That doesn't make any sense to me. All fixed-pos elements are siblings, all their frames are direct children of the viewport. The layout of one should not affect the layout of any others. Hmm, unless you're doing something funky with left/top:auto ... is that the case here?

> Could you give any pointers with regards to where to start looking on this?
> I have ideas, but perhaps you have a more enlightened one, what with knowing
> the code better :)

The first thing I'd do is come up with a minimal testcase. We'd want this for an automated test anyway.

Then examine the call for nsAbsoluteContainingBlock::ReflowAbsoluteFrame when it reflows the incorrectly-positioned nested fixed-pos element, and figure out how it's top/left are being set. That should all be in nsAbsoluteContainingBlock.

> That would require we keep local copies of width/height. With that in mind,
> is this still what you'd prefer to see? (ReflowAbsoluteFrame calls a
> function that takes nscoord& and modifies them)

Yes.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> That doesn't make any sense to me. All fixed-pos elements are siblings, all
> their frames are direct children of the viewport. The layout of one should
> not affect the layout of any others. Hmm, unless you're doing something
> funky with left/top:auto ... is that the case here?

When left and right are both auto, or top and bottom are both auto, we go through nsHTMLReflowState::CalculateHypotheticalBox via nsHTMLReflowState::InitAbsoluteConstraints to calculate mComputedOffsets. When mComputedOffsets was computed that way for an axis, we don't want to add your containing block top/left value.
Thanks, that last comment was, obviously, a big help :)
Attachment #734767 - Attachment is obsolete: true
Attachment #734767 - Flags: review?(roc)
Attachment #735139 - Flags: review?(roc)
Comment on attachment 735139 [details] [diff] [review]
Fix application of fixed margins to position:fixed hierarchies v4

Review of attachment 735139 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +468,5 @@
> +    rect.x += aContainingBlockRect.x;
> +  }
> +  if (eStyleUnit_Auto != aKidFrame->StylePosition()->mOffset.GetTopUnit() ||
> +      eStyleUnit_Auto != aKidFrame->StylePosition()->mOffset.GetBottomUnit()) {
> +    rect.y += aContainingBlockRect.y;

wrap all this in "if (aContainingBlockRect.TopLeft() != nsPoint(0,0))". Also, CSE aKidFrame->StylePosition()->mOffset.

::: layout/generic/nsAbsoluteContainingBlock.h
@@ +91,2 @@
>                    nscoord                  aContainingBlockWidth,
>                    nscoord                  aContainingBlockHeight,

This should be a const nsRect& aContainingBlock

@@ +122,5 @@
>  
>    nsresult ReflowAbsoluteFrame(nsIFrame*                aDelegatingFrame,
>                                 nsPresContext*           aPresContext,
>                                 const nsHTMLReflowState& aReflowState,
> +                               const nsRect&            aContainingBlockRect,

Let's call it aContainingBlock
After testing more and reading through nsHTMLReflowState a bit, this is the best I could come up with - just checking for left/top && right/bottom == auto isn't enough, as that'll catch unnested frames that don't have any of those properties specified.

I think  the check I've added is reliable, at least for fixedpos frames - I'm not 100% confident that this code would work perfectly if we used it for non-fixed frames, but we don't and I don't think we will any time soon, so...
Attachment #735139 - Attachment is obsolete: true
Attachment #735139 - Flags: review?(roc)
Attachment #735301 - Flags: review?(roc)
Comment on attachment 735301 [details] [diff] [review]
Fix application of fixed margins to position:fixed hierarchies v5

Review of attachment 735301 [details] [diff] [review]:
-----------------------------------------------------------------

Now I don't understand the need for the flags. Doesn't mHorizontalComputedOffsetUsedContainingBlock get set to true exactly when eStyleUnit_Auto == mStylePosition->mOffset.GetLeftUnit() && eStyleUnit_Auto == mStylePosition->mOffset.GetRightUnit()? Which doesn't make any sense, since that's when the offset used the placeholder instead of the containing block.

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +458,5 @@
> +  // If nsHTMLReflowState used the containing block of the placeholder frame
> +  // to determine the computed offsets, and that containing block's parent is
> +  // the delegating frame, it means that a sibling frame was used to calculate
> +  // the offsets, and that sibling frame will already have the absolute
> +  // containing block origin offset applied.

This comment is a bit confusing. I think you want something more like

// If the frame was positioned based on the placeholder position, instead of relative
// to the containing block, don't add the offset.

@@ +473,5 @@
> +      }
> +    }
> +
> +    if (!(kidReflowState.mHorizontalComputedOffsetUsedContainingBlock &&
> +          cbFrameParent == aDelegatingFrame)) {

Why do you need this check of cbFrameParent? It shouldn't be needed.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Comment on attachment 735301 [details] [diff] [review]
> Fix application of fixed margins to position:fixed hierarchies v5
> 
> Review of attachment 735301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Now I don't understand the need for the flags. Doesn't
> mHorizontalComputedOffsetUsedContainingBlock get set to true exactly when
> eStyleUnit_Auto == mStylePosition->mOffset.GetLeftUnit() && eStyleUnit_Auto
> == mStylePosition->mOffset.GetRightUnit()? Which doesn't make any sense,
> since that's when the offset used the placeholder instead of the containing
> block.

Right, there's no real need for the flags, I was just interpreting your comment on IRC but that's the only thing I could think of to flag... I obviously didn't interpret correctly though.

> ::: layout/generic/nsAbsoluteContainingBlock.cpp
> @@ +458,5 @@
> > +  // If nsHTMLReflowState used the containing block of the placeholder frame
> > +  // to determine the computed offsets, and that containing block's parent is
> > +  // the delegating frame, it means that a sibling frame was used to calculate
> > +  // the offsets, and that sibling frame will already have the absolute
> > +  // containing block origin offset applied.
> 
> This comment is a bit confusing. I think you want something more like
> 
> // If the frame was positioned based on the placeholder position, instead of
> relative
> // to the containing block, don't add the offset.
> 
> @@ +473,5 @@
> > +      }
> > +    }
> > +
> > +    if (!(kidReflowState.mHorizontalComputedOffsetUsedContainingBlock &&
> > +          cbFrameParent == aDelegatingFrame)) {
> 
> Why do you need this check of cbFrameParent? It shouldn't be needed.

So, I think I meant what I said in the comment - as I read it, fixed position elements are always laid out with respect to the containing block in ComputeHypotheticalBox?

I'm having a hard time finding anything except for the cbFrameParent to use to distinguish these frames from frames that need the offset.
(In reply to Chris Lord [:cwiiis] from comment #27)
> So, I think I meant what I said in the comment - as I read it, fixed
> position elements are always laid out with respect to the containing block
> in ComputeHypotheticalBox?

Not just fixed position elements, any absolute element sorry. The parts dealing with cbOffset in nsHTMLReflowState::CalculateHypotheticalBox at the bottom - quote:

  // The current coordinate space is that of the nearest block to the placeholder.
  // Convert to the coordinate space of the absolute containing block
  // One weird thing here is that for fixed-positioned elements we want to do
  // the conversion incorrectly; specifically we want to ignore any scrolling
  // that may have happened;

So if the absolute containing block isn't the delegate frame, it will be a child of the delegate frame that already has the offset applied, I'd have thought?
After some discussion on IRC, I think it's worth clarifying what I meant the contentDocumentFixedPositionMargins to do. After talking with jwatt, I realise I picked very much the wrong name for it in layout-land...

I intended for these margins to effectively shrink the viewport as far as fixed-position elements are concerned. A better name would have been ViewportMarginsForFixedPositionContent. So in terms of what's happening in nsAbsoluteContainingBlock, every frame should have the top-left offset applied to it exactly once, regardless of its positioning.

Does that make my patch any more/less valid?
I understood what you wanted to do, even if the name isn't very good :-).
If we have a small element that's positioned at left:50%, top:50%, that wouldn't normally intersect these margins, should it actually be moved by these margins? i.e. should we actually be laying everything out to the modified viewport, or should we just be clamping the edges of things that would intersect the margins?

I think the former, but I'm just checking. The problem with the latter is that if you have "top:0; height:50px" and another element with "top:50px" then they might start overlapping if the first element is moved but the second isn't.
This testcase uses top/bottom:auto. Initially the "Fixed" div is rendered exactly where it would be if it's not position:fixed, but as you scroll, it stays where it is. In this testcase, I think it's pretty clear that adding aContainingBlock.top to the y-position of the element would be wrong.

So my question is, why do you want to add the top/left of aContainingBlock to elements with left/right:auto or top/bottom:auto? Can you point to some example pages that behave badly if you don't do that?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Created attachment 736132 [details]
> top:auto/bottom:auto testcase
> 
> This testcase uses top/bottom:auto. Initially the "Fixed" div is rendered
> exactly where it would be if it's not position:fixed, but as you scroll, it
> stays where it is. In this testcase, I think it's pretty clear that adding
> aContainingBlock.top to the y-position of the element would be wrong.
> 
> So my question is, why do you want to add the top/left of aContainingBlock
> to elements with left/right:auto or top/bottom:auto? Can you point to some
> example pages that behave badly if you don't do that?

Wow, thanks for this example - I didn't realise you could do that... Makes sense, but I hadn't considered it.

Originally, I intended for all position:fixed to layout as if the browser size was deflated by these margins, but I hadn't considered this kind of example.

So I think what should happen:

- The corresponding position axis vs. the margin isn't auto, offset
- The corresponding position axis vs. the margin is auto, but the containing block is the body, offset
- Neither of these conditions are met, don't offset - the offset will either come via the containing block, or it doesn't make sense to apply it

It's the 2nd condition that a patch that just looks at auto won't comply with, and the reason I stipulate that is that I can see people doing top-aligned bars without actually specifying top.

Sound reasonable?
Also note to self; the 3rd condition (no offset) is going to require Layers changes for correct async scrolling behaviour.
This does what I would consider to be the correct thing. Need a follow-up patch to improve async reconciliation in this case...
Attachment #735301 - Attachment is obsolete: true
Attachment #735301 - Flags: review?(roc)
Attachment #736439 - Flags: review?(roc)
(In reply to Chris Lord [:cwiiis] from comment #33)
> So I think what should happen:
> 
> - The corresponding position axis vs. the margin isn't auto, offset
> - The corresponding position axis vs. the margin is auto, but the containing
> block is the body, offset

Can you clarify this? What exactly do you mean by "the containing block is the body"?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> So my question is, why do you want to add the top/left of aContainingBlock
> to elements with left/right:auto or top/bottom:auto? Can you point to some
> example pages that behave badly if you don't do that?

Also I'd still like a direct answer to these questions :-)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> (In reply to Chris Lord [:cwiiis] from comment #33)
> > So I think what should happen:
> > 
> > - The corresponding position axis vs. the margin isn't auto, offset
> > - The corresponding position axis vs. the margin is auto, but the containing
> > block is the body, offset
> 
> Can you clarify this? What exactly do you mean by "the containing block is
> the body"?

If the placeholder frame's containing block is the primary frame of the body element for the document. (i.e. the position:fixed element is a direct child of the <body> element)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> > So my question is, why do you want to add the top/left of aContainingBlock
> > to elements with left/right:auto or top/bottom:auto? Can you point to some
> > example pages that behave badly if you don't do that?
> 
> Also I'd still like a direct answer to these questions :-)

Yes, sorry, got carried away :)

So my worry is a site would do something like this: http://chrislord.net/files/mozilla/fixed8.html

In short,

<body>
  <div style="position:fixed" id="transient-header"></div>
  <div id="content"></div>
</body>

And rely on the auto positioning putting it at/near the top of the page. That said, it took me longer to craft my example page than it would've done to resort to 'top: 0' if I was seriously making a page, so if you think this is a silly worry I'm perfectly happy to not offset any auto-positioned elements :)

I pick the cookie-warning example as it's more likely that they'd be ok with the element overlapping content.

CSS is weird... I'd love to know a legitimate use for auto-positioned fixed content that isn't a descendent of non-auto-positioned fixed content...
(In reply to Chris Lord [:cwiiis] from comment #38)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> > (In reply to Chris Lord [:cwiiis] from comment #33)
> > > So I think what should happen:
> > > 
> > > - The corresponding position axis vs. the margin isn't auto, offset
> > > - The corresponding position axis vs. the margin is auto, but the containing
> > > block is the body, offset
> > 
> > Can you clarify this? What exactly do you mean by "the containing block is
> > the body"?
> 
> If the placeholder frame's containing block is the primary frame of the body
> element for the document. (i.e. the position:fixed element is a direct child
> of the <body> element)

In my testcase, the position:fixed element is a direct child of the body element. So your rules would add the offset in my testcase, which we agreed would be wrong. Right? :-)
(In reply to Chris Lord [:cwiiis] from comment #39)
> So my worry is a site would do something like this:
> http://chrislord.net/files/mozilla/fixed8.html
> 
> In short,
> 
> <body>
>   <div style="position:fixed" id="transient-header"></div>
>   <div id="content"></div>
> </body>
> 
> And rely on the auto positioning putting it at/near the top of the page.
> That said, it took me longer to craft my example page than it would've done
> to resort to 'top: 0' if I was seriously making a page, so if you think this
> is a silly worry I'm perfectly happy to not offset any auto-positioned
> elements :)

Yes, I think this is a possibility. But as long as it's only a theoretical possibility and not something we see causing problems in important sites, I think we should ignore it.
(If there was a simple way to handle it without risking breaking other things, I'd say let's handle it now, but I don't see one.)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> (If there was a simple way to handle it without risking breaking other
> things, I'd say let's handle it now, but I don't see one.)

Yup, this reasoning is fine with me, I'll attach a revised patch later/tomorrow :)

It's worth noting that the new Chrome Beta has the same dynamic toolbar feature as us, but they offset all position:fixed content, regardless of auto-positioning. If pages come to expect this, we might want to revisit this issue.
Do as we've discussed.
Attachment #736439 - Attachment is obsolete: true
Attachment #736439 - Flags: review?(roc)
Attachment #737249 - Flags: review?(roc)
Comment on attachment 737250 [details] [diff] [review]
Fix async scrolling of auto-positioned fixed position content

Review of attachment 737250 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +854,5 @@
>     * a document that has had fixed position element margins set on it, these
>     * will be mirrored here. This allows for asynchronous animation of the
>     * margins by reconciling the difference between this value and a value that
>     * is updated more frequently.
> +   * If the left or top margins are negative, this means the elements this

nit: s/this means/it means that

::: layout/base/nsDisplayList.cpp
@@ +2957,5 @@
>                                             aContainerParameters.mYScale,
>                                           NSAppUnitsToFloatPixels(fixedMargins.left, factor) *
>                                             aContainerParameters.mXScale);
> +
> +  // If the frame is auto-positioned on both axes, set the top/left layer

nit: s/both axes/either axis [or at least, that is what the code is doing, I hope it is the comment which is wrong]
Attachment #737250 - Flags: review?(ncameron) → review+
https://hg.mozilla.org/mozilla-central/rev/abc416eac56f
https://hg.mozilla.org/mozilla-central/rev/cf860d3271ab
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Whiteboard: [NavBar]
Verified fixed on:
Build: Firefox for Android 23.0a1(2013-04-21)
Device;LG Nexus 4
OS: Android 4.2.2
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: