Closed Bug 85876 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.3

People

(Reporter: ian, Assigned: dbaron)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: css1, testcase, Whiteboard: [Hixie-P3])

Attachments

(9 files)

complete patch
6.75 KB, patch
Details | Diff | Splinter Review
same as previous, but -b
3.22 KB, patch
Details | Diff | Splinter Review
testcase for max-element-width caching (dynamic)
823 bytes, text/html
Details
testcase for max-element-width caching (static; for comparison)
323 bytes, text/html
Details
improved patch (diff -b), still needs more work
4.15 KB, patch
Details | Diff | Splinter Review
another nice testcase for dynamic sizing of floats
910 bytes, text/html
Details
fixes all but m/b/p issue in most recent testcase (-b)
8.47 KB, patch
Details | Diff | Splinter Review
also fixes m/b/p problem, ready for review (diff -u, for testing)
12.32 KB, patch
Details | Diff | Splinter Review
also fixes m/b/p problem, ready for review (diff -ub, for reading)
9.36 KB, patch
Details | Diff | Splinter Review 
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.
Blocks: float
Keywords: css1, mozilla1.0, testcase
Whiteboard: [Hixie-P3]
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)
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.
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)..
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
*** Bug 88352 has been marked as a duplicate of this bug. ***
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
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...
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 (! ...) { ... }
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).
[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.
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.
FWIW, the patch on this bug fixes this bug, bug 82314, bug 85874, bug 85876, and
part of bug 86746.
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
Closed: 23 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.0 → mozilla0.9.3
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.

Attachment

General

Creator:
Created:
Updated:
Size: