Last Comment Bug 205087 - relative positioned elements flow around floats at new position instead of original
: relative positioned elements flow around floats at new position instead of or...
Status: RESOLVED FIXED
[patch]
: regression, testcase
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.7alpha
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Hixie (not reading bugmail)
Mentors:
http://www.searchy.nl/traffic/mysqlst...
: 194124 (view as bug list)
Depends on:
Blocks: 218913 222739
  Show dependency treegraph
 
Reported: 2003-05-09 14:13 PDT by Frank de Bot
Modified: 2004-01-10 20:30 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Website in mozilla 1.2.1 (39.81 KB, image/png)
2003-05-09 18:04 PDT, Frank de Bot
no flags Details
Website in mozilla 1.4b (56.89 KB, image/png)
2003-05-09 18:04 PDT, Frank de Bot
no flags Details
testcase (370 bytes, text/html)
2003-05-09 21:20 PDT, Andrew Schultz
no flags Details
the logic of what needs to be done to fix this (5.38 KB, patch)
2003-05-09 21:38 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
Website in mozilla 1.4rc1 (54.73 KB, image/png)
2003-06-03 07:41 PDT, Frank de Bot
no flags Details
patch (12.91 KB, patch)
2004-01-02 16:13 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch (13.10 KB, patch)
2004-01-10 20:22 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review

Description Frank de Bot 2003-05-09 14:13:32 PDT
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 2003-05-09 16:33:42 PDT
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 2003-05-09 18:04:06 PDT
Created attachment 122904 [details]
Website in mozilla 1.2.1

Website in mozilla 1.2.1
Comment 3 Frank de Bot 2003-05-09 18:04:41 PDT
Created attachment 122905 [details]
Website in mozilla 1.4b

Website in mozilla 1.4b
Comment 4 Andrew Schultz 2003-05-09 21:20:39 PDT
Created attachment 122914 [details]
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-7 (review requests must explain patch) 2003-05-09 21:25:32 PDT
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-7 (review requests must explain patch) 2003-05-09 21:38:28 PDT
Created attachment 122915 [details] [diff] [review]
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-7 (review requests must explain patch) 2003-05-15 14:37:26 PDT
*** Bug 194124 has been marked as a duplicate of this bug. ***
Comment 8 Madhur Bhatia 2003-05-15 14:42:35 PDT
testcase attachment 123447 [details] from a duped bug 194124
Comment 9 Frank de Bot 2003-06-03 07:41:37 PDT
Created attachment 124827 [details]
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-7 (review requests must explain patch) 2003-09-11 10:35:31 PDT
I should just use a frame property to fix this, probably.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 16:13:58 PST
Created attachment 138313 [details] [diff] [review]
patch
Comment 12 Boris Zbarsky [:bz] 2004-01-10 16:36:14 PST
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-7 (review requests must explain patch) 2004-01-10 20:22:46 PST
Created attachment 138787 [details] [diff] [review]
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-7 (review requests must explain patch) 2004-01-10 20:30:52 PST
Fix checked in to trunk, 2004-01-10 20:29 -0800.

Note You need to log in before you can comment on or make changes to this bug.