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
29.12 KB, image/jpeg
1.14 KB, text/html
575 bytes, text/html
576 bytes, text/html
7.54 KB, image/gif
2.16 KB, text/html
1.14 KB, patch
|Details | Diff | Splinter Review|
7.31 KB, patch
|Details | Diff | Splinter Review|
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: 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reassigning to attinasi.
Assignee: karnaze → attinasi
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
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
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.
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 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.
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.
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.
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+
Seeking sr, reassign to self.
Assignee: attinasi → arunan_bala
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
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.
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...
Yeah, this is definitely fixed by the reflow branch.
Status: NEW → RESOLVED
Closed: 18 years ago → 14 years ago
Depends on: reflow-refactor
Resolution: --- → FIXED
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.
Fixed the margin hackery and switched over to using ems instead of pixels.
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.