Last Comment Bug 201897 - [FIX] shrink-wrap absolutely positioned boxes, right of container, have 0 width
: [FIX] shrink-wrap absolutely positioned boxes, right of container, have 0 width
Status: RESOLVED FIXED
[csswg][reflow-refactor]
: testcase
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: Trunk
: All All
: P1 normal with 1 vote (vote)
: ---
Assigned To: Mats Palmgren (:mats)
: Hixie (not reading bugmail)
: Jet Villegas (:jet)
Mentors:
http://strawberrians.org/Map/backup/D...
: 182814 224984 228781 249922 250498 251774 254937 255330 271139 296497 307173 (view as bug list)
Depends on: 271933 310135 271927 271990 272596 272611 275179 279579 317561 325486 325940 329302
Blocks: 129375 145111 157681 170927 178739 189367 201001 254254 260172 261522
  Show dependency treegraph
 
Reported: 2003-04-13 16:55 PDT by Chris A. Giorgi
Modified: 2014-04-26 03:11 PDT (History)
25 users (show)
asa: blocking1.7b-
asa: blocking1.7-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (from bug 224984) (402 bytes, text/html)
2003-11-07 10:24 PST, Mats Palmgren (:mats)
no flags Details
Testcase #2 (1.48 KB, text/html)
2003-11-07 17:53 PST, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (1.72 KB, patch)
2003-11-07 18:05 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Testcase #3 (4.77 KB, text/html)
2004-09-27 17:45 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #4 (1.96 KB, text/html)
2004-09-27 17:46 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 2 (6.61 KB, patch)
2004-09-27 18:03 PDT, Mats Palmgren (:mats)
bzbarsky: review-
Details | Diff | Splinter Review
Screenshot of Testcase #3 (54.76 KB, image/png)
2004-09-27 18:15 PDT, Mats Palmgren (:mats)
no flags Details
Screenshot of Testcase #4 (58.20 KB, image/png)
2004-09-27 18:17 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #5 (2.30 KB, text/html)
2004-10-03 13:56 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 3 (6.52 KB, patch)
2004-10-03 14:07 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 4 (14.60 KB, patch)
2004-10-03 18:00 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 4 (diff -uw) (8.32 KB, patch)
2004-10-03 18:02 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Perf testcase 1 (27.85 KB, text/html)
2004-10-03 18:42 PDT, Mats Palmgren (:mats)
no flags Details
Perf testcase 2 (27.97 KB, text/html)
2004-10-03 18:43 PDT, Mats Palmgren (:mats)
no flags Details
Perf testcase 1 (27.81 KB, text/html)
2004-10-04 13:27 PDT, Mats Palmgren (:mats)
no flags Details
Perf testcase 2 (27.94 KB, text/html)
2004-10-04 13:27 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 5 (works like floats) (15.60 KB, patch)
2004-10-05 17:21 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 5 (diff -uw) (9.33 KB, patch)
2004-10-05 17:21 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Perf testcase 3 (triggers MEW > availWidth) (28.02 KB, text/html)
2004-10-05 17:23 PDT, Mats Palmgren (:mats)
no flags Details
Perf testcase 4 (triggers MEW > availWidth) (28.13 KB, text/html)
2004-10-05 17:25 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 6 (works like floats) (15.60 KB, patch)
2004-10-06 16:05 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review
Patch rev. 6 (diff -uw) (9.33 KB, patch)
2004-10-06 16:06 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 7 (works like floats) (15.78 KB, patch)
2004-11-08 17:35 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 8 (14.93 KB, patch)
2004-11-08 17:44 PST, Mats Palmgren (:mats)
bzbarsky: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch rev. 8 (diff -uw) (8.65 KB, patch)
2004-11-08 17:45 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 9 (works like floats) (6.42 KB, patch)
2004-12-02 17:32 PST, Mats Palmgren (:mats)
bzbarsky: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch rev. 10 (works like floats) (6.74 KB, patch)
2004-12-05 15:06 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Testcase #6, http://www.fynf.at/ (712 bytes, text/html)
2004-12-06 11:10 PST, Mats Palmgren (:mats)
no flags Details
Patch B rev. 1 (correction for rev. 10) (1.65 KB, patch)
2004-12-06 16:54 PST, Mats Palmgren (:mats)
bzbarsky: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Chris A. Giorgi 2003-04-13 16:55:04 PDT
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-8 2003-04-13 17:13:08 PDT
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-8 2003-04-13 17:15:41 PDT
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-8 2003-05-02 10:16:33 PDT
*** Bug 182814 has been marked as a duplicate of this bug. ***
Comment 4 Mats Palmgren (:mats) 2003-06-13 09:59:42 PDT
Reporter, the URL is not accessible.

Maybe a duplicate of bug 170927?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2003-11-07 08:48:42 PST
*** Bug 224984 has been marked as a duplicate of this bug. ***
Comment 6 Mats Palmgren (:mats) 2003-11-07 10:24:15 PST
Created attachment 134999 [details]
Testcase (from bug 224984)
Comment 7 Mats Palmgren (:mats) 2003-11-07 17:53:55 PST
Created attachment 135026 [details]
Testcase #2
Comment 8 Mats Palmgren (:mats) 2003-11-07 18:05:28 PST
Created attachment 135027 [details] [diff] [review]
Patch rev. 1
Comment 9 Mats Palmgren (:mats) 2003-11-07 18:09:10 PST
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 [:bz] (still a bit busy) 2003-11-07 20:33:53 PST
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-8 2003-11-16 20:08:29 PST
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 (:mats) 2003-11-16 20:16:08 PST
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 (:mats) 2003-12-17 12:45:58 PST
*** Bug 228781 has been marked as a duplicate of this bug. ***
Comment 14 David Baron :dbaron: ⌚️UTC-8 2004-01-24 01:15:50 PST
*** Bug 228781 has been marked as a duplicate of this bug. ***
Comment 15 Asa Dotzler [:asa] 2004-04-22 15:06:31 PDT
this is heavy lifting that's not likely to make it into the branch as we work
towards stabilization. 
Comment 16 Markus Hübner 2004-06-14 05:53:50 PDT
Is that something that we can try for 1.8b ?
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2004-07-16 16:17:07 PDT
*** Bug 251774 has been marked as a duplicate of this bug. ***
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2004-08-09 12:19:48 PDT
*** Bug 254937 has been marked as a duplicate of this bug. ***
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2004-08-12 00:37:25 PDT
*** Bug 255330 has been marked as a duplicate of this bug. ***
Comment 20 Mats Palmgren (:mats) 2004-09-27 17:45:16 PDT
Created attachment 160282 [details]
Testcase #3
Comment 21 Mats Palmgren (:mats) 2004-09-27 17:46:06 PDT
Created attachment 160284 [details]
Testcase #4
Comment 22 Mats Palmgren (:mats) 2004-09-27 18:03:19 PDT
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.
Comment 23 Mats Palmgren (:mats) 2004-09-27 18:15:49 PDT
Created attachment 160286 [details]
Screenshot of Testcase #3
Comment 24 Mats Palmgren (:mats) 2004-09-27 18:17:17 PDT
Created attachment 160287 [details]
Screenshot of Testcase #4
Comment 25 Mats Palmgren (:mats) 2004-09-27 18:26:29 PDT
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 [:bz] (still a bit busy) 2004-09-28 22:32:24 PDT
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 (:mats) 2004-10-03 13:56:26 PDT
Created attachment 160950 [details]
Testcase #5
Comment 28 Mats Palmgren (:mats) 2004-10-03 14:07:14 PDT
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...
Comment 29 Mats Palmgren (:mats) 2004-10-03 17:05:52 PDT
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 [:bz] (still a bit busy) 2004-10-03 17:17:52 PDT
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 (:mats) 2004-10-03 18:00:57 PDT
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)
Comment 32 Mats Palmgren (:mats) 2004-10-03 18:02:30 PDT
Created attachment 160959 [details] [diff] [review]
Patch rev. 4 (diff -uw)
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2004-10-03 18:16:07 PDT
> 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 (:mats) 2004-10-03 18:42:50 PDT
Created attachment 160962 [details]
Perf testcase 1
Comment 35 Mats Palmgren (:mats) 2004-10-03 18:43:54 PDT
Created attachment 160964 [details]
Perf testcase 2
Comment 36 Mats Palmgren (:mats) 2004-10-03 18:50:15 PDT
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 [:bz] (still a bit busy) 2004-10-03 18:51:04 PDT
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-8 2004-10-03 19:07:17 PDT
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 (:mats) 2004-10-04 13:27:13 PDT
Created attachment 161069 [details]
Perf testcase 1
Comment 40 Mats Palmgren (:mats) 2004-10-04 13:27:54 PDT
Created attachment 161070 [details]
Perf testcase 2
Comment 41 Mats Palmgren (:mats) 2004-10-04 13:29:11 PDT
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 [:bz] (still a bit busy) 2004-10-04 14:19:50 PDT
OK.  8% isn't so bad...
Comment 43 Mats Palmgren (:mats) 2004-10-05 17:21:03 PDT
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.
Comment 44 Mats Palmgren (:mats) 2004-10-05 17:21:57 PDT
Created attachment 161216 [details] [diff] [review]
Patch rev. 5 (diff -uw)
Comment 45 Mats Palmgren (:mats) 2004-10-05 17:23:44 PDT
Created attachment 161217 [details]
Perf testcase 3 (triggers MEW > availWidth)
Comment 46 Mats Palmgren (:mats) 2004-10-05 17:25:14 PDT
Created attachment 161218 [details]
Perf testcase 4 (triggers MEW > availWidth)
Comment 47 Mats Palmgren (:mats) 2004-10-05 17:41:09 PDT
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 [:bz] (still a bit busy) 2004-10-05 17:48:09 PDT
> 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 (:mats) 2004-10-05 18:13:53 PDT
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 (:mats) 2004-10-06 16:05:11 PDT
Created attachment 161311 [details] [diff] [review]
Patch rev. 6 (works like floats)
Comment 51 Mats Palmgren (:mats) 2004-10-06 16:06:35 PDT
Created attachment 161312 [details] [diff] [review]
Patch rev. 6 (diff -uw)
Comment 52 Mats Palmgren (:mats) 2004-10-06 16:11:02 PDT
Your thoughts regarding the difference between the green table/div in
attachment 160284 [details] would be appreciated also.
Comment 53 David Baron :dbaron: ⌚️UTC-8 2004-10-06 16:16:27 PDT
Seems like the big loop you added should be a do { ... } while (0); loop.  (More
comments later.)
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2004-10-19 12:50:55 PDT
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-8 2004-10-19 15:46:03 PDT
Even if it did, 'min-width' beats 'max-width' which beats 'width'.
Comment 56 Mats Palmgren (:mats) 2004-11-05 18:11:11 PST
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 (:mats) 2004-11-05 18:15:33 PST
(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 (:mats) 2004-11-05 18:44:12 PST
(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 (:mats) 2004-11-08 17:35:02 PST
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.
Comment 60 Mats Palmgren (:mats) 2004-11-08 17:44:20 PST
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%).
Comment 61 Mats Palmgren (:mats) 2004-11-08 17:45:24 PST
Created attachment 165229 [details] [diff] [review]
Patch rev. 8 (diff -uw)
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2004-11-08 21:08:06 PST
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 2004-11-22 04:23:27 PST
*** Bug 271139 has been marked as a duplicate of this bug. ***
Comment 64 David Baron :dbaron: ⌚️UTC-8 2004-11-22 12:11:16 PST
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 (:mats) 2004-11-26 14:40:00 PST
(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 (:mats) 2004-11-26 16:30:02 PST
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 (:mats) 2004-11-26 16:52:30 PST
*** Bug 250498 has been marked as a duplicate of this bug. ***
Comment 68 Mats Palmgren (:mats) 2004-11-26 16:55:15 PST
*** Bug 249922 has been marked as a duplicate of this bug. ***
Comment 69 David Baron :dbaron: ⌚️UTC-8 2004-11-26 22:29:33 PST
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 2004-11-27 01:39:41 PST
Leak stats also jumped a bit on brad.
Comment 71 David Baron :dbaron: ⌚️UTC-8 2004-11-27 09:42:34 PST
(In reply to comment #70)
> Leak stats also jumped a bit on brad.

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------
BandRect                                        576          -
Comment 72 Mats Palmgren (:mats) 2004-11-27 10:28:31 PST
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-8 2004-11-27 10:30:07 PST
No.
Comment 74 Boris Zbarsky [:bz] (still a bit busy) 2004-11-27 21:32:20 PST
> 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] 2004-11-28 05:35:45 PST
This caused regression bug 272099 ?
Comment 76 José Jeria 2004-11-28 05:38:00 PST
Yes, see comment 69 and comment 72
Comment 77 Martijn Wargers [:mwargers] (not working for Mozilla) 2004-12-01 07:38:06 PST
Could this also have caused bug 272596?
Comment 78 David Baron :dbaron: ⌚️UTC-8 2004-12-01 12:20:59 PST
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 (:mats) 2004-12-02 17:29:16 PST
(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 (:mats) 2004-12-02 17:32:49 PST
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.
Comment 81 Boris Zbarsky [:bz] (still a bit busy) 2004-12-02 19:36:19 PST
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-8 2004-12-02 23:34:22 PST
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-8 2004-12-04 02:01:57 PST
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 (:mats) 2004-12-05 15:06:57 PST
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.
Comment 85 Mats Palmgren (:mats) 2004-12-05 15:20:47 PST
(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 (:mats) 2004-12-05 17:41:57 PST
Checked in "Patch rev. 10" to fix the regressions, 2004-12-05 16:27 PDT.
Comment 87 Robert Kaiser 2004-12-06 07:46:10 PST
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 2004-12-06 07:55:48 PST
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 2004-12-06 09:17:48 PST
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 [:bz] (still a bit busy) 2004-12-06 09:29:11 PST
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 (:mats) 2004-12-06 11:10:47 PST
Created attachment 168042 [details]
Testcase #6, http://www.fynf.at/
Comment 92 Mats Palmgren (:mats) 2004-12-06 16:54:20 PST
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.
Comment 93 Boris Zbarsky [:bz] (still a bit busy) 2004-12-06 17:00:58 PST
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 (:mats) 2004-12-06 17:11:04 PST
(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 [:bz] (still a bit busy) 2004-12-06 21:07:15 PST
If it _did_ show up, we'd want a check, not an assert... ;)
Comment 96 Mats Palmgren (:mats) 2004-12-08 19:24:27 PST
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-8 2004-12-11 19:40:33 PST
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 (:mats) 2004-12-12 08:44:56 PST
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 (:mats) 2004-12-18 09:09:07 PST
I have filed the remaining case separately, bug 275179.

-> FIXED
Comment 100 Boris Zbarsky [:bz] (still a bit busy) 2005-08-31 16:19:29 PDT
*** Bug 296497 has been marked as a duplicate of this bug. ***
Comment 101 Bill Gianopoulos [:WG9s] 2006-02-05 05:32:18 PST
I suspect this may have caused the issue in bug 325940.
Comment 102 Gérard Talbot 2007-04-21 23:16:42 PDT
*** Bug 307173 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.