User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130307023931 Steps to reproduce: As seen on the example: http://jsfiddle.net/XUM3y/2/ I created a fixed div a fixed child, then transitioned it, maintaining its original overflow value. When the transition finishes, I reset its overflow, which becomes set to hidden. Furthermore, I also adjust the margin-left of the child. Actual results: The child element disappeared from the viewport. Expected results: The child element should be still visible, and shifted to the right by a bit in comparison to its origin.

Can you provide/attach a jQuery-less Testcase?

Here it is. I simplified it a bit, but the same behavior if I am not mistaken. Furthermore, adjusting the height/width of the browser (or similar actions like zoom) will cause the div to reappear. http://jsfiddle.net/V7zmK/3/

Thanks for the Testcase. Repro'd with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130317 Firefox/22.0 ID:20130317030923 CSet: 0b052daa913c This seems not to be a Regression since even modifying the Testcase to use -moz-transition reveals that this happens since transition is supported.

So: (1) when the button is clicked: parent.style.overflow = 'visible'; sets overflow:visible on the parent, overriding: parent.className = 'hide'; which also sets width:0, margin-left: 30px (which are not overriden) which starts a transition on 'width' and 'margin-left' (which can animate) (2) when the transition ends, we fire the transitionend event, which does: parent.style.overflow = ''; which removes the overriding set in (1), making the #parent.hide rule kick in and set the parent to overflow:hidden, which hides the child child.className = 'shift'; which sets a negative margin on the child Any further clicks of the "hide" button set overflow to visible, but don't set any animatable properties, so don't trigger transitions (and thus the transitionend event), so overflow stays visible after later clicks of the hide button. I think all the weirdness is coming from the handling of 'top:auto' and 'left:auto' on the child when the parent has overflow:hidden. (It turns out the child isn't hidden, it's just positioned offscreen.)

Probably the first thing to investigate here is this code: http://hg.mozilla.org/mozilla-central/file/b2636816c7fd/layout/generic/nsHTMLReflowState.cpp#l1223 which was originally added for bug 268111. Tossing a needinfo? to bz about this, though I think this is a relatively obscure case that's been broken a long time, so probably not a high priority.

Another possibility is just that position:fixed elements are reflowed in the wrong order for making this work correctly, though I'd expect that they'd be reflow in what I'd consider the right order (depth-first pre-order traversal of tree).

So I converted the inner fixed-pos thing into a span, and then a frame dump on the viewport shows: FixedList< Block(span)(1)@0x126c12c20 next=0x126c12a18 [content=0x122f9aa00] {0,0,0,0} [state=0000162000d00702] sc=0x11375eb10(i=0,b=0)< > HTMLScroll(div)(1)@0x126c12a18 {0,0,0,0} [state=0000060000000502] [content=0x122f9a880] [sc=0x126b8ef60]< Block(div)(1)@0x126c12b80 [content=0x122f9a880] {0,0,0,0} [state=0000102000d00602] sc=0x11375e0f8(i=1,b=0) pst=:-moz-scrolled-content< line 0x126c12d10: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} < Placeholder(span)(1)@0x126c12cb8 {0,0,0,0} [state=0000000000400402] [content=0x122f9aa00] [sc=0x126b8de18] outOfFlowFrame=Block(span)(1)@0x126c12c20 > > > which explains the goings-on. What's not clear to me yet is why we end up with that frametree.

And if I take out the overflow:hidden, then I get the right order in the FixedList, looks like.

So the problem is that this is how scrollframes work (at least in ConstructScrollableBlock): 1) BeginBuildingScrollFrame to create the scrollframe. 2) Create the scrolled frame. 3) ConstructBlock on the scrolledFrame (which construcs its descendants) 4) FinishBuildingScrollFrame. 5) AddChild the scrollframe. In particular, the kid gets added to the fixed list before the parent in this case. And then at paint time we sort things correctly so they paint in the right order, iirc. This can really only happen for fixed-pos, and I think I recall running into this before... We handle the painting part, like I said, but it does lead to this problem with auto offsets. So I think we have more or less three options here: A) AddChild the scrollframe before its descendant stuff is done and then handle failure in the descendants by removing it from the constructor state somehow. B) As (A), but make all frame construction stuff infallible (which it might be already) so we don't have to worry about failure. C) Add some sort of fixed-pos-specific hackery to ConstructScrollableBlock that will ensure the ordering in the FixedList matches content ordering. Robert, thoughts? I obviously rather like (B), but it's a bit of a project....

I very strongly prefer B. Handling failure in frame construction is a nightmare already.

OK, let's give that a shot.

This is something to similar to Bug 850015.

Yeah, similar like a duplicate. ;)

Comment on attachment 726683 [details] [diff] [review] Make sure that we add the parent frame before its kids when constructing a scrollable block, so that things will appear in the right order in fixed-pos lists. Gah, this missed two cycles now waiting for review. Daniel, are you happy reviewing this?

Probably. I'm just reading through the involved code a bit to make sure I understand what's going on. Observation: the static testcase attachment on this bug only renders incorrectly ~50% of the time for me, whereas the one on the duplicate bug 850015 fails 100% of the time. The relevant difference is the <meta charset> tag on the attachment here, which triggers a randomly-timed call to nsPresContext::DoChangeCharSet(), which IIRC triggers style re-resolution and apparently gives us a chance to get correct rendering (due to an incremental reflow or a repaint), even in builds without the patch applied (like current nightly). So: if you remove that <meta> tag, you get a 100% reliably-failing static testcase. If you like, it might be worth including a static variant of the testcase in the patch (with no <meta> tag), alongside the patch's existing dynamic testcase.

Also, for the record -- correct me if I'm wrong: so, the reason the purple box ends up at (0,0) is that we reflow it *before* we ever reflow its placeholder, and so when we query the placeholder for the static position, it gives us the (bogus) 0,0 position. Correct? And then on subsequent reflows (e.g. after the <meta charset> tag fires, or after you resize the window), we end up positioning it correctly because *now* the placeholder frame now has a meaningful position. (?) Assuming I'm understanding that correctly, it seems like this means we should be able to catch this sort of problem by asserting that a placeholder frame always gets its first reflow before its out-of-flow frame's first reflow. I.e., we could add something like MOZ_ASSERT(mState & NS_FRAME_FIRST_REFLOW || !mOutOfFlowFrame->GetStateBits() && NS_FRAME_FIRST_REFLOW, "Our out-of-flow frame shouldn't get reflowed before us..."); That doesn't necessarily have to happen here (and I'm not 100% sure whether it'd apply for floats with placeholders as well, or just abspos/fixedpos), but it seems like it'd be worth doing to catch stuff like this in the future.

(er s/&&/&)

Comment on attachment 726683 [details] [diff] [review] Make sure that we add the parent frame before its kids when constructing a scrollable block, so that things will appear in the right order in fixed-pos lists. r=me, anyway, modulo my above optional suggestions. (the static testcase and the assertion)

Sorry about dropping that request on the floor.

> we reflow it *before* we ever reflow its placeholder, and so when we query the > placeholder for the static position, it gives us the (bogus) 0,0 position. Correct? Yes, exactly. > we end up positioning it correctly because *now* the placeholder frame now has a > meaningful position. As far as I can tell, yes. The assert suggestion is interesting. I filed bug 874418 on it and will see how try likes it. A priori it seems like a reasonable thing to assert, for sure.

Oh, and good catch about the <meta> in the static testcase!

Thanks!

(In reply to Boris Zbarsky (:bz) from comment #27) > https://hg.mozilla.org/integration/mozilla-inbound/rev/31b951eae30d (Not a huge deal, but the landed patch still had "r=dbaron" in commit message, when it meant "r=dholbert".)

Yikes. Sorry about that. :(