Bug 26397 - Changing position:relative to position:static results in mis-positioned div
Summary: Changing position:relative to position:static results in mis-positioned div
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Robert Hogan
URL: http://www.quirksmode.org/css/positio...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-14 21:32 PDT by Simon Fraser (smfr)
Modified: 2012-11-25 10:36 PST (History)
3 users (show)

See Also:


Attachments
Testcase (1.41 KB, text/html)
2009-06-14 22:03 PDT, Simon Fraser (smfr)
no flags Details
Patch (5.01 KB, patch)
2012-10-08 11:08 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.80 KB, patch)
2012-10-15 12:39 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2012-10-25 09:40 PDT, Robert Hogan
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-06-14 21:32:51 PDT
Quirksmode shows a bug when changing from position: relative to position: static:

http://www.quirksmode.org/css/position.html
Comment 1 Simon Fraser (smfr) 2009-06-14 22:03:02 PDT
Created attachment 31278 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2009-06-14 23:05:10 PDT
This isn't just a layer problem. The renderer for the absolute child isn't laid out after it is moved to a new container.

This fixes it; don't know if it's the best fix:

diff --git a/WebCore/rendering/bidi.cpp b/WebCore/rendering/bidi.cpp
index 635ad9a..28d2869 100644
--- a/WebCore/rendering/bidi.cpp
+++ b/WebCore/rendering/bidi.cpp
@@ -840,16 +840,16 @@ void RenderBlock::layoutInlineChildren(bool relayoutChildren, int& repaintTop, i
                 if (relayoutChildren && (o->style()->paddingLeft().isPercent() || o->style()->paddingRight().isPercent()))
                     o->setPrefWidthsDirty(true, false);
             
-                if (o->isPositioned())
+                if (o->isPositioned()) {
                     o->containingBlock()->insertPositionedObject(box);
-                else {
+                    o->setNeedsLayout(true);
+                } else {
                     if (o->isFloating())
                         floats.append(FloatWithRect(box));
                     else if (fullLayout || o->needsLayout()) // Replaced elements
                         toRenderBox(o)->dirtyLineBoxes(fullLayout);
-
-                    o->layoutIfNeeded();
                 }
+                o->layoutIfNeeded();
             } else if (o->isText() || (o->isRenderInline() && !endOfInline)) {
                 if (fullLayout || o->selfNeedsLayout())
                     dirtyLineBoxesForRenderer(o, fullLayout);
Comment 3 Robert Hogan 2012-10-08 11:08:25 PDT
Created attachment 167566 [details]
Patch
Comment 4 Robert Hogan 2012-10-15 12:39:08 PDT
Created attachment 168756 [details]
Patch
Comment 5 Ojan Vafai 2012-10-24 15:29:09 PDT
Comment on attachment 168756 [details]
Patch

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

R- mainly for the cpp comment. Conceptually, this looks good and I'm happy to approve the next iteration.

As far as the test, we don't have strict guidelines on test style, but we try to keep tests and minimal and clean as possible since we'll need to maintain them. Also, we try to avoid setTimeout as much as possible as that often leads to slow and/or flaky tests.

> Source/WebCore/rendering/RenderBlock.cpp:3736
> +            if (o || (containingBlockPosition == ContainingBlockPositionChanged && !r->style()->hasStaticBlockPosition(isHorizontalWritingMode())))

I'd rather we change this to only look at containingBlockPosition. I think the code would be more clear if we just passed in ContainingBlockPositionChanged for both callers in styleWillChange.

> Source/WebCore/rendering/RenderBlock.h:67
> +enum ContainingBlockPosition { ContainingBlockPositionChanged, ContainingBlockPositionUnchanged };

How about calling this ContainingBlockPositionDifference or something? ContainingBlockPosition would make me think it would have position values (e.g. static, relative, etc).

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
> +	"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

Typically for new tests we just use the HTML doctype:
<!DOCTYPE html>

Also, can you remove the cruft from this test:
-lang attribute
-type attributes
-no need for head/link/meta elements
-use 4 space tabs, not actual tab characters

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:13
> +  if (window.testRunner) {
> +      testRunner.waitUntilDone();
> +  }

One-line if statements should not have brackets.

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:20
> +  window.addEventListener('load', function() {
> +    setTimeout(function() {
> +    	var testEl = document.getElementById('container');
> +    	testEl.style.position = 'static';
> +        setTimeout(testRunner.notifyDone(), 0);
> +    }, 0);
> +  }, false);

I think you can get rid of some of these events/setTimeouts and make the test run faster.

1. Move this block of code to a script element at the end of the body element.
2. Force a layout by grabbing testEl.offsetHeight
3. Change the position.

Then you shouldn't need to waitUntilDone at all and this can be a sync test.

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:33
> +  	padding: 1em;

This isn't needed?

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:51
> +  	position: absolute;
> +  	top: 0;
> +  	right: 0;
> +  	width: 100px;
> +  	height: 100px;

Could move this into a #inner, #marker shared clause to make it more clear that the only difference between them is the background-color.
Comment 6 Robert Hogan 2012-10-25 09:40:28 PDT
Created attachment 170673 [details]
Patch
Comment 7 Ojan Vafai 2012-10-30 12:22:22 PDT
Comment on attachment 170673 [details]
Patch

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

Looks great! Thanks.

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static-expected.html:3
> +<style type="text/css">

Nit: don't need the type attribute.

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static-expected.html:6
> +    margin-left: 100px;
> +    width: 400px;

Nit: indent  is off.

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static-expected.html:37
> +      <div id="inner"></div>

Nit: indent is off.

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:3
> +<style type="text/css">

ditto

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:6
> +    margin-left: 100px;
> +    width: 400px;

ditto

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:38
> +      <div id="inner"></div>

ditto

> LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.html:40
> +    <script type="text/javascript">

nit: don't need the type attribute
Comment 8 Robert Hogan 2012-11-25 10:36:09 PST
Committed r135670: <http://trac.webkit.org/changeset/135670>