RESOLVED FIXED 26397
Changing position:relative to position:static results in mis-positioned div 
https://bugs.webkit.org/show_bug.cgi?id=26397
Summary Changing position:relative to position:static results in mis-positioned div
Simon Fraser (smfr)
Reported 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
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
DetailsFormatted DiffDiff
Patch (8.80 KB, patch)
2012-10-15 12:39 PDT, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (8.29 KB, patch)
2012-10-25 09:40 PDT, Robert Hogan 		ojan: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2009-06-14 22:03:02 PDT
Created attachment 31278 [details] Testcase
Simon Fraser (smfr)
Comment 2 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);
Robert Hogan
Comment 3 2012-10-08 11:08:25 PDT
Created attachment 167566 [details] Patch
Robert Hogan
Comment 4 2012-10-15 12:39:08 PDT
Created attachment 168756 [details] Patch
Ojan Vafai
Comment 5 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.
Robert Hogan
Comment 6 2012-10-25 09:40:28 PDT
Created attachment 170673 [details] Patch
Ojan Vafai
Comment 7 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
Robert Hogan
Comment 8 2012-11-25 10:36:09 PST
Committed r135670: <http://trac.webkit.org/changeset/135670>
Note You need to log in before you can comment on or make changes to this bug.