Closed
Bug 851514
Opened 12 years ago
Closed 12 years ago
auto offsets (top/left) on position:fixed element don't work with an overflow:hidden;position:fixed parent element
Categories
(Core :: Layout: Positioned, defect)
Core
Layout: Positioned
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: magicskeer, Assigned: bzbarsky)
References
Details
(Keywords: testcase)
Attachments
(3 files)
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.
Reporter | ||
Comment 2•12 years ago
|
||
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/
Flags: needinfo?(magicskeer)
Comment 3•12 years ago
|
||
Comment 4•12 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Style System (CSS)
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
Version: 19 Branch → Trunk
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.)
Component: Style System (CSS) → Layout: R & A Pos
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Transition a parent element, then setting overflow to hidden causes a child with position fixed to also be hidden → auto offsets (top/left) on position:fixed element don't work with an overflow:hidden;position:fixed parent element
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.
Flags: needinfo?(bzbarsky)
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).
Assignee | ||
Comment 9•12 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 10•12 years ago
|
||
And if I take out the overflow:hidden, then I get the right order in the FixedList, looks like.
Assignee | ||
Comment 11•12 years ago
|
||
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....
Flags: needinfo?(roc)
I very strongly prefer B. Handling failure in frame construction is a nightmare already.
Flags: needinfo?(roc)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #726683 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [need review]
Comment 15•12 years ago
|
||
This is something to similar to Bug 850015.
Assignee | ||
Comment 16•12 years ago
|
||
Yeah, similar like a duplicate. ;)
Assignee | ||
Comment 18•12 years ago
|
||
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?
Attachment #726683 -
Flags: review?(dbaron) → review?(dholbert)
Comment 19•12 years ago
|
||
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.
Comment 20•12 years ago
|
||
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.
Comment 21•12 years ago
|
||
(er s/&&/&)
Comment 22•12 years ago
|
||
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)
Attachment #726683 -
Flags: review?(dholbert) → review+
Sorry about dropping that request on the floor.
Assignee | ||
Comment 24•12 years ago
|
||
> 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.
Assignee | ||
Comment 25•12 years ago
|
||
Oh, and good catch about the <meta> in the static testcase!
Comment 26•12 years ago
|
||
Thanks!
Assignee | ||
Comment 27•12 years ago
|
||
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Comment 28•12 years ago
|
||
(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".)
Assignee | ||
Comment 29•12 years ago
|
||
Yikes. Sorry about that. :(
Comment 30•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•