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

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Daniel BODEA]

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" 
align="center">
<div>
test
</div>
</div>

2. same thing with a <center> tag

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

3. the TABLE problem

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

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

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.

Comment 1

[Daniel BODEA]

Attachment: A screen shot

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 3

[Greg K.]

Confirming using Mac/2001091311 (0.9.4). I have constructed a testcase, which I shall 
attach momentarily.

Comment 4

[Greg K.]

Attachment: Testcase exhibiting the problem described in several examples

Comment 5

[karnaze (gone)]

Reassigning to attinasi.

Comment 6

[Daniel BODEA]

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 
target.style.left = "25px" for instance has no effect whatsoever on the layer 
(TABLE).

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

Comment 7

[Kevin McCluskey (gone)]

Problem still exists with N6.2 build on WINNT.

Comment 8

[Marc Attinasi]

Moving Mozilla 1.01 bugs to 'future' milestone with priority P1

I will be pulling bugs from 'future' milestones when scheduling later work.

Comment 9

[Arunan Balasubramaniam]

Attachment: Proposed patch

Comment 10

[Arunan Balasubramaniam]

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 http://bugzilla.mozilla.org/show_bug.cgi?id=55694#c6, 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.

Comment 11

[Håkan Waara]

Waterson, Attinasi, can you guys have a look at arunan's patch?

Comment 12

[Chris Waterson]

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.

Comment 13

[Arunan Balasubramaniam]

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

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

The code takes a different path for one block which is why you don't see this
bug on a single DIV.

Comment 14

[Arunan Balasubramaniam]

Requesting review on teeny weeny patch.

Bug still there in 2002093015 and still fixed in current CVS by proposed patch.

Comment 15

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

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?

Comment 16

[Arunan Balasubramaniam]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Arunan Balasubramaniam]

Attachment: Test 1 for NS_STYLE_FLOAT_EDGE_CONTENT

Comment 19

[Arunan Balasubramaniam]

Attachment: Test 2 for NS_STYLE_FLOAT_EDGE_CONTENT

Comment 20

[Arunan Balasubramaniam]

Attachment: Result of patching the NS_STYLE_FLOAT_EDGE_CASE

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.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 22

[Arunan Balasubramaniam]

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;
  else
    result = 3;
else
  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].

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 24

[Arunan Balasubramaniam]

Attachment: Tests for NS_STYLE_FLOAT_EDGE_xxx

Comment 25

[Arunan Balasubramaniam]

Attachment: Second patch

The comment made me think twice about adding the check everywhere.  Is it a bit
much?

Comment 26

[Arunan Balasubramaniam]

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
http://bugzilla.mozilla.org/show_bug.cgi?id=55694#c6.  I can't see the
difference in 38157-a.

Comment 27

[Arunan Balasubramaniam]

Requesting review.

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

----

Ok, I made a new testcase:

   http://www.hixie.ch/tests/adhoc/css/box/absolute/width/001.html

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

Comment 28

[Arunan Balasubramaniam]

This patch also changes the behaviour in the test case for Bug 139550.

Comment 29

[Arunan Balasubramaniam]

Sorry - "changes the behaviour" means that the first test case in bug 139550
will now shrink wrap instead of spreading out to 100% width.

Comment 30

[Arunan Balasubramaniam]

Discussion in Bug 139550 says that both the old and new behaviour are wrong, so
it's not quite a regression.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 102058 [details] [diff] [review]
Second patch

r=roc+moz

Comment 32

[Arunan Balasubramaniam]

Seeking sr, reassign to self.

Comment 33

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

Comment on attachment 102058 [details] [diff] [review]
Second patch

sr=dbaron

Do you need me to check this in?

Comment 34

[Arunan Balasubramaniam]

Yes, please check this in.  Cheers.

Comment 35

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

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.

Comment 36

[Arunan Balasubramaniam]

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

Comment 37

[Bernd]

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.

Comment 38

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

The backout was bug 194014.

Comment 39

[Arunan Balasubramaniam]

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.

Comment 40

[Arunan Balasubramaniam]

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 :)

Comment 41

[Mats Palmgren (inactive)]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Yeah, this is definitely fixed by the reflow branch.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reftests

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.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Attachment: Improved reftests

Fixed the margin hackery and switched over to using ems instead of pixels.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 256994 [details] [diff] [review]
Improved reftests

r+ from bz on IRC