Closed Bug 640443 Opened 13 years ago Closed 12 years ago

Absolute position within relatively positions flexible box elements doesn't work

Categories

(Core :: Layout: Positioned, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: blair.mitchelmore, Assigned: dholbert)

References

()

Details

Attachments

(7 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre

It seems like elements absolutely positioned with a parent using display:-moz-box and position:relative don't get positioned relative to that parent, as would be expected.

Reproducible: Always

Steps to Reproduce:
1. Build a page with an absolutely positioned element within another positioned element with display:-moz-box set.
2. View that page.
3. Tada.
Actual Results:  
The child element is positioned relative to the body root (or more likely, the next closest positioned parent)

Expected Results:  
The child element would be positioned relative to its positioned parent.
Depends on: 455338
in fact, when the -moz-box node uses absolute position, -moz-box-orient and -moz-box-flex (maybe -moz-box-*) don't work at all.

sample code to reproduce:
<body>
<div style="position: fixed; top: 0; bottom: 0; left: 0; right: 0;" class="hbox">
  <div class="flex">1</div>
  <div style="width:20px">2</div>
</div>
</body>
Right, -moz-box-flex in absolutely positioned elements doesn't work. Works fine in WebKit though. Example:  http://jsfiddle.net/WVRH8/
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86_64 → All
Note that while bug 649701 was marked as a duplicate of bug 640443, this does not sound like a duplicate (or at least not an exact duplicate):

Bug 649701 pertains to a structure like this:

div display:-moz-box
  div -moz-box-flex: 1.0;position:relative
    div position:absolute;top:20px;left:20px;right:20px;bottom:20px

and complains about the bottom style being ignored completely.

vs. bug 640443 pertains to a structure like this:

div with display:-moz-box;position:relative
  div with position:absolute;top:25px;left:25px;width:200px;height:100px

and complains about the child element being positioned relative to the body root (or more likely, the
next closest positioned parent).
An infrastructure for this has landed in bug 10209.
Component: Layout → Layout: R & A Pos
QA Contact: layout → layout.r-and-a-pos
Version: unspecified → Trunk
What are the chances of this getting fixed? It's blocking bug 729878 - we'll have a flexible layout implemented with 'display: -moz-box' and need to temporarily position elements absolutely to animate re-ordering and use them as custom drag images.
Blocks: 729878
Daniel: can you have a look and see if you can help unblock this while you're in the flexbox code?
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Sure.

So, the most proximal cause is that we treat "display: -moz-box" as "display: block" if the element is floated or absolutely-positioned.  So we create a block frame instead of a box frame, and naturally all of the box-specific properties have no effect (as noted in comment 1).

This happens in EnsureBlockDisplay in nsRuleNode.cpp, here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#156
(called for abspos & floated content)

We need to add "NS_STYLE_DISPLAY_MOZ_BOX" to the "// do not muck with these" list there (and more, that's just the first step).
(In reply to Tim Taubert [:ttaubert] from comment #6)
> What are the chances of this getting fixed? It's blocking bug 729878 - we'll
> have a flexible layout implemented with 'display: -moz-box' and need to
> temporarily position elements absolutely to animate re-ordering and use them
> as custom drag images.

Tim: When you need to do that, you should be able to (temporarily) wrap the -moz-box in a normal block, and use absolute positioning on _that_.
 So instead of
>   <div style="display: -moz-box; position: absolute; left: 10px">...</div>
 you could do:
>   <div style="position: absolute; left: 10px"><div style="display: -moz-box">...</div></div>

It adds one step (wrap/unwrap whenever you want to move or stop moving the box), but it should work in existing Firefox builds.

(In reply to Jet Villegas from comment #7)
> Daniel: can you have a look and see if you can help unblock this while
> you're in the flexbox code?

Note that this is about old-flexbox, which I haven't touched much & IIUC is a bit crufty/fragile (and is being somewhat obsoleted by newer-flexbox anyway).  So -- if the workaround above is sufficient for the purposes of bug 729878, I might prefer to hold off on this at the moment.
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Tim: When you need to do that, you should be able to (temporarily) wrap the
> -moz-box in a normal block, and use absolute positioning on _that_.

(or not temporarily -- the block wrapper could be permanent, too, if that doesn't mess up your design.)
This is the layout I'm currently implementing: http://people.mozilla.com/~shorlander/files/new-tab-prototype-i03/new-tab-prototype-i03.html

I managed to correctly position absolute elements contained in the grid by wrapping the whole grid in a block element but the problem is that the block element doesn't adjust to the window size.

So I thought about a workaround that "freezes" the whole grid when starting a drag/drop but I'd rather have a real fix than such an ugly workaround :)

I wasn't sure if your testcase covers the same thing as mine because your box elements are positioned absolutely themselves. Feel free to ignore mine if that's the same root cause.
Attachment #601445 - Attachment description: testcase → testcase (positioned -moz-box)
Attachment #601562 - Attachment description: another testcase → another testcase (positioned child of -moz-box)
So there are (at least) two distinct issues that have been described here.
 (A) relatively-positioned boxes *should* behave as containers for absolutely positioned children, but they currently don't. (covered in comment 1 & comment 12)

 (B) when a box itself is absolutely positioned, it reverts to being a block. (not a box)  (covered in comment 1 and comment)

Let's focus just on (A) here, since that's the original issue & it also seems to be what Tim needs.  So, no more worrying about boxes that are themselves absolutely positioned.  (& hence disregard my comment 8)
(In reply to Daniel Holbert [:dholbert] from comment #13)
>  (B) when a box itself is absolutely positioned, it reverts to being a
> block. (not a box)  (covered in comment 1 and comment)

That's bug 579776, isn't it?

> Let's focus just on (A) here, since that's the original issue & it also
> seems to be what Tim needs.  So, no more worrying about boxes that are
> themselves absolutely positioned.  (& hence disregard my comment 8)

Sounds good to me.
Attached patch partial (hacky) fix (obsolete) — Splinter Review
So the box isn't getting treated as an abspos container because it has the FCDATA_SKIP_ABSPOS_PUSH bit set in its frame construction data, which means we skip this clause:
> 3548 nsCSSFrameConstructor::ConstructFrameFromItemInternal(
[...]
> 3714     } else if (!(bits & FCDATA_SKIP_ABSPOS_PUSH) &&
> 3715                maybeAbsoluteContainingBlockDisplay->IsPositioned()) {
> 3716       aState.PushAbsoluteContainingBlock(maybeAbsoluteContainingBlock,
> 3717                                          absoluteSaveState);
> 3718     }
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#3714

The attached patch removes that bit, and teaches nsBoxFrame to reflow & destroy its abspos children.  This is sufficient to make abspos work inside of a -moz-box, at least vertically. ("top" & "bottom" work, but not "right" & "left"; not sure why)

I'm sure that FCDATA_SKIP_ABSPOS_PUSH bit is there for a reason (and clearing it in the spot where I did will impact more than just nsBoxFrames), so this is in no way a complete/correct patch.
(In reply to Tim Taubert [:ttaubert] from comment #14)
> (In reply to Daniel Holbert [:dholbert] from comment #13)
> >  (B) when a box itself is absolutely positioned, it reverts to being a
> > block. (not a box)  (covered in comment 1 and comment)
> 
> That's bug 579776, isn't it?

( er I meant "...and comment 8)" there )

Yup, it appears to be. Cool.
Is this going to land before next source migration?
Attachment #601445 - Attachment description: testcase (positioned -moz-box) → testcase (positioned -moz-box) (ignore; this is bug 579776)
(In reply to Daniel Holbert [:dholbert] from comment #15)
> Created attachment 601729 [details] [diff] [review]
> partial (hacky) fix
[...]
> The attached patch removes that bit, and teaches nsBoxFrame to reflow &
> destroy its abspos children.  This is sufficient to make abspos work inside
> of a -moz-box, at least vertically. ("top" & "bottom" work, but not "right"
> & "left"; not sure why)

Actually, I re-tested this today, and it works for both vertical & horizontal now. Woohoo! (Not sure why it wasn't working when I tested it before.)

I'll generate a Try build so that Tim can see if this does the trick.

>(In reply to Dietrich Ayala (:dietrich) from comment #17)
> Is this going to land before next source migration?

I'm not sure -- if all we need is the patch I have posted, then likely-yes. I'm not sure if there's any nasty fallout/invalid-assumptions from removing the FCDATA_SKIP_ABSPOS_PUSH bit, though. (a tryserver build may help with that, too)  CC'ing bz in case he has any feedback on this or on the patch so far...
> I'm sure that FCDATA_SKIP_ABSPOS_PUSH bit is there for a reason 

The main reason is because those frames didn't use to support absos kids, I'd think.

The set of frames affected by your change is pretty large, but they _might_ all be subclasses of nsBoxFrame.  Worth checking.  Specifically, it will affect xul buttonboxes, autorepeatboxes, titlebars, resizers, listboxes, listitems, and things with most XUL display values, -moz-deck being an exception (so for -moz-box, -moz-inline-box, -moz-grid, -moz-inline-grid, -moz-grid-group, -moz-grid-line, -moz-groupbox, -moz-stack, -moz-inline-stack).

If we just want to allow "generic" boxes (and not all the other stuff), then you'd just want to change what kind of fcdata is created for the BOX and INLINE_BOX cases in FindXULDisplayData....  At the very least we need to audit to make sure that all the NS_NewWhatever functions for which we remove the bit are in fact creating subclasses of nsBoxFrame.

One other issue; I'm not sure how you tested, but it seems like nsBoxFrame::Reflow would only be called for a box whose parent is not a box.  A box whose parent is another box would directly end up calling Layout() or just going through the appropriate layout manager, right?
(In reply to Boris Zbarsky (:bz) from comment #19)
> If we just want to allow "generic" boxes (and not all the other stuff), then
> you'd just want to change what kind of fcdata is created for the BOX and
> INLINE_BOX

Yup -- the attached patch was just a first-pass, but I did a more targeted FCDATA tweak (just affecting BOX and INLINE_BOX) in my Try push:
  https://tbpl.mozilla.org/?tree=Try&rev=2a9660ea4911
  https://hg.mozilla.org/try/rev/8533338e0a01

That seems to have passed existing {ref,mochi,crash}tests (at least on Linux, so far).

> One other issue; I'm not sure how you tested, but it seems like
> nsBoxFrame::Reflow would only be called for a box whose parent is not a box.
> A box whose parent is another box would directly end up calling Layout() or
> just going through the appropriate layout manager, right?

So far, I've just tested w/ Tim's attached testcase & minor variants of it.  Haven't tested box-within-box yet -- thanks for that heads-up -- I'll be sure to test that and add tweaks there if necessary.
Attached patch fix v2 (obsolete) — Splinter Review
For reference, here's the patch that I pushed to Try. (tightened to only affect boxes, per first chunk of previous comment)
Attachment #601729 - Attachment is obsolete: true
Attached file testcase 3
(In reply to Boris Zbarsky (:bz) from comment #19)
> One other issue; I'm not sure how you tested, but it seems like
> nsBoxFrame::Reflow would only be called for a box whose parent is not a box.
> A box whose parent is another box would directly end up calling Layout() or
> just going through the appropriate layout manager, right?

Yup, looks like the patch so far doesn't work on the 3rd part of this testcase
  (with box > relpos box > abspos kid )
Attachment #601562 - Attachment description: another testcase (positioned child of -moz-box) → testcase 2 (positioned child of -moz-box)
Attachment #601445 - Attachment description: testcase (positioned -moz-box) (ignore; this is bug 579776) → testcase 1 [IGNORE; this is bug 579776] (based on jsfiddle in comment 2)
Attached patch fix v3 (obsolete) — Splinter Review
This adds a chunk to nsBoxFrame::DoLayout to call ReflowAbsoluteFrames.

(I switched away from using the "FinishReflowWithAbsoluteFrames" wrapper because I don't think we want the FinishReflow part.  At least in DoLayout, that part triggers assertions about overflow area needing to contain frame bounds, if it's there. All we really care about for this change is that the abspos children get reflowed).

I want to add a number of automated tests, but I'm requesting review anyway in case bz gets a chance to look over this sooner.

This passes attached testcases 2 & 3 (ignoring testcase 1), and it also passes this bug's URL ( http://test.blairmitchelmore.com/firefox/display-box-positioning.html )
Attachment #603633 - Attachment is obsolete: true
Attachment #603641 - Flags: review?(bzbarsky)
Attached patch fix v3a (obsolete) — Splinter Review
(forgot to qref, so previous patch-version was missing the s/FinishReflowWithAbsoluteFrames/ReflowAbsoluteFrames/ change. Fixed now.)
Attachment #603641 - Attachment is obsolete: true
Attachment #603641 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-p linux]
Whiteboard: [autoland-try:-p linux] → [autoland-in-queue]
Attachment #603642 - Flags: review?(bzbarsky)
Autoland Patchset:
	Patches: 603642
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=45a4dc9af031
Try run started, revision 45a4dc9af031. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=45a4dc9af031
Comment on attachment 603642 [details] [diff] [review]
fix v3a

Seems to work pretty well for me.
Try run for 45a4dc9af031 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=45a4dc9af031
Results (out of 28 total builds):
    success: 25
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-45a4dc9af031
Whiteboard: [autoland-in-queue]
The current patch works well for -moz-boxes but not for -moz-stacks it seems. Is this just a little fix or are they completely different? I thought they just were "special boxes".
While trying to work around bug 579776 I discovered another issue. [position: absolute, left/right/top/bottom: 0] to mimic flex:1 does not work if the parent is a positioned flexible child of a box. That's hard to describe, I attached a testcase. Does that belong to this bug? BTW, setting the height to a specific value works, so the element adapts horizontally but not vertically.
(In reply to Tim Taubert [:ttaubert] from comment #28)
> The current patch works well for -moz-boxes but not for -moz-stacks it
> seems. Is this just a little fix or are they completely different? I thought
> they just were "special boxes".

Well -- nsStackFrame does inherit from nsBoxFrame, but they each have their own frame construction data, and the patch so far specifically targets the -moz-box / -moz-inline-box frame construction data.

If we were to make this affect _all_ "special boxes", it would be a bit more broad in scope:
  http://mxr.mozilla.org/mozilla-central/search?string=public%20nsBoxFrame
and that scares me a little. That might be the right thing to do long-term, but it could make this patch a bit harder to test/sanity-check, and that'd be bad right now per the time-pressure indicated by comment 17. :)

My preference would be to target the specific boxes / special-boxes (including stack, perhaps) that you need for about:newtab right now, and to perhaps do the rest of the subclasses in a followup bug as-needed / as cycles are available.
RE testcase 4: Tim says in IRC that he doesn't need stacks after all, so I think I'm going to leave the patch as-is (modulo review feedback & testcases).

RE testcase 5: I think that's a separate bug & independent from the work here. I filed bug 733875 on that (with testcases reduced from testcase 5 here).
Comment on attachment 603642 [details] [diff] [review]
fix v3a

r=me
Attachment #603642 - Flags: review?(bzbarsky) → review+
From a bit more testing, I noticed that the existing the patch (with an abspos child that's taller than its relpos-box parent) makes this testcase trigger the following assertion...
{
###!!! ASSERTION: this type of frame can't have overflow containers: '(aProperty != nsContainerFrame::OverflowContainersProperty() && aProperty != nsContainerFrame::ExcessOverflowContainersProperty()) || IsFrameOfType(nsIFrame::eCanContainOverflowContainers)', file ../../../mozilla/layout/generic/nsContainerFrame.cpp, line 1454
}
...while we're inside of the added ReflowAbsoluteFrames call.  That's undesirable.
Attachment #603985 - Attachment description: testcase 6 (fix hopefully shouldn't make this assert) → testcase 6 (the fix hopefully shouldn't make this assert)
So, ReflowAbsoluteFrames calls nsAbsoluteContainingBlock::Reflow, which reflows the abspos child (a block), using available space = the size of our nsBoxFrame. The block doesn't fit, so it creates a NIF for itself during its reflow.

At this point we're already in trouble, because we aren't going to have anywhere to put that NIF (I think?), since nsBoxFrame can't have overflow containers (according to the IsFrameOfType call in the comment 33 assertion).

Is there any way we can prevent our abspos children from splitting & making NIFs? (so they just end up overflowing & maybe being cropped, instead of needing an explicit overflow container)
(In reply to Daniel Holbert [:dholbert] from comment #34)
> So, ReflowAbsoluteFrames calls nsAbsoluteContainingBlock::Reflow, which
> reflows the abspos child (a block), using available space = the size of our
> nsBoxFrame. The block doesn't fit, so it creates a NIF for itself during its
> reflow.

(correction: "The block doesn't fit, so it returns an incomplete reflow status, which tells nsAbsoluteContainingBlock::Reflow to make a continuing frame for it)
Oh, I should have caught that.  You're passing GetSize() to the reflow state as the available space... you should probably pass an nsSize whose width is the actual available space (maybe the width of our frame is OK) but whose height is either always unconstrained or only constrained when our box is constrained....
Ah, of course! That indeed fixes it. I'll repost with that tweak & automated tests.
Attached patch fix v4 (obsolete) — Splinter Review
This version passes in an unconstrained available height, per comment 36.  Added reftests, to, with:
 -abspos child in a relpos -moz-box / -moz-inline-box (like testcase 2 here)
 -...and then with an additional -moz-box grandparent (like testcase 3 here)
 -...and a testcase that we don't want to assert (like testcase 6 here)

I've verified the following:
 - all of these reftests fail w/ current m-c
 - the last reftest fails due to an assertion w/ fix v3a applied
 - they all pass with _this_ patch (v4) applied.
Attachment #603642 - Attachment is obsolete: true
Attachment #604132 - Flags: review?(bzbarsky)
Comment on attachment 604132 [details] [diff] [review]
fix v4

(er, just  noticed a typo in a reftest header-comment; fixing & reposting)
Attachment #604132 - Flags: review?(bzbarsky)
Attached patch fix v4aSplinter Review
Attachment #604134 - Flags: review?(bzbarsky)
Attachment #604132 - Attachment is obsolete: true
Whiteboard: [autoland-try:-p linux]
Whiteboard: [autoland-try:-p linux] → [autoland-in-queue]
Autoland Patchset:
	Patches: 604134
	Branch: mozilla-central => try
Patch 604134 could not be applied to mozilla-central.
file layout/reftests/box/flexbox-abspos-container-1-ref.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-1-ref.html.rej
file layout/reftests/box/flexbox-abspos-container-1a.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-1a.html.rej
file layout/reftests/box/flexbox-abspos-container-1b.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-1b.html.rej
file layout/reftests/box/flexbox-abspos-container-1c.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-1c.html.rej
file layout/reftests/box/flexbox-abspos-container-1d.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-1d.html.rej
file layout/reftests/box/flexbox-abspos-container-2-ref.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-2-ref.html.rej
file layout/reftests/box/flexbox-abspos-container-2.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/box/flexbox-abspos-container-2.html.rej
abort: patch failed to apply

Patchset could not be applied and pushed.
Whiteboard: [autoland-in-queue]
(Wha-huh...?  Looks like RelEng bot is trying to apply 2 patches instead of one or something.  No matter, I'll just push to try manually.)
(also fwiw, I filed bug 734214 on whatever autoland weirdness happened in comment 41)
Comment on attachment 604134 [details] [diff] [review]
fix v4a

Looks reasonable, though do we need to deal with updating the overflow area if the abspos stuff overflows the box?
Attachment #604134 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #45)
> Looks reasonable, though do we need to deal with updating the overflow area
> if the abspos stuff overflows the box?

Good question, but that seems to already be taken care of -- in testcase 6 (with an overflowing abspos child), the box ends up with an overflow rect that includes the child's region.

From a bit of GDB detective-work, this overflow-rect-update happens at line 614 & 617 here:
> 575 nsBox::SyncLayout(nsBoxLayoutState& aState)
> 576 {
[...]
> 600   nsRect visualOverflow;
> 601 
> 602   if (ComputesOwnOverflowArea()) {
> 603     visualOverflow = GetVisualOverflowRect();
> 604   }
> 605   else {
> 606     nsRect rect(nsPoint(0, 0), GetSize());
> 607     nsOverflowAreas overflowAreas(rect, rect);
> 608     if (!DoesClipChildren() && !IsCollapsed()) {
> 609       // See if our child frames caused us to overflow after being laid
> 610       // out. If so, store the overflow area.  This normally can't happen
> 611       // in XUL, but it can happen with the CSS 'outline' property and
> 612       // possibly with other exotic stuff (e.g. relatively positioned
> 613       // frames in HTML inside XUL).
> 614       nsLayoutUtils::UnionChildOverflow(this, overflowAreas);
> 615     }
> 616 
> 617     FinishAndStoreOverflow(overflowAreas, GetSize());
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsBox.cpp#602

(Note that "ComputesOwnOverflowArea()" returns false, for nsBoxFrame. (it's inline in nsBoxFrame.h))

The above code (nsBox::SyncLayout) is called by nsBox::EndLayout, which is called after the code added in this bug.

Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/d069fd0abc23
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 1373767
You need to log in before you can comment on or make changes to this bug.