Closed Bug 95511 Opened 23 years ago Closed 23 years ago

Shrinkwrapping of floats no longer takes right padding into account [FLOAT]

Categories

(Core :: Layout, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ian, Assigned: alexsavulov)

References

()

Details

(4 keywords, Whiteboard: [Hixie-P2][Hixie-1.0])

Attachments

(3 files)

On http://www.damowmow.com/portal/ there are some floats with padding all the way
around. The text is now overlapping the right padding though. This is a recent
regression. I will create real testcases shortly.
Whiteboard: [Hixie-P2][Hixie-1.0]
This started between 2001-08-13-08 and 2001-08-13-21 builds (as did bug 95512).
This was caused by the checkin for bug 93363 (rev 3.450 of nsBlockFrame.cpp)
This patch seems to fix this bug, but not bug 95512.  It seems like the right
thing to do:

Index: nsBlockFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v
retrieving revision 3.450
diff -u -d -r3.450 nsBlockFrame.cpp
--- nsBlockFrame.cpp    2001/08/14 00:08:08     3.450
+++ nsBlockFrame.cpp    2001/08/15 23:50:29
@@ -4120,11 +4123,12 @@
   // Update xmost
   nscoord xmost;
   if(aLine->IsBlock() && aState.GetFlag(BRS_NOWRAP)) {
-    // since the nowrap blocks tend to be as wide as their widest element or
-    // sequel of elements that can't be wrapped anymore (see patch for bug 80817)
-    // let them set xmost to be the width of the widest element of the reflowed
line
-    // (patches bug 93363 and other NOWRAP dups)
-    xmost = aMaxElementSize.width;
+    // since the nowrap blocks tend to be as wide as their widest
+    // element or sequel of elements that can't be wrapped anymore (see
+    // patch for bug 80817) let them set xmost to be the width of the
+    // widest element of the reflowed line (patches bug 93363 and other
+    // NOWRAP dups)
+    xmost = aLine->mBounds.x + aMaxElementSize.width;
   } else {
     xmost = aLine->mBounds.XMost();
   }

Ian, could you confirm that this bug is related to the left padding/border and
not the right padding/border?
Indeed:
   http://www.damowmow.com/mozilla/bugs/95511/001.html
   http://www.damowmow.com/mozilla/bugs/95511/002.html

The fact that white-space can affect things at this level worries me a little...
Keywords: qawantedpatch, testcase
Actually, now that I looked at bug 93363 in more detail, I think the patch there
is wrong, and we need to find a different way to fix it.
I think I have a better patch for bug 93363, which removes the code explicitly
designed to emulate the nav4 behavior that that bug requested we undo.
Hmm.  That still doesn't fix the odd wrapping in bug 95512.
reassigning to me
Assignee: karnaze → alexsavulov
The odd wrapping 'bug' in 95512 might not actually be a bug at all - the text
should wrap, and the <BR> elements in the markup are making it look bad.
Anyone want to review my second patch?  I think it's the right thing to do.  It
looks like the way the fix to bug 93363 worked was to cancel out the code in the
middle hunk of my patch by setting mKidXMost incorrectly (except it didn't do it
quite right -- see my first patch).
Sure, I buy that. It sure seems simpler. Does it break anything?
Regression test differences:

regression test verify/18445_2.rgd failed (block/bugs)
regression test verify/margins.rgd failed (table/core)
regression test verify/bug1188.rgd failed

  It looks like the button on the combobox is slightly wider with my changes. 
I'm not sure which way is right, but I suspect it used to be wider before bug
93363.  (margins.html actually reported only frame state differences, and not
bbox differences, which seems weirder)

regression test verify/ms.rgd failed

  slight horizontal difference (I can't see it), hopefully an improvement

regression test verify/html40charsbydesc.rgd failed (net/HTML_Chars)

  I'm supposed to find the two tiny horizontal bbox differences in this huge
testcase?  Perhaps noise, or a problem with incremental reflow with all the images?

regression test verify/html40charsbynumber.rgd failed

  Ditto.
David, your patch (attachment 46012 [details] [diff] [review]) is regressing bug 57828.
(is bringing the source to the stand before 58728 was patched by Chris)

We have either to restart from the point where 57828 was unpatched and
everything else worked, or live things as they are and see what's the trouble
with those <div>s in hixie's testcases.

Chris suggested to remove the patch for 57828 as soon as the block code is
repaired and i started to work in that direction. That's why I choosed to remove
Chris patch and uncomment a line commented by Buster in the block code and so
repair 57828 and 80817. That brought me to the point where I had to repair 93363
be cause I also discovered that removing Chris patch repairs 93363. So I decided
to apply the patch for 80817 and repair 93363. I observed the problems with the
<div>s before this bug was posted and wanted to open a bug about that, but I was
occupied by other things and unfortunately (for me :-) Hixie caught me "in
flagranti". 

David, what do you think about all this?
I'm waiting for your oppinion before I do anything.
Hmmm.  It's really not clear to me how nowrap and floats should interact, which
is what bug 57828 seems to be about.  I would think maybe the text should end up
below the float rather than next to it (or do we always stuff at least some text
on every line when we're wrapping around floats?).
I wish we could just ignore bug 57828.  It's such a complicated and messy bug.

Does my first patch fix the incremental reflow problems on LXR?  If so, than
maybe we should just do that for now.
I agree that bug 57828 is an ugly bug :-) and that we handle floaters pretty
poorly. The problem we have is the place that the bug occurs: dailynews.yahoo.com
Now, from my experience with my friends that I'm trying to convert from using
IE, they say, "as long as NS does not display pages correctly like IE" - not my
oppinion - "I'm not going to use it". Is is a damn situation we have here...
in the patch for 93363 i forgot to add the left padding

- xmost = aMaxElementSize.width;
+ xmost = aMaxElementSize.width + aState.BorderPadding().left;
I propose this patch to solve the <div>s problem.

Need reviews, thx!
I mean, it is not a particular <div> problem, it is a general block problem, but
I was thinking about Hixie's testcases (see above).
damn... the URL http://www.damowmow.com/portal/ still displays incorrect
I think the problem with what you proposed is that it doesn't account for left
margin on the child block.  (Is that what http://www.damowmow.com/portal/ has?)
 The first patch I proposed should account for that, though.
No, apparently that's not it.  And maybe the line's bounds do include the margin
of the block.  And the aState should be for the parent block, so I'm not sure
why your patch isn't equivalent to my first patch.
Initialy attachment 46146 [details] is displayed correctly, but if you resize, you'll see
what happens.
Looks like an incremental reflow problem

note: you have to apply the patch:
- xmost = aMaxElementSize.width;
+ xmost = aMaxElementSize.width + aState.BorderPadding().left;

compare with N6.1
looks like we have troubles with lists. I noticed that <ul> and <li> do not
respect padding for example. Just add padding to <ul> and <li> and see how NS6.1
(does not contain my patches) display the <ul> <li> stuff. compare to IE5.x
actualy the patch should be:

- xmost = aMaxElementSize.width;
+ xmost = aLine->mMaxElementWidth + aState.BorderPadding().left;

but still don't know what's the matter with the lists
yeah! the last patch works. 

- xmost = aMaxElementSize.width;
+ xmost = aLine->mMaxElementWidth + aState.BorderPadding().left;

this, be cause aLine->mMaxElementWidth caches the value for the incremental reflow.
(next time I should read the comments too :-)

anyone wants to review? :-) 

(sorry about all this spam)
[s]r=attinasi - good catch with that aLine->mMaxElementWidth bit!

I think the branch needs to be updated as well, so nominating topembed.
Keywords: topembed
Attached patch definitive patchSplinter Review
r=dbaron if it fixes both this bug and the incremental reflow problem in bug
95512
well, bug 95512 is made of two bugs:

- the incremental part 
- the evangelization part

The test case I put there acts correctly but the lxr page still makes troubles
when I type in the inputs.

I would like to have this one here fixed before i go and fix the problem with
the incremental reflow. I don't know yet why does the reduced test case act ok,
but on the LXR page not (well if I resize first then the layou do not change,
while typing)

damn, I'm going to wait with this patch for a while need to do exact tests and
regression testing
I still vote for the last proposed patch (attachment 46225 [details] [diff] [review]) even though the
LXR/seamonkey page performs "ugly" while typing text (the new look is a
consequence of the correct wrap behaviour - subject for evangelization).

David, I think that we're landing right with this patch - unfortunately is
revealing the incremental bug seen in bug 95512. It also happens in NS6.1,
please check my comment and newest test case there.

I'm keeping this patch "on hold" until you check this out.

thanx 
Status: NEW → ASSIGNED
Sure, r=dbaron, but I'd prefer if you add a comment to both places (the two
places I patched in my proposed patch) saying that they cancel each other out
and the only reason they're there is bug 57828.  But I think incremental reflow
problems are actually quite annoying, and we might be better off breaking bug
57828.  But you don't seem to like that idea, so why don't we just go with this
for now.
fixed on trunk (0.9.4)/branch 0.9.2
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment on attachment 46012 [details] [diff] [review]
proposed patch

This patch was incorporated in the patch for bug 97383, which also refixed bug
57828 correctly.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: