205087 - relative positioned elements flow around floats at new position instead of original

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P1)

Description

[Frank de Bot]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030507
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030507

The site
http://www.searchy.nl/traffic/mysqlstat/mysqlstat/cgi-bin/mysqlstat.cgi?t=s001.searchy.nl&skin=
was correctly generated in previous versions of mozilla (1.2.1), but no in the
current version 1.4beta.

Reproducible: Always

Steps to Reproduce:
1. start mozilla (duh)
2. Go to
http://www.searchy.nl/traffic/mysqlstat/mysqlstat/cgi-bin/mysqlstat.cgi?t=s001.searchy.nl&skin=
3. See it for your self

Actual Results:  
Shown website correctly

Expected Results:  
Show website on a wrong manner (It was correct in 1.2.1)

Comment 1

[Madhur Bhatia]

please attach a screenshot of how the website looked in 1.2.1 and how does it
look in 1.4beta. Thanx!!

Try shift + reload to see if that fixes the display.

Comment 2

[Frank de Bot]

Attachment: Website in mozilla 1.2.1

Website in mozilla 1.2.1

Comment 3

[Frank de Bot]

Attachment: Website in mozilla 1.4b

Website in mozilla 1.4b

Comment 4

[Andrew Schultz]

Attachment: testcase

with Mozilla linux trunk 20030509, the green box with "bar"s is displayed to
the right of the red box with "foo"s.
with Mozilla 1.2.1, the green box is displayed below (and partially
overlapping) the red box.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The space manager translation needs to ignore relative positioning.

This was probably exposed by making relative positioning not cause the creation
of a new space manager (float formatting context).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: the logic of what needs to be done to fix this

This patch contains the logic of what needs to be done to fix this. 
Unfortunately, it won't compile, and it's a bit of a pain to make it compile,
since we don't have a reflow state to get the computed offsets from during
state recovery.

This is the kind of bug that's hard to fix given our current architecture.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 194124 has been marked as a duplicate of this bug. ***

Comment 8

[Madhur Bhatia]

testcase attachment 123447 [details] from a duped bug 194124

Comment 9

[Frank de Bot]

Attachment: Website in mozilla 1.4rc1

The website in mozilla 1.4rc1  The site shows different, but it still wrong.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I should just use a frame property to fix this, probably.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138313 [details] [diff] [review]
patch

>Index: layout/html/base/src/nsBlockReflowContext.cpp
>@@ -364,24 +373,41 @@ nsBlockReflowContext::ReflowBlock(const 
>+    if (offsets)
>+      *offsets = nsPoint(aComputedOffsets.left, aComputedOffsets.top);

Why not just |offsets->MoveTo(aComputedOffsets.left, aComputedOffsets.top) ? 
Doesn't really matter either way, I guess.

>Index: layout/html/base/src/nsBlockReflowState.cpp

>+    if (kid && !(kid->GetStateBits() & NS_BLOCK_SPACE_MGR)) {

Add a comment explaining why the NS_BLOCK_SPACE_MGR check needs to be there?

And add a comment here in general explaining why we're mucking with the coord
systems?

>+        nsPoint *offsets;
>+        nsIFrameManager *frameManager = mPresContext->GetFrameManager();
>+        frameManager->GetFrameProperty(kid,
>+                                       nsLayoutAtoms::computedOffsetProperty,
>+                                       0, (void**)&offsets);
>+        tx -= offsets->x;
>+        ty -= offsets->y;

If we had OOM earlier when allocating offsets, this is bad.  Should probably
null-check offsets....

>@@ -957,26 +970,24 @@ nsBlockReflowState::FlowAndPlaceFloat(ns

>-      nsIFrame* prevInFlow;
>-      floatFrame->GetPrevInFlow(&prevInFlow);

I assume that this was not supposed to be clobbering the prevInFlow from the
preceding code?  Or is it the same prevInFlow?

r+sr=bzbarsky with those comments addressed.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This addresses bz's comments.  (Regarding the last comment -- it's a fix for a
-Wshadow warning (I still use -Wshadow), and the existing variable was for the
same frame.)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2004-01-10 20:29 -0800.