Closed Bug 97777 Opened 19 years ago Closed 14 years ago

An absolutely positioned DIV with centered alignment adds extra space around the block elements it contains


(Core :: Layout, defect, P1)






(Reporter: dali, Unassigned)



(Keywords: css2, testcase)


(8 files, 2 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    Gecko/20010726

It's simple. The DIV can either have the align="center" attribute or contain a 
<center></center> block or the div equivalent, all the other block elements 
inside get aligned in the center, but the DIV stretches by adding arbitrary 
extra space on the left and right sides, space that I haven't been able to cut 
out in any way.

If one of the block elements inside the DIV is a TABLE with the align="center" 
attribute, it gets worse... the DIV stretches up to the right window border.

Reproducible: Always
Steps to Reproduce:
1. first case: the align="center" attribute

<div style="position: absolute; top: 200px; left: 200px; border: solid black" 

2. same thing with a <center> tag

<div style="position: absolute; top: 200px; left: 200px; border: solid 

3. the TABLE problem

<div style="position: absolute; top: 200px; left: 200px; border: solid black">
<table align="center"></tr></td>

Actual Results:  The extra space added randomly changes the position of the 
elements, making it impossible to predetermine the actual location of the 

Expected Results:  Keep the DIV packed all the time (as long as there is no 
with property). IE5.5 implements the right behaviour.

Tested with the same results under MacOS 8.6 and Win2k so logicaly it's the 
same thing on the other platforms too.
Attached image A screen shot
Reporter: please attach the full HTML page you are working with.  There could be
extra tags that come into play or even a DOCTYPE or not.
Confirming using Mac/2001091311 (0.9.4). I have constructed a testcase, which I shall 
attach momentarily.
Ever confirmed: true
Reassigning to attinasi.
Assignee: karnaze → attinasi
Keywords: css2
Related to this problem... I replaced the DIV with a TABLE (position: absolute) 
in order to center align things. It works just fine except the fact that = "25px" for instance has no effect whatsoever on the layer 

The only solution I found to get this done properly is to wrap a TABLE in an 
absolute DIV, and center the cell of the table. The style.left problem seems to 
me somehow essential and could be solved theoretically by breaking the hard 
links between the general DOM implementation and certain elements of the DOM 
tree, that is by turning the global scope of the DOM access methods REALLY 
global. Theoretically...
Problem still exists with N6.2 build on WINNT.
Target Milestone: --- → mozilla1.0.1
Moving Mozilla 1.01 bugs to 'future' milestone with priority P1

I will be pulling bugs from 'future' milestones when scheduling later work.
Priority: -- → P1
Target Milestone: mozilla1.0.1 → Future
Attached patch Proposed patch (obsolete) — Splinter Review
Hi there.  I know this isn't HELPWANTED, but it is FUTURE and I'm really
interested in Gecko.  Marc Attinasi said he didn't mind people having a
go at his futured bugs, so I did.

The patch just attached fixes the test case.  Running the regression tests
gave the following errors:

block\bugs\verify\38157-a.rgd failed         
block\bugs\verify\97777.rgd failed           
block\bugs\verify\58652.rgd failed           
table\bugs\verify\bug29157.rgd failed        
table\bugs\verify\bug29326.rgd failed        
table\bugs\verify\bug33855.rgd failed        
table\bugs\verify\bug4382.rgd failed         
table\bugs\verify\bug45055-2.rgd failed      
table\bugs\verify\bug55694.rgd failed        
table\bugs\verify\bug89315.rgd failed        
table\bugs\verify\bug96334.rgd failed        
formctls\base\verify\select_js.rgd failed

The regression test seems to work differently some times (it's crashing
every other time I run it) so I don't know if I'm doing something wrong
with it.

Of those failures above, only bug 58652 and bug 55694 looked different
to me.  And in, the
new behaviour is put forward as correct.

The patch only modifies two paths through the function, leaving all of
those in the switch statement as they are.  I couldn't work out a test
case to test those paths, but I'd guess they might need the same sort of
thing added.
Waterson, Attinasi, can you guys have a look at arunan's patch?
My understanding of shrink-wrapping is shaky at best. This patch seems
reasonable to me; I'll defer to attinasi, dbaron, and roc who may know more.

My only comment is that it seems like we'd also want to apply the same logic in
the situations where we've got floaters, too.
Should this bug have "patch" added to the keywords?

While I'm writing, I realised I should have written an explanation for the patch
[I didn't originally as I didn't want to preach on egg sucking ;)], so:

The problem is that the shrink wrapping code is not calculating the greatest
width of the block's children.  Because it was using a finite space as the
available space in the initial "width finding" reflow, the children would align
themselves within that space.  Because they aligned themselves, the supposed
width for each was actually the left padding + the width.  The second
"positioning" reflow uses this incorrect width, and then the children align
themselves within that.

You can see this because the space added within the block is equal to the space
outside to the right of the block.  Putting more DIVs inside each other further
sub-divides things in a funky way (3 nested DIVs take 75% of the available space

The patch forces the available space to be infinite (by using
NS_UNCONSTRAINEDSIZE) so the children cannot align themselves during the first

The code takes a different path for one block which is why you don't see this
bug on a single DIV.
Keywords: patch
Keywords: review
Requesting review on teeny weeny patch.

Bug still there in 2002093015 and still fixed in current CVS by proposed patch.
For the first change, it looks like you should also be changing the codepaths
where floaters are present.  I really don't know why we have so many different
ways of doing roughly the same thing, or what the differences are.  Do you?
Can't say I know the differences for sure; doing "minor" bugs like this was my
attempt to understand Gecko.  But:

The different code paths in that function are used to determine the space
available to content in elements which may be of block-ish type or not, noting
when floaters may or may not be present.  I don't know the architecture enough
to comment on "why" though.  LXR points out a couple of bugs that may be of
interest (not posting links as I want to be entirely clear in my head first).

I am going to try again to create test cases for the floater code paths, as it
seems everbody agrees they probably need the same fix and it just needs to be
I like the patch. I think it should apply across all paths. You can do that by
just adding an if (GetFlag(SHRINKWRAPWIDTH)) at the end of the function.
OK, this is what happens when you apply the same patch to the
NS_STYLE_FLOAT_EDGE case; better but not correct for the right floater.  I'm
going to attempt to create tests for the other NS_STYLE_FLOAT_xxx cases and see
if this problem is the same across all of them.
Looks good. We just don't handle shrink-wrapping right floaters well in general,
because we can't place them against the right edge. That's not your problem.

Would it be OK to add the "if (GetFlag(SHRINKWRAPWIDTH))" check to each
aResult.Width = (foo) ? (bar) : (wibble) check instead of at the end?  I would
prefer to do it this way as the logic currently is like (basically):

if (a)
  if (b)
    result = 1;
  else if (c)
    result = 2;
    result = 3;
  result = 4;

Importantly, at the moment aResult.width only gets set once per code path.  If
"if (GetFlag(SHRINKWRAPWIDTH))" were added at the bottom, aResult.width could
get set per code path and *also* could get reset at the bottom for all code
paths which IMHO is less clear.

Changes would be to lines 265, 295, 319 [and the already done 329 and 339].
I wouldn't object violently, but I would point out that if you set it once on
each code path you're duplicating code and also introducing the possibility that
someone will add or change a code path and forget to do your check.
Attached patch Second patchSplinter Review
The comment made me think twice about adding the check everywhere.  Is it a bit
Attachment #79833 - Attachment is obsolete: true
Running the regression tests four times, the only regressions in all runs were:

block\bugs\verify\38157-a.rgd failed         
table\bugs\verify\bug55694.rgd failed        

55694 is different in the same way as before, still now behaving as suggested in  I can't see the
difference in 38157-a.
Requesting review.

I consulted Hixie about his comment in Bug 55694.  Here's what he said:


Ok, I made a new testcase:

[wrt 55694]
> Currently the testcase has the green box pushed out to the left edge
> of the screen (i.e. 100% width).  With the patch attached, the green
> box gets shrink-wrapped.

Shrink-wrapping is the correct behaviour. However, this might break some
top sites, so there's a good chance we'll have to add a quirk for this
behaviour, so if you check this in please be on the look-out for
regressions from this fix.


I would be happy to have a go at adding the quirk if required.
This patch also changes the behaviour in the test case for Bug 139550.
Sorry - "changes the behaviour" means that the first test case in bug 139550
will now shrink wrap instead of spreading out to 100% width.
Discussion in Bug 139550 says that both the old and new behaviour are wrong, so
it's not quite a regression.
Comment on attachment 102058 [details] [diff] [review]
Second patch

Attachment #102058 - Flags: review?(dbaron) → review+
Attachment #102058 - Flags: superreview?(dbaron)
Seeking sr, reassign to self.
Assignee: attinasi → arunan_bala
Comment on attachment 102058 [details] [diff] [review]
Second patch


Do you need me to check this in?
Attachment #102058 - Flags: superreview?(dbaron) → superreview+
Yes, please check this in.  Cheers.
Fix checked in to trunk, 2003-01-08 18:24 PDT.
(nsBlockReflowState.cpp revision 3.463)

Thanks for the patch, and sorry for the long delay in reviewing it.

Marking bug FIXED.
Closed: 18 years ago
Resolution: --- → FIXED
Summary: An absolute positioned DIV with centered alignement adds extra space around the block elements it contains → An absolutely positioned DIV with centered alignment adds extra space around the block elements it contains
> Thanks for the patch, and sorry for the long delay in reviewing it.

No problem.

I'm just wondering about the regression tests in the source tree.  Should one or
more of the test cases get added to CVS too?
I backed out the patch as the patch collides with the shrinkwrapping that tables
do. It caused regressions on percentage width based tables as already noticed
during the testing of this patch (the failure at the testcase for bug 55694) and
it caused a unconstrained incremental reflow, which tables cannot handle.

I will attach some testcases that this patch has broken that a correct patch
should not.
Resolution: FIXED → ---
First, I am sorry about causing headaches running up to 1.3 with bug 194014.

Secondly, I would still like to fix this bug.  I think that my next steps
would be to:

  - Make this a standards mode only patch as suggested in comment 27.
  - Fix the case found in bug 194014 where an image added to a table was
not handled correctly (and test the fix in both standards and quirks mode).

  Before I did this though I would like to confirm that change to percentage
table widths in standards mode is acceptable, as I don't want to waste
anybody else's time again.
Giving up, sorry.  Don't have the free time I did when I started on this.  Gecko
will be rewritten before I get to sit down and figure out tables :)
Assignee: arunan_bala → position
Severity: normal → trivial
This seems to have been fixed by bug 300030. The layout of the testcases
in SeaMonkey 2006120801 on Linux looks reasonable to me, but I don't
know for sure how -moz-float-edge should behave...
Keywords: testcase
Yeah, this is definitely fixed by the reflow branch.
Closed: 18 years ago14 years ago
Depends on: reflow-refactor
Flags: in-testsuite?
Resolution: --- → FIXED
Attached patch Reftests (obsolete) — Splinter Review
These are based on two of the testcases posted to the bug. dbaron, I'm curious as to what you think of my workaround for the second test. I had to adjust the margins slightly to compensate for the fact that I was using border-collapse: collapse. That styling was necessary in order for the table-based version to display the same as the div-based version.
Attachment #256750 - Flags: superreview?(dbaron)
Attachment #256750 - Flags: review?(dbaron)
Fixed the margin hackery and switched over to using ems instead of pixels.
Attachment #256750 - Attachment is obsolete: true
Attachment #256750 - Flags: superreview?(dbaron)
Attachment #256750 - Flags: review?(dbaron)
Whiteboard: [checkin needed]
Comment on attachment 256994 [details] [diff] [review]
Improved reftests

r+ from bz on IRC
Attachment #256994 - Flags: review+
Flags: in-testsuite? → in-testsuite+
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.