52242 - bottom margin dropped on floating IMG elements in tables [FLOAT]

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Mirek Hankus]

Check above URL. There is an image located inside a table. Image has VSPACE
attribute. It is renedered OK when there is no ALIGN attribute. But when you add
ALIGN="left" bottom space of image is ignored (top is ok).

Comment 1

[Loco]

Confirmed in MacOS 9.0.4 2000-09-04-08, Win 98 2000-09-06-08, Linux 6.2 2000-09-
06-08.  Updating status to NEW, and platform to ALL.

Comment 2

[Loco]

Also confirmed working in 4.x for Mac.

Comment 3

[Kevin McCluskey (gone)]

Dividing up Claytons bugs to triage

Comment 4

[Kevin McCluskey (gone)]

Confirmed that is still a problem with 12/13/2000 build on WINNT. The test case
works fine in I.E 5.5.
Reassigning to buster

Comment 5

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 6

[bsharma]

qa contact updated.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Comment 8

[Christopher Hoess (gone)]

Attachment: Uses remote images-replace if necessary

Comment 9

[Christopher Hoess (gone)]

Note that this works fine when *not* in a table-changing component.
Reporter: vspace is deprecated, use CSS to position elements when possible.

Comment 10

[karnaze (gone)]

table cell block problem with floating image. -->waterson.

Comment 11

[Chris Waterson]

Attachment: test case that uses `style="margin: 20px;"' instead of `vspace=20'

vspace is converted to top- and bottom-margin; you can see this problem when
setting these values with CSS properties, as well.

Comment 12

[Chris Waterson]

cc'ing hixie before I make a fool of myself.

Comment 13

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

I think this is because of the somewhat ad hoc way table cells deal with
expanding to hold floats -- they use the overflow area, IIRC, which should
really only be used for painting/event handling/'overflow' handling.  We really
need a better concept of a formatting context that contains floats.

(Do we have the same problem for floats within floats?)

Comment 14

[Chris Waterson]

Attachment: attempt at a float-in-a-float test case

Here's an attempt at a test case. Both these look ``correct'' AFAICT in IE5.5
Mac; however, we bungle them pretty badly, and probably in ways that are not
related to this specific bug. I'll file different bugs if none are on-file
already. Hixie?

Comment 15

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

Oh yeah, expanding floats with 'height: auto' based on floats within them isn't
in the spec yet. :-)

Comment 16

[Chris Waterson]

So I've investigated this a bit more, and I don't think that the problem is
necessarily the use of the overflow area. This seems fairly reasonable to me,
and we've got the frame's area and margins stored correctly in the floater
cache, and transferred correctly to the space manager. So, if we could
incorporate the space manager's state into the information we return to the
table cell, that might solve the problem.

Another way to deal with this would be to include the floater's bottom margin in
the in-flow line's combined area. I don't fully understand the implications of
doing this, but I tend to think that this would be an overly general way to
handle a specific problem -- floats in table cells. Specifically, this might
screw up 10.6.3. (I'm curious: would any of you CSS lawyers consider our
handling of this case `correct' based on a literal interpretation of 10.6.3?
Namely, ``Only children in the normal flow are taken into account''.)

Anyway, still thinking about this. Comments welcome.

Comment 17

[Chris Waterson]

Attachment: stab in the dark

This patch needs thorough testing. It fixes this test case, but probably has
other unexpected side effects.

The basic idea is to include the space manager's YMost() when computing an
auto-height block frame's height. I don't think that we need to do this in the
horizontal direction: it should be done implicitly as we compute the combined
area for the lines.

Comment 18

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

Comment on attachment 64166 [details] [diff] [review]
stab in the dark

r=dbaron.  This is exactly how I've proposed CSS should be changed and should
be pretty much how IE works.

Comment 19

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

(I think this would probably allow us to get *rid* of using the overflow area
for table cells, although there might still be issues with absolute positioning.
 Not using the overflow area would fix some bugs with relative positioning
within table cells, I think.)

Comment 20

[Chris Waterson]

I'm not sure that table cells are using overflow area (at least, I don't see
anything obvious in the table code). In regards to this bug, the place that I
saw overflow area being used was in nsBlockFrame::ReflowFloater: the overflow
area computed during the reflow is returned as the combined area for the float.
Is this what you were thinking of?

Comment 21

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

Hrm.  Combined area.  I need to refresh my memory on what that is, but it sounds
evil.

Comment 22

[Chris Waterson]

This passes the regression tests, AFAICT. Lots of frame-state mismatches, but no
bbox errors or visual artifacts. I'll comment the patch a bit better; marc,
could you sr=?

Comment 23

[Marc Attinasi]

Comment on attachment 64166 [details] [diff] [review]
stab in the dark

sr=attinasi

Comment 24

[Marc Attinasi]

Are you going to check in the testcase as a regression test?  Might be a good
idea...

Comment 25

[Chris Waterson]

Fix checked in, with regression test.

Comment 26

[Amarendra Hanumanula]

 No more regressions. Verified with Build ID: 2002031303.

Comment 27

[Amarendra Hanumanula]

 No more regressions. Verified with Build ID: 2002031303.

Comment 28

[Matthias Versen [:Matti]]

repairing bugzilla problem
"status:Verified, Resolution: "")

Comment 29

[Matthias Versen [:Matti]]

fixed

Comment 30

[Matthias Versen [:Matti]]

verified fixed