Bug 65477 - Nested fixed position element not staying with parent
Summary: Nested fixed position element not staying with parent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-01 10:48 PDT by age
Modified: 2013-01-17 17:51 PST (History)
15 users (show)

See Also:


Attachments
Reduction (795 bytes, text/html)
2011-09-05 12:52 PDT, Robert Hogan
no flags Details
Another test case (834 bytes, text/html)
2011-09-06 12:11 PDT, Dave Hyatt
no flags Details
Patch (63.59 KB, patch)
2011-09-20 04:38 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (11.76 KB, patch)
2011-12-18 07:40 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (11.78 KB, patch)
2011-12-18 08:04 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-12-09 10:32 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2012-12-10 10:17 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2012-12-13 12:16 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (17.32 KB, patch)
2012-12-20 10:02 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (19.32 KB, patch)
2013-01-15 12:46 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description age 2011-08-01 10:48:53 PDT
Whenever there is a fixed-position element nested within any parent(s) with relative positioning, it will not move/remain with the parent until the window is resized. Here is an example:

http://jsfiddle.net/tcA6M/

If the #wrap div is changed to absolute or fixed, the box will behave correctly. As is, if you resize the window after the move, the box will reposition.
Comment 1 James Robinson 2011-08-01 16:05:15 PDT
Regression range 81970 to 82211 (http://trac.webkit.org/log/?rev=82211&stop_rev=81970&verbose=on).
Comment 2 James Robinson 2011-08-01 16:06:16 PDT
Possibly http://trac.webkit.org/changeset/82144/?
Comment 3 Robert Hogan 2011-09-05 12:52:00 PDT
Created attachment 106355 [details]
Reduction
Comment 4 Robert Hogan 2011-09-05 14:01:57 PDT
(In reply to comment #2)
> Possibly http://trac.webkit.org/changeset/82144/?

I tried it - that's not it. Almost certainly http://trac.webkit.org/changeset/81992/
Comment 5 Dave Hyatt 2011-09-06 12:11:44 PDT
Created attachment 106460 [details]
Another test case

r81992 did break that particular test case, but only because it went to the optimized layout path. The bug still exists even prior to r81992. r81922 just widened the set of cases that would get caught by it.  See the attached test case that is entirely using only absolute/fixed positioned elements and no relative positioned ones.
Comment 6 Dave Hyatt 2011-09-06 12:54:13 PDT
We just need to refine this case a bit:


// When a non-positioned block element moves, it may have positioned children that are implicitly positioned relative to the
        // non-positioned block.  Rather than trying to detect all of these movement cases, we just always lay out positioned
        // objects that are positioned implicitly like this.  Such objects are rare, and so in typical DHTML menu usage (where everything is
        // positioned explicitly) this should not incur a performance penalty.
        if (relayoutChildren || (r->style()->hasStaticBlockPosition(isHorizontalWritingMode()) && r->parent() != this && r->parent()->isBlockFlow()))
            r->setChildNeedsLayout(true, false);

I think this check needs to be carefully broadened. We need to try to think of all the cases that exist right now where a static distance computation that needs to be redone despite the block not needing layout.

There are two kinds of layouts that can occur to force a static distance computation to need to be re-evaluated:

(1) A positioned block moves.
(2) Normal flow layout causes the need for a recomputation.

In both cases the values cached for the distance just from your container are correct. The issue is you need to actually be marked as needing a layout.

There are a number of possibilities here:
(1) We just recompute the static distance every time in layoutPositionedObjects. This seems like it could get slow for large numbers of positioned objects though, but it might be a good first cut. You would basically do computeLogicalWidth if you had a static inline distance and computeLogicalHeight if you had a static block inline distance, and then mark as needing layout if the static dis

(2) The only way to not be in the containing block hierarchy that I can think of is to be Fixed Positioned. So for fixed positioned objects we could just always re-calculate the static distances.

(3) If you're in the containing block hierarchy, then I think you only need to re-evaluate if you needed a normal flow layout, i.e., if childNeedsLayout or selfNeedsLayout. So I think we could add that limitation.

In addition we could refine this dirtying to do a layout that is more like the positioned movement layout, i.e., and only do a layout if width/height are going to change. (The mere act of testing the new widths/heights will recompute the static distance for us.)
Comment 7 Dave Hyatt 2011-09-06 13:22:17 PDT
Good first cut is probably just to have a special case for fixed positioned elements, but it would be nice to detect that there is an intermediate positioned block in the way before wasting time recomputing static distance always.

So basically something like this (in pseudocode):

if (position is fixed and ancestor is absolute positioned and hasstaticinlinedistance || hasstaticblockdistance) {
   recompute logical width;
   recompute logical height;
   if (width or height changed)
      setChildNeedsLayout;
}

I think that should do it. This could be instead of the other check that is already in place for non-positioned block movement.
Comment 8 Dave Hyatt 2011-09-06 13:30:14 PDT
Oh and even in simplified layout you'll have to check fixed positioned kids that have the intermediate positioned object that can screw with their movement. That will be kind of annoying...
Comment 9 Robert Hogan 2011-09-20 04:38:36 PDT
Created attachment 107982 [details]
Patch
Comment 10 Dave Hyatt 2011-11-04 12:15:39 PDT
Comment on attachment 107982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107982&action=review

r-

> Source/WebCore/rendering/RenderBlock.cpp:2247
> +        if  (r->parent()->style()->position() != AbsolutePosition)
> +            continue;

This needs to be a full-blown ancestor check all the way up the tree I think? I don't think you can get away with just a parent check. Try making a test case with a <span> in between the absolute positioned parent and the fixed positioned child and you should see what I mean.

> Source/WebCore/rendering/RenderBlock.cpp:2286
> +        if (relayoutChildren || ((r->style()->hasStaticBlockPosition(isHorizontalWritingMode()) || r->style()->hasStaticInlinePosition(isHorizontalWritingMode())) 
> +            && r->parent() != this))

I still think this check is all wrong and worry that just adding this in unqualified is too broad. See comment #6 in the bug.

Fixing this only in simplified layout will also result in the bug not being fixed if you have to do a full layout. Should come up with test cases for that (dirty more to make simplified layout not trigger).
Comment 11 Robert Hogan 2011-12-18 07:40:08 PST
Created attachment 119761 [details]
Patch
Comment 12 Robert Hogan 2011-12-18 08:04:05 PST
(In reply to comment #10)
> (From update of attachment 107982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107982&action=review
> 
> r-
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2247
> > +        if  (r->parent()->style()->position() != AbsolutePosition)
> > +            continue;
> 
> This needs to be a full-blown ancestor check all the way up the tree I think? I don't think you can get away with just a parent check. Try making a test case with a <span> in between the absolute positioned parent and the fixed positioned child and you should see what I mean.

Yes fixed I think, along with an updated test.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:2286
> > +        if (relayoutChildren || ((r->style()->hasStaticBlockPosition(isHorizontalWritingMode()) || r->style()->hasStaticInlinePosition(isHorizontalWritingMode())) 
> > +            && r->parent() != this))
> 
> I still think this check is all wrong and worry that just adding this in unqualified is too broad. See comment #6 in the bug.

Instead of broadening this check I've relied on the fact that a fixed position child of an absolute position ancestor will always be after it in the positioned objects list. Now, if an absolute positioned element in the list needs layout then any fixed position element after it in the list will also get a layout (if it is not parented to the RenderView).

> 
> Fixing this only in simplified layout will also result in the bug not being fixed if you have to do a full layout. Should come up with test cases for that (dirty more to make simplified layout not trigger).

I'm sure it can be done but I couldn't think of a way of triggering a full layout to coincide with just moving the absolute element.
Comment 13 Robert Hogan 2011-12-18 08:04:49 PST
Created attachment 119763 [details]
Patch
Comment 14 Julien Chaffraix 2012-04-19 15:58:27 PDT
Comment on attachment 119763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119763&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2215
> +    } else if (isRenderView() || hasTransform()) {
> +        // If an absolute position element inside a relative positioned container moves, and the absolute element has a fixed position
> +        // child, neither the renderview nor the fixed element learn of the movement since posChildNeedsLayout() is only marked as far as the 
> +        // relative positioned container. So check if any fixed position elements in the renderview's need to move with their absolute ancestors.
> +        layoutFixedPositionObjects();
> +    }

Why do we need a separate layout function? (which means another positioned object traversal). Isn't it possible to actually not layout some positioned child because of your logic?

> Source/WebCore/rendering/RenderBlock.cpp:2305
> +    bool positionedAncestorForcesLayout = false;

The name is misleading. It's not the ancestor that you are checking but the siblings.

> Source/WebCore/rendering/RenderBlock.cpp:2319
> +        if (r->style()->position() == AbsolutePosition && r->needsLayout())
> +            positionedAncestorForcesLayout = true;
> +        else if (r->style()->position() == FixedPosition && positionedAncestorForcesLayout && r->parent() != this)
> +            r->setChildNeedsLayout(true, false);

I don't think I understand this logic. If you have seen an absolute position sibling then it impacts this sibling |r|. It doesn't sound good to me.

> LayoutTests/fast/inline/bug65477-fixed-position-movement.html:55
> +  <div id='wrap'>

The test should include the bug title and number. Also it would be easier to see what's wrong if you actually said what you expect and why. (I am guessing we would put the children under the wrong parent but that's a guess).
Comment 15 Robert Hogan 2012-04-20 11:00:13 PDT
Thanks for the review, Julien!

A few things to clarify before I revisit this patch. I think I have answers to all your challenges so it may be a case of seeing if there is a better way of doing this.

(In reply to comment #14)
> Why do we need a separate layout function? 

For the reason stated more verbosely in the comment, but which boils down to: posChildNeedsLayout() is only set as far as a relative container - so a fixed position object that is a child of an absolute element doesn't get laid out here when it needs to be.

>(which means another positioned object traversal). 

In the code as it stands, there's no extra traversal.

> Isn't it possible to actually not layout some positioned child because of your logic?

I don't think so. Currently layoutPositionedObjects() is only called if posChildNeedsLayout() is true, and this patch keeps it that way.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:2305
> > +    bool positionedAncestorForcesLayout = false;
> 
> The name is misleading. It's not the ancestor that you are checking but the siblings.

It's not misleading, just counter-intuitive! The positioned objects list doesn't contain siblings, they can be from a number of different areas in the rendertree.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:2319
> > +        if (r->style()->position() == AbsolutePosition && r->needsLayout())
> > +            positionedAncestorForcesLayout = true;
> > +        else if (r->style()->position() == FixedPosition && positionedAncestorForcesLayout && r->parent() != this)
> > +            r->setChildNeedsLayout(true, false);
> 
> I don't think I understand this logic. If you have seen an absolute position sibling then it impacts this sibling |r|. It doesn't sound good to me.

See above, they're not siblings.

> 
> > LayoutTests/fast/inline/bug65477-fixed-position-movement.html:55
> > +  <div id='wrap'>
> 
> The test should include the bug title and number. Also it would be easier to see what's wrong if you actually said what you expect and why. (I am guessing we would put the children under the wrong parent but that's a guess).

Will do.
Comment 16 Robert Hogan 2012-12-09 10:32:06 PST
Created attachment 178426 [details]
Patch
Comment 17 WebKit Review Bot 2012-12-09 14:33:51 PST
Comment on attachment 178426 [details]
Patch

Attachment 178426 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15230316

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment 18 Build Bot 2012-12-09 19:05:33 PST
Comment on attachment 178426 [details]
Patch

Attachment 178426 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15214897

New failing tests:
fast/inline/fixed-pos-moves-with-abspos-parent.html
fast/loader/javascript-url-in-object.html
fast/inline/fixed-pos-moves-with-abspos-parent-relative-ancestor.html
Comment 19 Robert Hogan 2012-12-10 10:17:25 PST
Created attachment 178583 [details]
Patch
Comment 20 Julien Chaffraix 2012-12-11 15:03:07 PST
Comment on attachment 178583 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178583&action=review

Dave mentioned using RenderBlock::layoutPositionedObjects for the fix instead of tying it to style change. Your earlier efforts went in this direction, is there something that made your rethink the approach?

One upside of a layout approach is that it removes the need for an extra positioned object traversal. Also during RenderBlock::layoutPositionedObjects, you know your containing block and it should fix both full and simplified layout.

> Source/WebCore/ChangeLog:17
> +        (WebCore):

Let's remove these nasty lines.

> Source/WebCore/ChangeLog:18
> +        (WebCore::RenderBlock::styleWillChange):

Does this code need to run on styleWillChange? AFAICT unless you really need to do some mutation pre-style change, this should be moved to styleDidChange (see for example, the code for floats).

> Source/WebCore/rendering/RenderBlock.cpp:288
> +static void setFixedPositionDescendantsNeedLayout(RenderBlock* child)

The naming is just confusing: |child| is actually the starting renderer and you pass |this| to it.

> Source/WebCore/rendering/RenderBlock.cpp:294
> +    RenderBox* fixedPositionObject;

We usually define the variable when we need them.

> Source/WebCore/rendering/RenderBlock.cpp:308
> +        while (parent && !parent->isRenderView()) {
> +            if (parent == child) {
> +                fixedPositionObject->setChildNeedsLayout(true, MarkContainingBlockChain);
> +                return;
> +            }
> +            parent = parent->parent();
> +        }

I don't understand the need for this loop. A positioned object gets inserted into its containing block's map (look at the logic in RenderBlock::handlePositionedChild) so fixedPositionedObject->containingBlock() should ALWAYS be |child|.
Comment 21 Robert Hogan 2012-12-12 07:39:36 PST
(In reply to comment #20)
> (From update of attachment 178583 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178583&action=review
> Dave mentioned using RenderBlock::layoutPositionedObjects for the fix instead of tying it to style change. Your earlier efforts went in this direction, is there something that made your rethink the approach?

The problem I'm trying to solve is that a fixed position child of an absolute positioned object has to move with its parent if it doesn't have a position of its own. The location of the fixed position child is controlled by its containing block, the RenderView. So whenever the absolute positioned object moves, the RenderView needs to know that one of its fixed position objects needs a layout too.

Rather than try to work out whether this situation has arisen every time we do a layout, I've confined detecting it to the time the absolute positioned object actually changes its position. 


> One upside of a layout approach is that it removes the need for an extra positioned object traversal. Also during RenderBlock::layoutPositionedObjects, you know your containing block and it should fix both full and simplified layout.

No matter where we do this, we will need a check that establishes if the absolute positioned object has a fixed position child or vice versa. Whenever that situation arises, the RenderView needs to know about, the fixed position object needs a layout set, and the RenderView needs to layout its positioned objects to ensure the fixed position object is positioned correctly.


> > Source/WebCore/ChangeLog:18
> > +        (WebCore::RenderBlock::styleWillChange):
> Does this code need to run on styleWillChange? AFAICT unless you really need to do some mutation pre-style change, this should be moved to styleDidChange (see for example, the code for floats).

The choice was arbitrary, I'll follow your suggestion.

> > Source/WebCore/rendering/RenderBlock.cpp:294
> > +    RenderBox* fixedPositionObject;
> We usually define the variable when we need them.
> > Source/WebCore/rendering/RenderBlock.cpp:308
> > +        while (parent && !parent->isRenderView()) {
> > +            if (parent == child) {
> > +                fixedPositionObject->setChildNeedsLayout(true, MarkContainingBlockChain);
> > +                return;
> > +            }
> > +            parent = parent->parent();
> > +        }
> I don't understand the need for this loop. A positioned object gets inserted into its containing block's map (look at the logic in RenderBlock::handlePositionedChild) so fixedPositionedObject->containingBlock() should ALWAYS be |child|.

No, fixedPositionedObject->containingBlock() is always the RenderView, unless the tree has become detached in some way I guess - but it is never |child|, which is always the absolutely positioned parent of the fixed position object.
Comment 22 Julien Chaffraix 2012-12-12 07:56:12 PST
Comment on attachment 178583 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178583&action=review

>>> Source/WebCore/rendering/RenderBlock.cpp:294
>>> +    RenderBox* fixedPositionObject;
>> 
>> We usually define the variable when we need them.
> 
> No, fixedPositionedObject->containingBlock() is always the RenderView, unless the tree has become detached in some way I guess - but it is never |child|, which is always the absolutely positioned parent of the fixed position object.

I disagree with your analysis (and see that I missed the child->view()->positionedObjects() call, which btw makes the patch even *more* wrong from my perspective as you are poking at random - probably unrelated - objects). Look at RenderObject::containingBlock as there are 3 cases where we may not return the RenderView: transform, svg foreign object and flow thread.
Comment 23 Robert Hogan 2012-12-12 09:31:03 PST
(In reply to comment #22)
> (From update of attachment 178583 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178583&action=review
> >>> Source/WebCore/rendering/RenderBlock.cpp:294
> >>> +    RenderBox* fixedPositionObject;
> >> 
> >> We usually define the variable when we need them.
> > 
> > No, fixedPositionedObject->containingBlock() is always the RenderView, unless the tree has become detached in some way I guess - but it is never |child|, which is always the absolutely positioned parent of the fixed position object.
> I disagree with your analysis (and see that I missed the child->view()->positionedObjects() call, which btw makes the patch even *more* wrong from my perspective as you are poking at random - probably unrelated - objects). Look at RenderObject::containingBlock as there are 3 cases where we may not return the RenderView: transform, svg foreign object and flow thread.

I stand corrected on that point - the containingBlock() for a fixed position can be a couple of other things than just the RenderView. To account for such cases, instead of child->view()->positionedObjects() I would have to climb child's ancestors and collect their object lists all the way up to the RenderView.

That seems a reasonable improvement to me, but I sense your objection to the patch is broader than just that. 

My problem is that I think I've justified it, bar that correction you've pointed out, so need to know what else is the matter with it.
Comment 24 Robert Hogan 2012-12-13 12:16:53 PST
Created attachment 179313 [details]
Patch
Comment 25 Eric Seidel (no email) 2012-12-13 12:33:25 PST
Comment on attachment 179313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179313&action=review

> Source/WebCore/rendering/RenderBlock.cpp:302
> +    // Find the nearest ancestor that could contain a fixed position descendant of |ancestor| in its positioned objects list.
> +    RenderObject* o = ancestor->parent();
> +    while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock()) && !o->isRenderFlowThread()) {
> +#if ENABLE(SVG)
> +        if (o->isSVGForeignObject())
> +            break;
> +#endif
> +        o = o->parent();
> +    }
> +
> +    if (!o || !o->isRenderBlock())
> +        return;

Shouldn't this be a helper function? :)  RenderBlock* nearestAncestorFixedPositionRoot()? or something?
Comment 26 Julien Chaffraix 2012-12-19 10:20:58 PST
Comment on attachment 179313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179313&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:302
>> +        return;
> 
> Shouldn't this be a helper function? :)  RenderBlock* nearestAncestorFixedPositionRoot()? or something?

Yes, it's called containingBlock() but we can't use it here as ancestor->position() != FixedPosition. As discussed on IRC, this is a big red flag to copy containingBlock and is a definite NO! for me as we have had tons of bugs due to container() vs containingBlock() differences. This code is so subtle that another copy is guaranteed to bite us down the road.
Comment 27 Robert Hogan 2012-12-20 10:02:06 PST
Created attachment 180356 [details]
Patch
Comment 28 Julien Chaffraix 2013-01-10 11:01:59 PST
Comment on attachment 180356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180356&action=review

I prefer this approach. Ideally, you would want Dave to give a cursory glance on it.

> Source/WebCore/rendering/RenderBlock.cpp:2588
> +    bool canContainFixedPosObjects = isRenderView() || hasTransform()
> +#if ENABLE(SVG)
> +                                    || isSVGForeignObject()
> +#endif
> +                                    || isRenderFlowThread();

This should go into a helper method as it is duplicated with RenderObject::containingBlock.

> Source/WebCore/rendering/RenderBlock.cpp:2621
> +    if (!child->style()->hasStaticBlockPosition(isHorizontalWritingMode()) && !child->style()->hasStaticInlinePosition(isHorizontalWritingMode()))
> +        return;

You could store the result of hasStaticBlockPosition / hasStaticInlinePosition for further use below.

> Source/WebCore/rendering/RenderBlock.cpp:2631
> +        LayoutUnit oldLeft = box->logicalLeft();

s/oldLeft/oldLogicalLeft

> Source/WebCore/rendering/RenderBlock.cpp:2634
> +        LayoutUnit left = box->logicalLeft();
> +        if (left != oldLeft) {

No need to store box->logicalLeft() here:

if (box->logicalLeft() != oldLogicalLeft)

> Source/WebCore/rendering/RenderBlock.cpp:2639
> +            child->setChildNeedsLayout(true, MarkOnlyThis);
> +            return;
> +        }
> +    }
> +    if (child->style()->hasStaticBlockPosition(isHorizontalWritingMode())) {

The return seems very artificial and is needed only because you don't put:

else if (child->style()->hasStaticBlockPosition(isHorizontalWritingMode())) {

> LayoutTests/ChangeLog:15
> +        * fast/inline/fixed-pos-moves-with-abspos-inline-parent-expected.txt: Added.
> +        * fast/inline/fixed-pos-moves-with-abspos-inline-parent.html: Added.
> +        * fast/inline/fixed-pos-moves-with-abspos-parent-expected.txt: Added.
> +        * fast/inline/fixed-pos-moves-with-abspos-parent-relative-ancestor-expected.txt: Added.
> +        * fast/inline/fixed-pos-moves-with-abspos-parent-relative-ancestor.html: Added.
> +        * fast/inline/fixed-pos-moves-with-abspos-parent.html: Added.
> +        * fast/inline/fixed-pos-with-transform-container-moves-with-abspos-parent-expected.txt: Added.
> +        * fast/inline/fixed-pos-with-transform-container-moves-with-abspos-parent.html: Added.

The tests could really be converted to check-layout.js and they would be shorter (thus more readable).

> LayoutTests/fast/inline/fixed-pos-moves-with-abspos-parent-relative-ancestor.html:32
> +        testRunner.waitUntilDone();

This looks wrong. You shouldn't need any deferred logic. If you need layout to run, you can always force it.

> LayoutTests/fast/inline/fixed-pos-moves-with-abspos-parent-relative-ancestor.html:38
> +        if (document.getElementById('inner').offsetLeft >= 200) {

It's better to put an equality as it would nice unwanted changes to the value.

> LayoutTests/fast/inline/fixed-pos-moves-with-abspos-parent.html:10
> +        #wrap {
> +        position: absolute;
> +        width: 400px;
> +        height: 200px;
> +        border: 2px solid black; left:0; top:0;
> +        }

Let's try to keep a consistent style. Here you are not indenting the CSS rules and also several rules are on the same (last) line.
Comment 29 Robert Hogan 2013-01-15 12:46:08 PST
Created attachment 182830 [details]
Patch
Comment 30 Dave Hyatt 2013-01-17 11:13:36 PST
Comment on attachment 182830 [details]
Patch

r=me
Comment 31 WebKit Review Bot 2013-01-17 12:21:38 PST
Comment on attachment 182830 [details]
Patch

Clearing flags on attachment: 182830

Committed r140024: <http://trac.webkit.org/changeset/140024>
Comment 32 WebKit Review Bot 2013-01-17 12:21:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Hin-Chung Lam 2013-01-17 17:51:35 PST
This change seems to be causing a crash on Chromium debug builds for fast/inline/fixed-pos-moves-with-abspos-parent-relative-ancestor.html

Here's the test history:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Finline%2Ffixed-pos-moves-with-abspos-parent-relative-ancestor.html

Crash log on Mac 10.7 (dbg):

STDOUT: <empty>
STDERR: ASSERTION FAILED: !currBox->needsLayout()
STDERR: ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp(7769) : void WebCore::RenderBlock::checkPositionedObjectsNeedLayout()
STDERR: 1   0x725b681 WebCore::RenderBlock::checkPositionedObjectsNeedLayout()
STDERR: 2   0x745cf61 WebCore::RenderObject::checkBlockPositionedObjectsNeedLayout()
STDERR: 3   0x60af4bd WebCore::RenderObject::setNeedsLayout(bool, WebCore::MarkingBehavior)
STDERR: 4   0x723078f WebCore::RenderBlock::simplifiedLayout()
STDERR: 5   0x722f383 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit)
STDERR: 6   0x722e4c3 WebCore::RenderBlock::layout()
STDERR: 7   0x7540dc1 WebCore::RenderView::layoutContent(WebCore::LayoutState const&)
STDERR: 8   0x75416b7 WebCore::RenderView::layout()
STDERR: 9   0x832befb WebCore::FrameView::layout(bool)
STDERR: 10  0x6018c2f WebCore::Document::updateLayout()
STDERR: 11  0x6018d5a WebCore::Document::updateLayoutIgnorePendingStylesheets()
STDERR: 12  0x60f8598 WebCore::Element::offsetLeft()
STDERR: 13  0x6873872 _ZN7WebCore17ElementV8InternalL20offsetLeftAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE
STDERR: 14  0x10c7ed0 v8::internal::JSObject::GetPropertyWithCallback(v8::internal::Object*, v8::internal::Object*, v8::internal::String*)
STDERR: 15  0x10c79cc v8::internal::Object::GetProperty(v8::internal::Object*, v8::internal::LookupResult*, v8::internal::String*, PropertyAttributes*)
STDERR: 16  0x1009662 v8::internal::LoadIC::Load(v8::internal::InlineCacheState, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::String>)
STDERR: 17  0x101156e v8::internal::LoadIC_Miss(v8::internal::Arguments, v8::internal::Isolate*)
STDERR: 18  0x4420a3f6
STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef
STDERR:  [0x000003cd29ef]
STDERR:  [0x000003cd298b]
STDERR:  [0x000003cd261b]
STDERR:  [0x000096f1059b]
STDERR:  [0x0000ffffffff]
STDERR:  [0x00000745cf61]
STDERR:  [0x0000060af4bd]
STDERR:  [0x00000723078f]
STDERR:  [0x00000722f383]
STDERR:  [0x00000722e4c3]
STDERR:  [0x000007540dc1]
STDERR:  [0x0000075416b7]
STDERR:  [0x00000832befb]
STDERR:  [0x000006018c2f]
STDERR:  [0x000006018d5a]
STDERR:  [0x0000060f8598]
STDERR:  [0x000006873872]
STDERR:  [0x0000010c7ed0]
STDERR:  [0x0000010c79cc]
STDERR:  [0x000001009662]
STDERR:  [0x00000101156e]
STDERR:  [0x00004420a3f6]
STDERR: ax: bbadbeef, bx: 7b85f15c, cx: c1d15030, dx: c1d15030
STDERR: di: 8814569, si: 881452b, bp: c0056878, sp: c0056810, ss: 23, flags: 10282
STDERR: ip: 725b68b, cs: 1b, ds: 23, es: 23, fs: 0, gs: f