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

RESOLVED FIXED

Status

()

Core
Layout: R & A Pos
P1
normal
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: Chris A. Giorgi, Assigned: Mats Palmgren (vacation - back in August))

Tracking

(Depends on: 2 bugs, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7b -
blocking1.7 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [csswg][reflow-refactor], URL)

Attachments

(16 attachments, 13 obsolete attachments)

402 bytes, text/html
Details
1.48 KB, text/html
Details
4.77 KB, text/html
Details
1.96 KB, text/html
Details
54.76 KB, image/png
Details
58.20 KB, image/png
Details
2.30 KB, text/html
Details
27.81 KB, text/html
Details
27.94 KB, text/html
Details
28.02 KB, text/html
Details
28.13 KB, text/html
Details
14.93 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
8.65 KB, patch
Details | Diff | Splinter Review
6.74 KB, patch
Details | Diff | Splinter Review
712 bytes, text/html
Details
1.65 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
Assignee: other → position
Status: UNCONFIRMED → NEW
Component: Layout → Layout: R & A Pos
Ever confirmed: true
Whiteboard: [csswg]
Summary: Render error for absolute positioned block elements beyond initial viewport when scrolled. → shrink-wrap absolutely positioned boxes, right of container, have 0 width
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.
Blocks: 157681

Updated

15 years ago
OS: Linux → All
Hardware: PC → All
*** Bug 182814 has been marked as a duplicate of this bug. ***
Reporter, the URL is not accessible.

Maybe a duplicate of bug 170927?

Comment 5

14 years ago
*** Bug 224984 has been marked as a duplicate of this bug. ***
Created attachment 134999 [details]
Testcase (from bug 224984)
Created attachment 135026 [details]
Testcase #2
Created attachment 135027 [details] [diff] [review]
Patch rev. 1
The following code looks a bit odd to me:

            if (maxWidth <= 0) {
              maxWidth = 1;
            }

I think it should be:

            if (maxWidth < 0) {
              maxWidth = 0;
            }
Attachment #135027 - Flags: superreview?(dbaron)
Attachment #135027 - Flags: review?(bz-vacation)
Keywords: testcase
Summary: shrink-wrap absolutely positioned boxes, right of container, have 0 width → [FIX] shrink-wrap absolutely positioned boxes, right of container, have 0 width

Comment 10

14 years ago
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.
Attachment #135027 - Attachment is obsolete: true
Attachment #135027 - Flags: superreview?(dbaron)
Attachment #135027 - Flags: review?(bz-vacation)
Whiteboard: [csswg] → [csswg][reflow-refactor]
*** Bug 228781 has been marked as a duplicate of this bug. ***
*** Bug 228781 has been marked as a duplicate of this bug. ***
Priority: -- → P1

Updated

14 years ago
Flags: blocking1.7b?

Updated

14 years ago
Flags: blocking1.7b?
Flags: blocking1.7b-
Flags: blocking1.7?

Comment 15

13 years ago
this is heavy lifting that's not likely to make it into the branch as we work
towards stabilization. 
Flags: blocking1.7? → blocking1.7-

Comment 16

13 years ago
Is that something that we can try for 1.8b ?
Blocks: 249922

Updated

13 years ago
Blocks: 250498

Updated

13 years ago
Blocks: 170927

Comment 17

13 years ago
*** Bug 251774 has been marked as a duplicate of this bug. ***
Blocks: 189367
Blocks: 254254

Comment 18

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

Comment 19

13 years ago
*** Bug 255330 has been marked as a duplicate of this bug. ***
Created attachment 160282 [details]
Testcase #3
Created attachment 160284 [details]
Testcase #4
Assignee: core.layout.r-and-a-pos → mats.palmgren
Created attachment 160285 [details] [diff] [review]
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.
Attachment #160285 - Flags: superreview?(dbaron)
Attachment #160285 - Flags: review?(bzbarsky)
Created attachment 160286 [details]
Screenshot of Testcase #3
Created attachment 160287 [details]
Screenshot of Testcase #4
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

13 years ago
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.
Attachment #160285 - Flags: review?(bzbarsky) → review-

Updated

13 years ago
Blocks: 261522
Created attachment 160950 [details]
Testcase #5
Created attachment 160952 [details] [diff] [review]
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...
Attachment #160285 - Attachment is obsolete: true
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

13 years ago
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... ;)
Created attachment 160958 [details] [diff] [review]
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)
Attachment #160952 - Attachment is obsolete: true
Created attachment 160959 [details] [diff] [review]
Patch rev. 4 (diff -uw)

Comment 33

13 years ago
> 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?
Created attachment 160962 [details]
Perf testcase 1
Created attachment 160964 [details]
Perf testcase 2
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

13 years ago
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).)
Created attachment 161069 [details]
Perf testcase 1
Attachment #160962 - Attachment is obsolete: true
Created attachment 161070 [details]
Perf testcase 2
Attachment #160964 - Attachment is obsolete: true
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

13 years ago
OK.  8% isn't so bad...
Attachment #160285 - Flags: superreview?(dbaron)
Created attachment 161215 [details] [diff] [review]
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.
Created attachment 161216 [details] [diff] [review]
Patch rev. 5 (diff -uw)
Created attachment 161217 [details]
Perf testcase 3 (triggers MEW > availWidth)
Created attachment 161218 [details]
Perf testcase 4 (triggers MEW > availWidth)
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

13 years ago
> 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...
Created attachment 161311 [details] [diff] [review]
Patch rev. 6 (works like floats)
Attachment #161215 - Attachment is obsolete: true
Created attachment 161312 [details] [diff] [review]
Patch rev. 6 (diff -uw)
Attachment #161216 - Attachment is obsolete: true
Attachment #161311 - Flags: superreview?(dbaron)
Attachment #161311 - Flags: review?(bzbarsky)
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 54

13 years ago
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.
Attachment #161311 - Flags: review?(bzbarsky) → review+
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).
Attachment #161311 - Attachment is obsolete: true
Attachment #161311 - Flags: superreview?(dbaron)
Attachment #161312 - Attachment is obsolete: true
(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!
Created attachment 165224 [details] [diff] [review]
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.
Created attachment 165228 [details] [diff] [review]
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%).
Attachment #160958 - Attachment is obsolete: true
Attachment #160959 - Attachment is obsolete: true
Attachment #165224 - Attachment is obsolete: true
Created attachment 165229 [details] [diff] [review]
Patch rev. 8 (diff -uw)
Attachment #165228 - Flags: superreview?(dbaron)
Attachment #165228 - Flags: review?(bzbarsky)

Comment 62

13 years ago
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?
Attachment #165228 - Flags: review?(bzbarsky) → review+

Comment 63

13 years ago
*** Bug 271139 has been marked as a duplicate of this bug. ***
Attachment #165228 - Flags: superreview?(dbaron) → superreview+
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.
Blocks: 271924
Blocks: 271927
Blocks: 271933
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
No longer blocks: 250498
*** Bug 250498 has been marked as a duplicate of this bug. ***
No longer blocks: 249922
*** Bug 249922 has been marked as a duplicate of this bug. ***
Blocks: 178739
Blocks: 201001
Blocks: 129375
Blocks: 260172
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

13 years ago
Leak stats also jumped a bit on brad.
Blocks: 271990
(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.

Comment 74

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

Comment 76

13 years ago
Yes, see comment 69 and comment 72
Could this also have caused bug 272596?
No longer blocks: 271924, 271927, 271933, 271990
Depends on: 271924, 271927, 271933, 271990, 272611
Depends on: 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 167700 [details] [diff] [review]
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.
Attachment #167700 - Flags: superreview?(dbaron)
Attachment #167700 - Flags: review?(bzbarsky)

Comment 81

13 years ago
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.
Attachment #167700 - Flags: review?(bzbarsky) → review+
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.
Attachment #167700 - Flags: superreview?(dbaron) → superreview+
Created attachment 167958 [details] [diff] [review]
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.
Attachment #167700 - Attachment is obsolete: true
(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.

Comment 87

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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).
Created attachment 168042 [details]
Testcase #6, http://www.fynf.at/
Created attachment 168077 [details] [diff] [review]
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.
Attachment #168077 - Flags: superreview?(dbaron)
Attachment #168077 - Flags: review?(bzbarsky)

Comment 93

13 years ago
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...
Attachment #168077 - Flags: review?(bzbarsky) → review+
(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

13 years ago
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
Attachment #168077 - Flags: superreview?(dbaron) → superreview+
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
Depends on: 275179
I have filed the remaining case separately, bug 275179.

-> FIXED
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
No longer depends on: 271924
Resolution: --- → FIXED

Updated

13 years ago
Depends on: 279579

Comment 100

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

Updated

12 years ago
Depends on: 317561
I suspect this may have caused the issue in bug 325940.
Depends on: 325940

Updated

12 years ago
Depends on: 329302

Updated

12 years ago
Depends on: 310135
Depends on: 325486
Blocks: 145111

Updated

10 years ago
Duplicate of this bug: 307173
You need to log in before you can comment on or make changes to this bug.