User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030401 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030401 span or div blocks using absolute postioning fail to render properly beyond the initial viewport when scrolling is required. It appears that because the text box isn't rendered, the correct size for the box is never calculated and is thus layed out as if it had no contents. Reproducible: Always Steps to Reproduce: 1.View http://strawberrians.org/Map/backup/DisplayMap.php 2.Scroll to right-hand side Actual Results: Markers beyond the initial viewport are rendered at a size as if they contained no CDATA. The numbers are still layed out directly on the image. Expected Results: Size of bounding box should be calculated for all entities, not just those rendered in the current view. There is a work around, a table entity can be placed within the span or div block, forcing proper display as at http://www.strawberrians.org/Map/Map.php

This only happens when they're to the right, not the bottom, and the same problem that affects the popups on http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports Technically our behavior is correct according to http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width , but I need to try to get that changed.

Er, actually, no, our behavior is wrong according to the current spec, because the "available width" is computed after setting 'left' and 'right' to zero.

*** Bug 182814 has been marked as a duplicate of this bug. ***

Reporter, the URL is not accessible. Maybe a duplicate of bug 170927?

*** Bug 224984 has been marked as a duplicate of this bug. ***

The following code looks a bit odd to me: if (maxWidth <= 0) { maxWidth = 1; } I think it should be: if (maxWidth < 0) { maxWidth = 0; }

Comment on attachment 135027 [details] [diff] [review] Patch rev. 1 Yeah, that should clamp negative values to 0, not what it does. I'm bothered by this whole thing, though. This is setting the computed max-width to the width of the CB if it's unconstrained. But that's not what the spec at http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width says to do... In particular, we are looking at case 3 (auto right/width, non-auto left). Then the spec says to set "right" to zero and solve for width to find "available width". That will give a 0 for available width... Comment 2 doesn't agree with what the spec is actually saying, as far as I can tell (the spec says to set left _or_ right to 0, not to set both to 0).

This whole HTMLReflowState stuff is completely broken and we need a sane system for doing what's described in the spec. I see two changes in the patch: * clean up the code * don't subtract mComputedOffsets. The second one is wrong (according to the spec, at least, and I think it makes some sense). The real problem is that we should be doing (as the spec says): min(max(preferred minimum width, available width), preferred width) but we're really doing: min(available width, preferred width)

Comment on attachment 135027 [details] [diff] [review] Patch rev. 1 Yes, I agree with your comments. My gut feeling was to remove that block too, but then nothing worked so there is missing code elsewhere... I have an almost finished patch that adds that code though... stay tuned.

*** Bug 228781 has been marked as a duplicate of this bug. ***

*** Bug 228781 has been marked as a duplicate of this bug. ***

this is heavy lifting that's not likely to make it into the branch as we work towards stabilization.

Is that something that we can try for 1.8b ?

*** Bug 251774 has been marked as a duplicate of this bug. ***

*** Bug 254937 has been marked as a duplicate of this bug. ***

*** Bug 255330 has been marked as a duplicate of this bug. ***

I have tested this on all URLs + testcases here and on dependants and as far as I can see, it fixes all of them.

A couple of notes on Testcase #4: The first green table is now shrink-wrapped (like the DIV), I believe this is the correct rendering. Not visible in the screenshot, but the second green table only resizes when the window width shrinks - not when it grows - I think this a incremental reflow bug in the table code (all the other boxes grow as expected).

Comment on attachment 160285 [details] [diff] [review] Patch rev. 2 >Index: layout/html/base/src/nsAbsoluteContainingBlock.cpp >+ nscoord availWidth = aReflowState.mComputedWidth; ... >+ if (aContainingBlockWidth != -1) { >+ availWidth = aContainingBlockWidth - >+ kidReflowState.mComputedMargin.left - kidReflowState.mComputedMargin.right; That's inconsistent. We should be subtracting the margin in both cases or neither case, I would think. I suspect doing it in both cases is the right thing to do. >+ kidReflowState.mComputedOffsets.right = 0; Why bother? No one will see this value after this point, will they? >+ kidReflowState.mComputedOffsets.left = 0; Same. >- >+ Random whitespace change? Why? I'm a little worried about performance impact for reflow of auto-width abs pos stuff... but I guess we have no bette way to do this for now. :( Marking r- based on the margin comments above, but the overall approach seems right.

(In reply to comment #26) > That's inconsistent... I suspect doing it in both cases is right... Yes, good catch - fixed. (added Testcase #5 that exposes that error) > Random whitespace change? Why? There are four spaces on that line. Spacing lines between sections of code should be empty (like they are in surrounding code). > I'm a little worried about performance impact for reflow of auto-width Ok, but the problem is that a second reflow is needed in most cases where we should apply the shrink-to-fit algorithm. I have experimented with this a lot and I could only find one case that can be optimised: When we get into the |else if (kidDesiredSize.width < availWidth)| block - (see patch) the second reflow is not needed if the CB is constrained, that is when |aReflowState.mComputedWidth != NS_UNCONSTRAINEDSIZE|. At first I also thought that were more cases but I always found some edge case that would break with only one reflow. I'm attaching this patch mostly for comparison. I have a new version where I folded the added logic into the reflow code below and that implements the optimisation above, but that code is a bit messy right now...

FWIW, doing 1 initial and 2 or more incremental reflows at each URL/testcase: if (kidDesiredSize.mMaxElementWidth > availWidth) { 598 times } else if (kidDesiredSize.width < availWidth) { 372 times } else 317 times

Mats, you could test the performance impact, if any, by taking the testcases at http://www.mozilla.org/performance/test-cases/dhtml/layers1/index.html and http://www.mozilla.org/performance/test-cases/dhtml/layers5/index.html, changing the widths to shrink-wrap, and maybe increasing the number of steps so that you can measure the time elapsed (using Date.now(), etc). I agree that this seems to be the only way to do what we want in the current architecture, but I'd be curious to know whether my worries about performance are founded... ;)

The optimisation I thought was possible failed on the *right* table in Testcase #4 on bug 197022 (attachment 119772 [details]). So, this patch always reflows twice in shrink-to-fit situations. The difference to the last version is that I have folded the two reflows into one code block. This was to implement the (failed) optimisation but I still think this version is better. Now we have that view positioning code before the first reflow which I assume matters (flicker?). Also, if we find a case we can optimise later it will be easier to add. A note on the existing code: I'm surprised that nsAbsoluteContainingBlock::ReflowAbsoluteFrame is called twice for every nsAbsoluteContainingBlock::IncrementalReflow (which means we will reflow shrink-to-fit boxes four times). Is there a reason for this? (I will do performance measurements on the URLs you gave with this patch)

> I'm surprised that nsAbsoluteContainingBlock::ReflowAbsoluteFrame > is called twice The first call is because we have a reflow command for the absolute list (reflows all dirty absolute frames) and the second is because the reflow path contains one of our child frames, right? Could we flip those two chunks around? Presumably any frames we flow because they're in the reflow path would get marked clean after the reflow and we wouldn't flow them twice? Would that screw up the eReflowReason_Initial twiddling in the dirty block, though? Alternately, could we prune frames we reflow because they're dirty from the path so that they're ignored in the second block?

SHIFT-Reloading the Perf testcases 8 times (debug build), gives on average: Without patch: With patch rev. 4: testcase1: 9615ms 9525ms testcase2: 9880ms 9918ms I don't think those testcases do what we want to measure...

Mats, in those testcases reflow happens only once (so most of the time is spent elsewhere). You need to put successive steps on timeouts to give the event loop time to reflow.

Would it be faster to make things work like they do for floats, in nsBlockFrame::ReflowFloat? (Trying to make the code make sense before I land the reflow refactoring stuff may in fact be a bad idea (for performance, at least).)

SHIFT-Reloading the Perf testcases 3 times (debug build), gives on average: Without patch: With patch rev. 4: Perf test 1: R=1:23:80 CP=5000 R=1:31:30 CP=5453 Perf test 2: R=0:47:80 CP=2870 R=0:51:10 CP=3050 which seems to indicate reflow times increased about 8% R="Real time" CP="CP time" as given by the nsTimer framework. (I hacked PresShell::ProcessReflowCommands so it timed all reflow commands into 'mReflowWatch' incrementally). I also verfied that I got the right number of calls to nsAbsoluteContainingBlock::ReflowAbsoluteFrame() this time :-)

OK. 8% isn't so bad...

This is the version that David mentioned in comment 38. 1st reflow is constrained to the available width, a 2nd reflow is only done if |kidDesiredSize.mMaxElementWidth > availWidth| (constrained to MEW this time). I think the performance impact of this change can be ignored since a second reflow likely isn't needed for the majority of abs.pos. boxes. Note that there is one subtle difference to the approach in rev. 4 - percentage-width tables grow to fill the entire avaiable width rather than being shrink-wrapped. See Testcase #4 or #5 - this patch does not change the current rendering whereas rev. 4 renders the first table only as wide as the table content requires.

SHIFT-Reloading the Perf testcases 3 times (debug build), gives on average: Without patch: With patch rev. 5: Perf test 3: R=0:36:00 CP=2167 R=1:29:20 CP=5330 Perf test 4: R=0:23:40 CP=1350 R=0:49:50 CP=2983 which seems to indicate that reflow times increased about 200-300% *for this specific case*. Could it be that the current rendering of Perf test 3/4 degenerates most boxes into width=0, which perhaps can explain why it is so much faster? Anyway, I'm guessing this case is rare...

> Could it be that the current rendering of Perf test 3/4 degenerates > most boxes into width=0 It does. I agree that this case is not something we should be worried about offhand. Testcase 1 and Testcase 2 are much more representative of things we'd actually run into.

Comment on attachment 161216 [details] [diff] [review] Patch rev. 5 (diff -uw) + if (maxWidth <= 0) { + maxWidth = 0; That should be '<' of course...

Your thoughts regarding the difference between the green table/div in attachment 160284 [details] would be appreciated also.

Seems like the big loop you added should be a do { ... } while (0); loop. (More comments later.)

Comment on attachment 161311 [details] [diff] [review] Patch rev. 6 (works like floats) This looks reasonable assuming you've tested what happens when width and right are auto, left is set, and the min-width is set to a value greater than the values you're clamping kidReflowState.mComputedMaxWidth to. That is, can you end up in a state where the kid reflow state has a bigger mComputedMinWidth than mComputedMaxWidth? If so, you probably need to clamp the mComputedMinWidth too or something.

Even if it did, 'min-width' beats 'max-width' which beats 'width'.

Comment on attachment 161311 [details] [diff] [review] Patch rev. 6 (works like floats) Hmm, there is still something wrong with this patch... Compare the "Screenshot of Testcase #3" attachment. The left- and right-most boxes should shrink-wrap around the "LongWord" + padding (the number that follows should be on the next line).

(In reply to comment #53) > Seems like the big loop you added should be a do { ... } while (0); loop. That wouldn't work since I start the second reflow with a "continue".

(In reply to comment #54) > (From update of attachment 161311 [details] [diff] [review]) > That is, can you end up in a state where the kid reflow state has a bigger > mComputedMinWidth than mComputedMaxWidth? Yes, that occurs for the magenta colored boxes in Testcase #3. Fixing it made no difference to the actual layout but the reflow state should be correct at all times of course. Nice catch!

This is the corrected version that fixes the problem mentioned in comment 56. This now works as floats do as far as I can see, the problem is that the float shrink-to-fit does not work in some cases. (I spawned bug 268499). I'm attaching it just for comparison.

This is a slightly updated version of rev. 4, two changes: 1. It uses ComputeContainingBlockRectangle() instead of rolling its own. 2. It uses an enum for the three states of the loop to make it easier to follow. This fixes all the URLs and testcases and the performance should be equal to what I presented in comment 41 (reflow times increases 8%).

Comment on attachment 165228 [details] [diff] [review] Patch rev. 8 r=bzbarsky, but perhaps the reflow state should store the containing block so it doesn't have to be computed twice? File a followup bug on that?

*** Bug 271139 has been marked as a duplicate of this bug. ***

sr=dbaron (based on quick skim and bz's review), although it would be nice if you could avoid the second pass under most conditions.

(In reply to comment #62) > (From update of attachment 165228 [details] [diff] [review]) > r=bzbarsky, but perhaps the reflow state should store the containing block so > it doesn't have to be computed twice? File a followup bug on that? > Did you mean the call to ComputeContainingBlockRectangle? if so, that is only done when "aContainingBlockWidth == -1" so it should only occur once.

Checked in attachment 165228 [details] [diff] [review] at 2004-11-26 15:28 PDT. (except the 0xdeadbeef warning which someone else had already fixed.) Filed bug 271924 on the table {inc} problem mentioned in comment 25. Filed bug 271927 on the double reflow problem in nsAbsoluteContainingBlock::IncrementalReflow (this is not the double reflow for shrink-to-fit), see comment 31 and comment 33. Filed bug 271933 on avoiding some of the shrink-to-fit reflows, see comment 64. -> FIXED

*** Bug 250498 has been marked as a duplicate of this bug. ***

*** Bug 249922 has been marked as a duplicate of this bug. ***

This broke the tabs at the top of http://www.mozilla.org/ (tested with a local backout). I don't see why it should have broken them.

Leak stats also jumped a bit on brad.

(In reply to comment #70) > Leak stats also jumped a bit on brad. --NEW-LEAKS-----------------------------------leaks------leaks%----------------- BandRect 576 -

I filed bug 271990 on the problem at http://www.mozilla.org/ Is it possible that that could also be the cause for the leak?

No.

> Did you mean the call to ComputeContainingBlockRectangle? Yes. > if so, that is only done when "aContainingBlockWidth == -1" so it should only > occur once. Doesn't it also happen in the reflow state constructor? The result is then stored in the copies of aContainingBlockWidth/Height that were passed to that function and doesn't make it back out to this code, as far as I recall...

This caused regression bug 272099 ?

Yes, see comment 69 and comment 72

Could this also have caused bug 272596?

So I'm inclining to think that we should back this out. It's caused a bunch of regressions that don't seem to have obvious fixes, and I'd rather not ship 1.8 with a completely different set of bugs than 1.7 and then fix them all with the reflow refactoring landing right afterwards. Then again, there may be some simple fix for those regressions (perhaps by doing it just like floats do?), but I don't see it immediately.

(In reply to comment #78) > So I'm inclining to think that we should back this out Yes, I'm not really comfortable with the regressions either. I still believe the patch is the right way to do it though - it's the underlying code that doesn't play nicely (tables and placement of right floats). But I could not find an easy way to fix that. > (perhaps by doing it just like floats do?) I'll post an updated version of "Patch rev. 7" that works like floats. Please note that this is not entirely correct though (bug 268499), but it does fix almost all of the intended R&A bugs without having the problems reported in the regression bugs.

I also fixed the problem Boris pointed out regarding the reflow reason: + aReason = eReflowReason_Resize; + continue; // Do a second reflow constrained to MEW.

Comment on attachment 167700 [details] [diff] [review] Patch rev. 9 (works like floats) >Index: layout/html/base/src/nsAbsoluteContainingBlock.cpp >+ situation = NOT_SHRINK_TO_FIT; // This is the last reflow >+ kidReflowState.mComputedWidth = PR_MIN(availWidth, kidReflowState.mComputedMaxWidth); Doesn't this need to make sure mComputedWidth doesn't drop below mComputedMinWidth? Other than that, r=bzbarsky.

Have you tested that this does the right thing in all 4 cases (fixed width either with width or non-auto offsets, intrinsic sizing to min-width, intrinsic sizing to container, intrinsic sizing to max-width) with border and padding? Some of the border/padding manipulation looks a little funny (should it really be subtracted from mew to get the new availwidth?). Setting availWidth to aReflowState.mComputedWidth doesn't make much sense to me. Shouldn't it just be NS_UNCONSTRAINEDSIZE if there's no containing block width? Or does it not make a difference since we'll either use the computed width or the width is already NS_UNCONSTRAINEDSIZE? If so, maybe NS_UNCONSTRAINEDSIZE is clearer. I don't claim to understand this, but if it works I guess it's ok.

Comment on attachment 167700 [details] [diff] [review] Patch rev. 9 (works like floats) sr=dbaron with bz's comment addressed so we can get this in and stop accumulating regression reports, but I hope you can look at the issues I raised sometime soon.

I fixed the min-width problem that Boris pointed out, and also added back a couple of lines that I missed from rev. 7. I have tested the cases requested in comment 82 in addition to the block regression tests, all the targeted R&A bugs and the regressions caused by revision 8 and all of that seems to work as intended now. I found one case where I expected a margin-right/left:auto element to be centered, but this does not seem to work before either. I'll file that separately if needed.

(In reply to comment #84) > I found one case where I expected a margin-right/left:auto element to be > centered, but this does not seem to work before either. That's bug 182748...

Checked in "Patch rev. 10" to fix the regressions, 2004-12-05 16:27 PDT.

Looks like this regressed my pages at http://www.fynf.at/ (they're a nice testcase for a few layout things actually), as the right darker block/bar (which has position:absolute) now gets a computed value of "right:30px" here, though it explicitely has "right:0em" set. with fixed width, margin, and padding set, and left not set (auto), I'd expect the block to be at the right edge of the view (and have "right:0px" as computed value)...

Erm, forgot to say, my regression happens with the last "Patch rev. 10" checkin and didn't happen before, I believe it worked correctly before that latest patch.

Sorry to add another comment, but I noticed that the left edge gets computed correctly in my regression, but "width:120px;right:0em;" get computed to "width:90px;right:30px;", according to DOM inspector.

So the problem is that we're overriding mComputedMaxWidth when the width is set.... (the else if (NS_UNCONSTRAINEDSIZE != availWidth) branch is taken for NOT_SHRINK_TO_FIT, which seems wrong).

(In reply to comment #90) > So the problem is that we're overriding mComputedMaxWidth when the width is > set.... (the else if (NS_UNCONSTRAINEDSIZE != availWidth) branch is taken for > NOT_SHRINK_TO_FIT, which seems wrong). That was indeed the problem, thanks Boris.

Comment on attachment 168077 [details] [diff] [review] Patch B rev. 1 (correction for rev. 10) Yeah, this looks better. Maybe toss in an assert in that branch that availWidth is not NS_UNCONSTRAINEDSIZE. It shouldn't be, based on code inspection, but...

(In reply to comment #93) > Yeah, this looks better. Maybe toss in an assert in that branch that > availWidth is not NS_UNCONSTRAINEDSIZE. I actually had a printf for that during testing and it didn't show up, I'll add it all the same...

If it _did_ show up, we'd want a check, not an assert... ;)

With rev. 10 we got back our lost BandRects: --FIXED-LEAKS---------------------------------leaks------leaks%----- BandRect 0 -100.00% http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1102301220.26523.gz&fulltext=1

Comment on attachment 168077 [details] [diff] [review] Patch B rev. 1 (correction for rev. 10) don't forget to land this. :-) (The file is in layout/generic/ now.)

Comment on attachment 168077 [details] [diff] [review] Patch B rev. 1 (correction for rev. 10) Added an assertion and checked in 2004-12-12 07:57 PDT

I have filed the remaining case separately, bug 275179. -> FIXED

*** Bug 296497 has been marked as a duplicate of this bug. ***

I suspect this may have caused the issue in bug 325940.