errors wrapping width:auto floats with width:auto children [FLOAT]

VERIFIED FIXED in mozilla0.9.3

Status

()

P3
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: ian, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {css1, testcase})

Trunk
mozilla0.9.3
x86
Windows 2000
css1, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P3], URL)

Attachments

(9 attachments)

(Reporter)

Description

18 years ago
STEPS TO REPRODUCE
   1. http://www.hixie.ch/tests/adhoc/css/box/float/010.xml
   2. Resize your window so that it is about one and a half times the width of
      the first float.

ACTUAL RESULTS
   The floats are not all the same size -- those in the second column attempt
   to fit "in the remaining space".

EXPECTED RESULTS
   The floats are identical and thus should all be identical to each other,
   with the same size and everything. The simple case (where the float doesn't
   contain any elements itself but just has text) was fixed recently.

This bug affects a real world webpage:
   http://monopoly.damowmow.com/results.html
Look at the "High Score Tables" section.
(Reporter)

Updated

18 years ago
Blocks: 78094
Keywords: css1, mozilla1.0, testcase
Whiteboard: [Hixie-P3]

Comment 1

18 years ago
You know, I can't find a duplicate for this bug, which is unusual. And I had a
really gitty, snotty, irritating way of pointing out the duplicate when I found
it all ready to use, too.

Anyway... I see this on Mozilla 0.9.1 (2001060712) for Mac OS 9. Also, as
another pointless aside, the window can be resized such that the scollbar
disappears. This happens when the second collumn is just about as small as it's
going to get. Scrolling the document by selecting text still works, but the
scrollbar isn't displayed.

(And the reporter didn't tell us which build of Mozilla s/he saw this on)
(Reporter)

Comment 2

18 years ago
Tested with a Mozilla trunk debug build from 2001-06-14 01:30 on Windows 2000, 
a Windows 2000 Netscape commercial optimised build from the 0.9.1 branch (pre-
release nsbeta1 bits), the Netscape commercial optimised build 2001061404 on
Windows 2000, the Mac nsbeta1 candidate build, and the Linux nsbeta1 candidate
build. As for not finding a duplicate... if this had already been reported, I
would have known about it, given that I have a dinner on the line when it comes
to floater bugs. See bug 78094. :-)

See also bug 85874 and bug 85216 for other bugs visible on the testcase.
(Assignee)

Comment 3

18 years ago
I'm wondering (without having looked all that closely) if some of the fix for
bug 59200 was just wallpaper over some problem in the code at the end of
nsBlockReflowContext::ReflowBlock (for which buster split
nsBlockReflowContext::DoReflowBlock into a separate function)..

Comment 4

18 years ago
The fix for bug 59200 (attachment 34686 [details] [diff] [review]) was largish; which part felt like
wallpaper?
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 5

18 years ago
*** Bug 88352 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

18 years ago
Fix:

Index: nsBlockFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v
retrieving revision 3.445
diff -u -d -r3.445 nsBlockFrame.cpp
--- nsBlockFrame.cpp	2001/07/09 22:44:07	3.445
+++ nsBlockFrame.cpp	2001/07/15 06:13:51
@@ -4793,8 +4793,8 @@
 #endif
 
   // Compute the available width. By default, assume the width of the
-  // available space rect.
-  nscoord availWidth = aState.mAvailSpaceRect.width;
+  // containing block.
+  nscoord availWidth = aState.mContentArea.width;
 
   // If the floater's width is automatic, we can't let the floater's
   // width shrink below its maxElementSize.



Tests 8 and 9 still don't work well at small widths (narrower than a float), but
10 does.
Assignee: waterson → dbaron
Status: ASSIGNED → NEW

Comment 7

18 years ago
dbaron, could you attach the complete patch; i.e., which also removes the
iteration in ReflowAndPlaceFloater()? Seems like we should get this in and kill
all the float bugs hixie filed...
(Assignee)

Comment 8

18 years ago
Created attachment 42421 [details] [diff] [review]
complete patch
(Assignee)

Comment 9

18 years ago
Created attachment 42422 [details] [diff] [review]
same as previous, but -b
I just noticed that since I moved most of the stuff out of the |while| loop I
should change the while (1) { if (...) break; ... } to while (! ...) { ... }
Created attachment 42427 [details]
testcase for max-element-width caching (dynamic)
Created attachment 42428 [details]
testcase for max-element-width caching (static; for comparison)
I'm going to attach a better patch that makes |isAutoWidth| in ReflowFloater get
set to the right thing, but it's still having problems with the cached mew not
being invalidated.  I think the solution is just to stop caching the mew. 
waterson added the caching for bug 59200, when he moved the ReflowFloater call
into the loop that it didn't need to be in.  So I should look and see which of
those changes should be undone (and maybe also removing the reverting of the fix
for bug 38157).
Created attachment 42437 [details] [diff] [review]
improved patch (diff -b), still needs more work
Created attachment 42466 [details]
another nice testcase for dynamic sizing of floats
Created attachment 42470 [details] [diff] [review]
fixes all but m/b/p issue in most recent testcase (-b)
Created attachment 42473 [details] [diff] [review]
also fixes m/b/p problem, ready for review (diff -u, for testing)
Created attachment 42474 [details] [diff] [review]
also fixes m/b/p problem, ready for review (diff -ub, for reading)

Comment 19

18 years ago
[s]r=waterson. (dbaron walked me thru this one-on-one.)
Summary of the changes:

 * remove the caching of the max element size since we no longer call
ReflowFloater in a loop (since the width of 'auto'-width floaters should depend
only on the width of the containing block and the content of the floater, not on
any nearby floaters).  This led to the changes in nsLineBox.{cpp,h} (to
nsFloaterCache), the change to the signature of ReflowFloater, and a few changes
inside ReflowFloater where we used the cached value.  (This reverses some of the
changes for bug 59200.)

 * Use the containing block width as |availWidth| instead of the available space
for inline content (which depends on other floaters)

 * Get the correct style context to determine if a floater is 'auto'-width

 * stop using the aState.mAvailableSpaceRect for the x and y coords of the
availableSpace rect, although the x and y coords really don't matter since the
caller of ReflowFloater moves the floater anyway.

 * cache the max element size in all cases where we compute it (why not?)

 * move a bit of stuff in FlowAndPlaceFloater out of the loop since size of the
floater can't be changed by where it is placed.  (This reverses some of the
changes for bug 59200.)
 * fix the computation of the available width for 'auto'-width floaters -- we
need to subtract the padding, border, and margin from the containing block's
width to find an upper bound on the maximum width.

I'm doing a debug build now to run the block regression tests.

Comment 21

18 years ago
The patch looks fine - thanks for the summary. Pending successful completion of
the block regression tests (and the table regression tests couldn't hurt
either), r=attinasi
I ran the block and table regression tests.

The following test changed and I expected it to (we would break the page it was
a testcase of if they hadn't already changed the page):

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/38157-a.html

The following test for the same bug also changed, but I don't really understand
it.  The new display seems correct and the old one wrong, but I don't understand
the old one:

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/38157-b.html

The following test also changed, somewhat surprisingly, so that there is a
little more space below the image:

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/40129.html
The following test cases claimed bbox mismatches, but I couldn't see any
differences:

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/viewer_tests/test4.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/other/wa_table_thtd_rowspan.html
The second time I looked at the 40129 testcase in the old build, it displayed
the same as it did in the new build (whereas I did see a difference the first
time), so I suspect there's some sort of incremental reflow bug hanging around
that depends on how long it takes the image to load.  (i.e., I don't think the
regression test difference was the result of my changes)  It could even be an
incremental reflow bug I fixed...
38157-b actually had two floats in it -- so the change in behavior was expected.
 (It looks a little disconcerting, but that's because the CSS used to achieve
the effect in the testcase isn't what one would think it would be from looking
at the testcase.)

However, I compared the other two tests more closely, and I found the difference
-- it's exactly as if the stylesheet had the rule:

caption > b { display: none; }

All b elements inside caption elements are disappearing!  I have no idea why
this is happening and it seems totally unrelated to my changes.
None of the children of caption show up with this patch.  I give up.
A "real" regression:  attachment 18503 [details] from bug 58815.
FWIW, the patch on this bug fixes this bug, bug 82314, bug 85874, bug 85876, and
part of bug 86746.

Comment 28

18 years ago
Don't worry about 38157-b.html. This one _always_ shows up as a false positive, 
and is timing dependent. I suspect that it's an incremental reflow problem.
Hmmm.  When I back my changes out of my tree and rebuild, I *still* see the
regressions.  So now I think it's just something that's messed up in my tree and
the changes are fine.
I applied the changes to a different tree (on a different platform, too), and
they work fine without causing the regressions, so checking in...
Fix checked in 2001-07-18 19:33 PDT.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.0 → mozilla0.9.3

Comment 32

17 years ago
Marking verified in the Sept 5th build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.