Bug 26397

Summary:

Changing position:relative to position:static results in mis-positioned div

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

Layout and Rendering

Assignee:

Robert Hogan <robert>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, hyatt, robert

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

URL:

http://www.quirksmode.org/css/position.html

Attachments:

Description	Flags
Testcase	none
Patch	none
Patch	none
Patch	ojan: review+

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	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
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.