201897 - [FIX] shrink-wrap absolutely positioned boxes, right of container, have 0 width

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P1)

Description

[Chris A. Giorgi]

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

Comment 1

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

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.

Comment 2

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

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.

Comment 3

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

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

Comment 4

[Mats Palmgren (inactive)]

Reporter, the URL is not accessible.

Maybe a duplicate of bug 170927?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase (from bug 224984)

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 9

[Mats Palmgren (inactive)]

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 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

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

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 12

[Mats Palmgren (inactive)]

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.

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

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

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

Comment 15

[Asa Dotzler [:asa]]

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

Comment 16

[Markus Hübner]

Is that something that we can try for 1.8b ?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 21

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 22

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

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

Comment 23

[Mats Palmgren (inactive)]

Attachment: Screenshot of Testcase #3

Comment 24

[Mats Palmgren (inactive)]

Attachment: Screenshot of Testcase #4

Comment 25

[Mats Palmgren (inactive)]

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 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Mats Palmgren (inactive)]

Attachment: Testcase #5

Comment 28

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

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

Comment 29

[Mats Palmgren (inactive)]

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

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

Comment 31

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

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)

Comment 32

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4 (diff -uw)

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Mats Palmgren (inactive)]

Attachment: Perf testcase 1

Comment 35

[Mats Palmgren (inactive)]

Attachment: Perf testcase 2

Comment 36

[Mats Palmgren (inactive)]

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

Comment 37

[Boris Zbarsky [:bzbarsky]]

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.

Comment 38

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

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

Comment 39

[Mats Palmgren (inactive)]

Attachment: Perf testcase 1

Comment 40

[Mats Palmgren (inactive)]

Attachment: Perf testcase 2

Comment 41

[Mats Palmgren (inactive)]

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

Comment 42

[Boris Zbarsky [:bzbarsky]]

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

Comment 43

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5 (works like floats)

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.

Comment 44

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5 (diff -uw)

Comment 45

[Mats Palmgren (inactive)]

Attachment: Perf testcase 3 (triggers MEW > availWidth)

Comment 46

[Mats Palmgren (inactive)]

Attachment: Perf testcase 4 (triggers MEW > availWidth)

Comment 47

[Mats Palmgren (inactive)]

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

Comment 48

[Boris Zbarsky [:bzbarsky]]

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

[Mats Palmgren (inactive)]

Comment on attachment 161216 [details] [diff] [review]
Patch rev. 5 (diff -uw)

+      if (maxWidth <= 0) {
+	 maxWidth = 0;

That should be '<' of course...

Comment 50

[Mats Palmgren (inactive)]

Attachment: Patch rev. 6 (works like floats)

Comment 51

[Mats Palmgren (inactive)]

Attachment: Patch rev. 6 (diff -uw)

Comment 52

[Mats Palmgren (inactive)]

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

Comment 53

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

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

Comment 54

[Boris Zbarsky [:bzbarsky]]

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.

Comment 55

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

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

Comment 56

[Mats Palmgren (inactive)]

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

Comment 57

[Mats Palmgren (inactive)]

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

Comment 58

[Mats Palmgren (inactive)]

(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!

Comment 59

[Mats Palmgren (inactive)]

Attachment: Patch rev. 7 (works like floats)

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.

Comment 60

[Mats Palmgren (inactive)]

Attachment: Patch rev. 8

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 61

[Mats Palmgren (inactive)]

Attachment: Patch rev. 8 (diff -uw)

Comment 62

[Boris Zbarsky [:bzbarsky]]

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?

Comment 63

[Erik Fabert]

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

Comment 64

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

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.

Comment 65

[Mats Palmgren (inactive)]

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

Comment 66

[Mats Palmgren (inactive)]

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

Comment 67

[Mats Palmgren (inactive)]

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

Comment 68

[Mats Palmgren (inactive)]

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

Comment 69

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

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.

Comment 70

[Giacomo Magnini]

Leak stats also jumped a bit on brad.

Comment 71

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

(In reply to comment #70)
> Leak stats also jumped a bit on brad.

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------
BandRect                                        576          -

Comment 72

[Mats Palmgren (inactive)]

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?

Comment 73

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

No.

Comment 74

[Boris Zbarsky [:bzbarsky]]

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

Comment 75

[Peter van der Woude [:Peter6]]

This caused regression bug 272099 ?

Comment 76

[José Jeria]

Yes, see comment 69 and comment 72

Comment 77

[Martijn Wargers (dead)]

Could this also have caused bug 272596?

Comment 78

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

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.

Comment 79

[Mats Palmgren (inactive)]

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

Comment 80

[Mats Palmgren (inactive)]

Attachment: Patch rev. 9 (works like floats)

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

Comment 81

[Boris Zbarsky [:bzbarsky]]

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.

Comment 82

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

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 83

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

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.

Comment 84

[Mats Palmgren (inactive)]

Attachment: Patch rev. 10 (works like floats)

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.

Comment 85

[Mats Palmgren (inactive)]

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

Comment 86

[Mats Palmgren (inactive)]

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

Comment 87

[Robert Kaiser]

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

Comment 88

[Robert Kaiser]

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.

Comment 89

[Robert Kaiser]

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.

Comment 90

[Boris Zbarsky [:bzbarsky]]

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

Comment 91

[Mats Palmgren (inactive)]

Attachment: Testcase #6, http://www.fynf.at/

Comment 92

[Mats Palmgren (inactive)]

Attachment: Patch B rev. 1 (correction for rev. 10)

(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 93

[Boris Zbarsky [:bzbarsky]]

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

Comment 94

[Mats Palmgren (inactive)]

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

Comment 95

[Boris Zbarsky [:bzbarsky]]

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

Comment 96

[Mats Palmgren (inactive)]

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 97

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

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 98

[Mats Palmgren (inactive)]

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

Comment 99

[Mats Palmgren (inactive)]

I have filed the remaining case separately, bug 275179.

-> FIXED

Comment 100

[Boris Zbarsky [:bzbarsky]]

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

Comment 101

[Bill Gianopoulos [:WG9s]]

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