The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Future

Status

()

Core
Layout
P1
trivial
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: Daniel BODEA, Unassigned)

Tracking

({css2, testcase})

Trunk
Future
css2, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 47803 [details]
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.

Comment 3

16 years ago
Confirming using Mac/2001091311 (0.9.4). I have constructed a testcase, which I shall 
attach momentarily.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

16 years ago
Created attachment 49477 [details]
Testcase exhibiting the problem described in several examples

Comment 5

16 years ago
Reassigning to attinasi.
Assignee: karnaze → attinasi
Keywords: css2
(Reporter)

Comment 6

16 years ago
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...
Problem still exists with N6.2 build on WINNT.
Target Milestone: --- → mozilla1.0.1

Comment 8

15 years ago
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
Created attachment 79833 [details] [diff] [review]
Proposed patch
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

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

Comment 12

15 years ago
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
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.

Updated

15 years ago
Keywords: patch

Updated

15 years ago
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
proved.
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.
Created attachment 102011 [details]
Test 1 for NS_STYLE_FLOAT_EDGE_CONTENT
Created attachment 102013 [details]
Test 2 for NS_STYLE_FLOAT_EDGE_CONTENT
Created attachment 102015 [details]
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.
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;
  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].
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.
Created attachment 102054 [details]
Tests for NS_STYLE_FLOAT_EDGE_xxx
Created attachment 102058 [details] [diff] [review]
Second patch

The comment made me think twice about adding the check everywhere.  Is it a bit
much?
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
http://bugzilla.mozilla.org/show_bug.cgi?id=55694#c6.  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:

   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.
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.
Attachment #102058 - Flags: review?(dbaron)
Comment on attachment 102058 [details] [diff] [review]
Second patch

r=roc+moz
Attachment #102058 - Flags: review?(dbaron) → review+

Updated

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

sr=dbaron

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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
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?

Comment 37

14 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The backout was bug 194014.
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
Status: REOPENED → NEW
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago10 years ago
Depends on: 300030
Flags: in-testsuite?
Resolution: --- → FIXED
Created attachment 256750 [details] [diff] [review]
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.
Attachment #256750 - Flags: superreview?(dbaron)
Attachment #256750 - Flags: review?(dbaron)
Created attachment 256994 [details] [diff] [review]
Improved reftests

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.