196870 - scaled images overflow differently than exact-size images

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Frankie]

While constructing a testcase for bug 186942 I discovered a separate quirk in
Mozilla's image layout. My testcase uses images whose actual size matches the
size specified in HTML, and the images are cropped at the edge of their
containing table cells. However, if you use a different image which is scaled to
fit, the rendered image overflows past the cell border.

Press the Submit button at the bottom of the test page to change the image source.

Comment 1

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

The bug is in the exact-size case, where they should overflow.  (Some have
suggested we should default to 'overflow: hidden' on table cells, but we don't,
and that's a different issue.)

Comment 2

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

Hey Francis, can you attach a single-cell testcase where the exact-size image
does not overflow? thanks!

Comment 3

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

Attachment: testcase showing clipping of non-scaled images

Comment 4

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

Attachment: testcase showing overflow (correct) of scaled images

Comment 5

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

I'll take this to get it on the radar. It's probably an easy fix.

Comment 6

[Frankie]

Followup question: Moz Mac displays these examples differently than Moz Win (I
don't have a Linux box). Moz Win calculates cell height based on the image size,
while Moz Mac only uses the div size (thereby cropping things even worse).

I'm planning to post it as a separate bug (unless you know of a prior report; I
couldn't find one). Should it go under Image:Layout or Layout:Tables?

Comment 7

[Frankie]

*** Bug 186942 has been marked as a duplicate of this bug. ***

Comment 8

[Boris Zbarsky [:bzbarsky]]

Just in case anyone cares... the difference in behavior comes from
nsImageFrame::Paint() -- when we draw a scaled image we don't intersect the
image rect with the dirty rect.  So we always draw the entire image as opposed
to just drawing the rect that needs to be drawn.

I tried to fix that as part of my patch for bug 83774, but couldn't get things
to work right (the obvious method of converting the dirty rect to image coords
didn't seem to work; I'm not convinced that this is not just a bug in the GTK
impl of DrawScaledImage(), but didn't really have time to debug it at the time).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and the problem is that the dirty rect is truncated at the table cell edge,
of course....

Comment 10

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

Re comment 9: is there a bug in the table painting code?

Re comment 6: are you testing different versions on Mac and Windows?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, if the intent is that the contents of a cell should be able to paint
outside the cell, then yes, I would say there is a bug.

Most likely, the issue is that SetOverflowClipRect() is called in
nsTableCellFrame::Paint() not only if overflow is hidden but also if
HasPctOverHeight() tests true....

Comment 12

[Frankie]

DBaron, thanks for the sanity check. Moz 1.2.1 (and earlier) use the image size
for cell height; Moz 1.3 (and later) use the div size. 

I'm guessing the latter is the W3C correct behavior? But I have to say the
former makes more sense.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, comment 11 is incorrect (HasPctOverHeight is in fact false in the non-scaled
testcase).  So the clipping is being introduced elsewhere....

Comment 14

[Boris Zbarsky [:bzbarsky]]

Aha.  A more likely candidate is nsTableRowFrame::PaintChildren which intersects
the dirty rect with the child rect and uses that intersection as the dirty rect
for the child paint.  That's what nsContainerFrame::PaintChild (used by blocks)
does too, UNLESS the child has outside children (which is the case here).  Table
rows don't check for outside children (probably because they're treating the
"table cells do not overflow" part of CSS2 pretty literally....)

Comment 15

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

Reassigning to bz on the grounds that he just found the bug :-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Urk.  Me no grok paint code too good, no sirree....

I can try to get to this sometime, but it's not going to be soon... I really
don't know much about our paint code, and even less about how tables work.  Some
info I'd need:

1)  Should table cells overflow like this in general?  That is, if a child of a
    table cell overflows, should that content overflow the cell itself?  How
    does this reconcile with step 1 of the column width calculation at
    http://www.w3.org/TR/CSS21/tables.html#auto-table-layout

    (I can see the argument for not counting overflow of <td> kids as overflow
    of the cell itself.... But we need to decide this once and for all and
    clearly document it in the code (and maybe add an example to the spec?))

2)  What parts of table painting assume this no-overflow business?  bernd?
    Help?

There's also the issue of nsTableCellFrame::Paint doing weird clipping if the
cell starts in a collapsed row or column; not sure what the deal there is or
what expected behavior is, and tempted to roll that over to a separate bug.

Comment 17

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

according to dbaron, cells should be able to overflow. If we want to change that
policy then the right thing to do is to put "td { overflow:hidden }" in some UA
style sheet, not rely on hacks involving the dirty rectangle. Note that if the
image was given a view somehow, the dirty rect passed by
nsTableRowFrame::PaintChildren would be irrelevant and the image would fully
appear. 

nsTableRowFrame::PaintChildren must check the FRAME_OUTSIDE_CHILDREN bit and not
adjust the dirty rect in that case. It's as simple as that, regardless of what
our table cell overflow policy is, and regardless of what other table painting
bugs may exist.

Comment 18

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

taking bug back from bz so he doesn't feel oppressed :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, fair enough.  That makes sense.

In that case, is there a reason not to just wipe out
nsTableRowFrame::PaintChildren and let it delegate to nsContainerFrame, which
does the right thing?  The only differences between the two are this dirty rect
business and the way the debug red border is painted (table row checks
NS_FRAME_PAINT_LAYER_DEBUG whereas nsContainerFrame does not).

Comment 20

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

I agree it should just call nsContainerFrame::PaintChildren.

Comment 21

[Bernd]

before the horkage of tables goes too far :-). Is there somebody out who doesnt
like he clipping??

Comment 22

[Bernd]

http://lxr.mozilla.org/seamonkey/search?string=virtual+void+PaintChildren

returns that tables have tree times overwritten nsContainerFrame::PaintChildren,
I would like to remove all those methods.

The primary idea of the overwriting was to handle cells that rowspan correctly.
This is obviously not necessary for table frames as rowspan will never cross
rowgroup boundaries.

The correct fix would require to report the area that rowspans have outside the
rowframe as overflow area. And to propagate the overflow area correctly (bug 173277)

Tables in auto layout overflow seldom, the testcase shown in the bug is one of
those cases. Tables in fixed layout overflow very frequent and should do that.
 

Comment 23

[Mats Palmgren (inactive)]

URL and testcase works fine now that bug 197311 is fixed.

Comment 24

[Bernd]

I removed the PaintChildren overwrite from the row frame

Comment 25

[Frankie]

v