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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hixie (not reading bugmail)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This started between 2001-08-13-08 and 2001-08-13-21 builds (as did bug 95512).

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This was caused by the checkin for bug 93363 (rev 3.450 of nsBlockFrame.cpp)

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?

Comment 4

[Hixie (not reading bugmail)]

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...

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: proposed patch

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Hmm.  That still doesn't fix the odd wrapping in bug 95512.

Comment 9

[Alexandru Savulov]

reassigning to me

Comment 10

[Marc Attinasi]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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).

Comment 12

[Chris Waterson]

Sure, I buy that. It sure seems simpler. Does it break anything?

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 14

[Alexandru Savulov]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?).

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 17

[Alexandru Savulov]

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...

Comment 18

[Alexandru Savulov]

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!

Comment 19

[Alexandru Savulov]

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).

Comment 20

[Alexandru Savulov]

damn... the URL http://www.damowmow.com/portal/ still displays incorrect

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 23

[Alexandru Savulov]

Attachment: reduced test case to use after applying the left padding addition

Comment 24

[Alexandru Savulov]

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

Comment 25

[Alexandru Savulov]

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

Comment 26

[Alexandru Savulov]

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

Comment 27

[Alexandru Savulov]

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)

Comment 28

[Marc Attinasi]

[s]r=attinasi - good catch with that aLine->mMaxElementWidth bit!

I think the branch needs to be updated as well, so nominating topembed.

Comment 29

[Alexandru Savulov]

Attachment: definitive patch

Comment 30

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

r=dbaron if it fixes both this bug and the incremental reflow problem in bug
95512

Comment 31

[Alexandru Savulov]

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

Comment 32

[Alexandru Savulov]

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 

Comment 33

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz on behalf of drivers

Comment 35

[Alexandru Savulov]

fixed on trunk (0.9.4)/branch 0.9.2

Comment 36

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.