Last Comment Bug 640443 - Absolute position within relatively positions flexible box elements doesn't work
: Absolute position within relatively positions flexible box elements doesn't work
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla13
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://test.blairmitchelmore.com/fire...
Depends on: 455338
Blocks: 729878
  Show dependency treegraph
 
Reported: 2011-03-09 18:49 PST by Blair Mitchelmore
Modified: 2012-03-11 19:57 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase 1 [IGNORE; this is bug 579776] (based on jsfiddle in comment 2) (731 bytes, text/html)
2012-02-28 16:16 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 2 (positioned child of -moz-box) (456 bytes, text/html)
2012-02-29 02:13 PST, Tim Taubert [:ttaubert]
no flags Details
partial (hacky) fix (2.29 KB, patch)
2012-02-29 13:06 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v2 (4.28 KB, patch)
2012-03-07 00:54 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
testcase 3 (786 bytes, text/html)
2012-03-07 00:56 PST, Daniel Holbert [:dholbert]
no flags Details
fix v3 (5.60 KB, patch)
2012-03-07 01:52 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v3a (5.59 KB, patch)
2012-03-07 01:56 PST, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review
testcase 4 (positioned child of -moz-stack) (679 bytes, text/html)
2012-03-07 06:34 PST, Tim Taubert [:ttaubert]
no flags Details
testcase 5 (positioned child of -moz-flex-box) (813 bytes, text/html)
2012-03-07 07:10 PST, Tim Taubert [:ttaubert]
no flags Details
testcase 6 (the fix hopefully shouldn't make this assert) (449 bytes, text/html)
2012-03-07 23:51 PST, Daniel Holbert [:dholbert]
no flags Details
fix v4 (13.37 KB, patch)
2012-03-08 11:09 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v4a (13.37 KB, patch)
2012-03-08 11:12 PST, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review

Description Blair Mitchelmore 2011-03-09 18:49:24 PST
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.
Comment 1 Xiaoyi Shi 2011-03-12 08:09:07 PST
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>
Comment 2 bastian 2011-03-20 05:08:38 PDT
Right, -moz-box-flex in absolutely positioned elements doesn't work. Works fine in WebKit though. Example:  http://jsfiddle.net/WVRH8/
Comment 3 Boris Zbarsky [:bz] 2011-04-13 12:07:36 PDT
*** Bug 649701 has been marked as a duplicate of this bug. ***
Comment 4 Matt Cooper 2011-04-13 12:43:25 PDT
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).
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 14:59:34 PDT
An infrastructure for this has landed in bug 10209.
Comment 6 Tim Taubert [:ttaubert] 2012-02-23 03:46:09 PST
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.
Comment 7 Jet Villegas (:jet) 2012-02-28 13:05:28 PST
Daniel: can you have a look and see if you can help unblock this while you're in the flexbox code?
Comment 8 Daniel Holbert [:dholbert] 2012-02-28 16:06:58 PST
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).
Comment 9 Daniel Holbert [:dholbert] 2012-02-28 16:16:18 PST
Created attachment 601445 [details]
testcase 1 [IGNORE; this is bug 579776] (based on jsfiddle in comment 2)
Comment 10 Daniel Holbert [:dholbert] 2012-02-28 16:46:38 PST
(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.
Comment 11 Daniel Holbert [:dholbert] 2012-02-28 16:48:53 PST
(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.)
Comment 12 Tim Taubert [:ttaubert] 2012-02-29 02:13:53 PST
Created attachment 601562 [details]
testcase 2 (positioned child of -moz-box)

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.
Comment 13 Daniel Holbert [:dholbert] 2012-02-29 13:02:18 PST
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)
Comment 14 Tim Taubert [:ttaubert] 2012-02-29 13:04:28 PST
(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.
Comment 15 Daniel Holbert [:dholbert] 2012-02-29 13:06:49 PST
Created attachment 601729 [details] [diff] [review]
partial (hacky) fix

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.
Comment 16 Daniel Holbert [:dholbert] 2012-02-29 13:10:09 PST
(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.
Comment 17 Dietrich Ayala (:dietrich) 2012-03-06 11:45:17 PST
Is this going to land before next source migration?
Comment 18 Daniel Holbert [:dholbert] 2012-03-06 16:59:01 PST
(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...
Comment 19 Boris Zbarsky [:bz] 2012-03-06 20:25:17 PST
> 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?
Comment 20 Daniel Holbert [:dholbert] 2012-03-06 20:33:33 PST
(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.
Comment 21 Daniel Holbert [:dholbert] 2012-03-07 00:54:36 PST
Created attachment 603633 [details] [diff] [review]
fix v2

For reference, here's the patch that I pushed to Try. (tightened to only affect boxes, per first chunk of previous comment)
Comment 22 Daniel Holbert [:dholbert] 2012-03-07 00:56:22 PST
Created attachment 603635 [details]
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 )
Comment 23 Daniel Holbert [:dholbert] 2012-03-07 01:52:47 PST
Created attachment 603641 [details] [diff] [review]
fix v3

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 )
Comment 24 Daniel Holbert [:dholbert] 2012-03-07 01:56:12 PST
Created attachment 603642 [details] [diff] [review]
fix v3a

(forgot to qref, so previous patch-version was missing the s/FinishReflowWithAbsoluteFrames/ReflowAbsoluteFrames/ change. Fixed now.)
Comment 25 Mozilla RelEng Bot 2012-03-07 02:00:49 PST
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 26 Tim Taubert [:ttaubert] 2012-03-07 02:05:16 PST
Comment on attachment 603642 [details] [diff] [review]
fix v3a

Seems to work pretty well for me.
Comment 27 Mozilla RelEng Bot 2012-03-07 03:45:27 PST
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
Comment 28 Tim Taubert [:ttaubert] 2012-03-07 06:34:04 PST
Created attachment 603694 [details]
testcase 4 (positioned child of -moz-stack)

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".
Comment 29 Tim Taubert [:ttaubert] 2012-03-07 07:10:02 PST
Created attachment 603711 [details]
testcase 5 (positioned child of -moz-flex-box)

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.
Comment 30 Daniel Holbert [:dholbert] 2012-03-07 11:46:45 PST
(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.
Comment 31 Daniel Holbert [:dholbert] 2012-03-07 13:02:08 PST
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 32 Boris Zbarsky [:bz] 2012-03-07 21:25:54 PST
Comment on attachment 603642 [details] [diff] [review]
fix v3a

r=me
Comment 33 Daniel Holbert [:dholbert] 2012-03-07 23:51:18 PST
Created attachment 603985 [details]
testcase 6 (the fix hopefully shouldn't make this assert)

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.
Comment 34 Daniel Holbert [:dholbert] 2012-03-08 00:09:54 PST
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)
Comment 35 Daniel Holbert [:dholbert] 2012-03-08 00:16:09 PST
(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)
Comment 36 Boris Zbarsky [:bz] 2012-03-08 07:59:59 PST
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....
Comment 37 Daniel Holbert [:dholbert] 2012-03-08 09:07:41 PST
Ah, of course! That indeed fixes it. I'll repost with that tweak & automated tests.
Comment 38 Daniel Holbert [:dholbert] 2012-03-08 11:09:39 PST
Created attachment 604132 [details] [diff] [review]
fix v4

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.
Comment 39 Daniel Holbert [:dholbert] 2012-03-08 11:10:41 PST
Comment on attachment 604132 [details] [diff] [review]
fix v4

(er, just  noticed a typo in a reftest header-comment; fixing & reposting)
Comment 40 Daniel Holbert [:dholbert] 2012-03-08 11:12:07 PST
Created attachment 604134 [details] [diff] [review]
fix v4a
Comment 41 Mozilla RelEng Bot 2012-03-08 11:40:30 PST
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.
Comment 42 Daniel Holbert [:dholbert] 2012-03-08 11:56:00 PST
(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.)
Comment 43 Daniel Holbert [:dholbert] 2012-03-08 12:03:25 PST
( Try build w/ patch v4: https://tbpl.mozilla.org/?tree=Try&rev=5a0fd7868ded )
Comment 44 Daniel Holbert [:dholbert] 2012-03-08 16:14:21 PST
(also fwiw, I filed bug 734214 on whatever autoland weirdness happened in comment 41)
Comment 45 Boris Zbarsky [:bz] 2012-03-09 23:14:09 PST
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?
Comment 46 Daniel Holbert [:dholbert] 2012-03-10 14:36:07 PST
(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!
Comment 47 Daniel Holbert [:dholbert] 2012-03-10 14:53:24 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d069fd0abc23
Comment 48 Daniel Holbert [:dholbert] 2012-03-11 19:57:22 PDT
https://hg.mozilla.org/mozilla-central/rev/d069fd0abc23

Note You need to log in before you can comment on or make changes to this bug.