176423 - image inside table get truncated

Status: RESOLVED WORKSFORME

(Core :: Layout: Block and Inline, defect, P3)

Description

[Jerry]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020910
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020910

access the testcase, image inside table get truncated.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Jerry]

Attachment: testcase

Comment 2

[Jerry]

I compare IE , opera with mozilla, they are all different.

mozilla, will set div's width as 50px, but image get truncated， for the
table cell is set to about 50px,
IE, will show image fully, and set div's width bigger than 50px.
opera, will set div's width as 50px, but show image fully.

I dont know which one is better.
it seems more like one question about overflow inside table.

I check the spec, I think opera may do the right thing.

Comment 3

[Bernd]

The table cell should not wrap the image. It sees as a child the div with 50px
width and due to the 4% should shrink to the width of the div as it is also the
MES. IE is definetely wrong. In my opinion the image should overwrite the
neighbour cell.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I commented on a bug with identical content yesterday.  Please find and dup.

Comment 5

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

I agree that Opera's behavior is correct.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Never mind me.  I was responding to an _email_ with identical content, not
commenting on a bug.  To summarize, since table cells do not allow overflow we
should be sizing to include the overflow area of the kid, not just its MES.  In
other words, I too think Opera is doing the right thing.

Comment 7

[Jerry]

assign it to me

Comment 8

[Jerry]

Attachment: patch 0.1

Error of this bug is that table cell has percent width, which is bigger than
its child <div>' mes, but smaller than cell's desired width.

patch 0.1 add some code to BasicTableLayoutStrategy::AssignPctColumnWidths(),
if in this case, use desired width instead of percent width.

Comment 9

[Jerry]

Attachment: patch 0.2

For a image without css width or height, it will cause an increamental reflow
,so make patch 0.2 to handle this.

Comment 10

[Jerry]

Comment on attachment 104834 [details] [diff] [review]
patch 0.2

patch 0.2 doesnot pass regression test, so mark it as obsolete

Comment 11

[karnaze (gone)]

The problem is with the block code. During the incremental reflow when the image
gets its size and reports a me=9000, the cell block reports a me=780 which is
too small.

 block 04D16F24 r=1 a=13545,UC c=13305,UC cnt=1576
  tblO 04D171DC r=1 a=13305,UC c=0,0 cnt=1577
   tbl 04D17324 r=1 a=13305,UC c=11745,UC cnt=1578
    rowG 04D1750C r=1 a=11745,UC c=11745,UC cnt=1579
     row 04D17698 r=1 a=11745,UC c=11745,UC cnt=1580
      cell 04D17864 r=1 a=840,UC c=780,UC cnt=1581
       block 04D178C4 r=1 a=780,UC c=780,UC cnt=1582
        block 04D17BD8 r=1 a=780,UC c=750,UC cnt=1583
         img 04D222F4 r=4 a=UC,UC c=UC,UC cnt=1585
         img 04D222F4 d=9000,870 me=9000
         img 04D222F4 r=2 a=750,UC c=UC,UC cnt=1588
         img 04D222F4 d=9000,870
        block 04D17BD8 d=780,968 me=780 m=9030
       block 04D178C4 d=9015,968 me=780 m=9030
      cell 04D17864 d=9075,1035 me=840 m=9090
     row 04D17698 d=11745,1035
    rowG 04D1750C d=11745,1035
   tbl 04D17324 d=11775,1125
  tblO 04D171DC d=11775,1125
 block 04D16F24 d=13305,1125

Comment 12

[Boris Zbarsky [:bzbarsky]]

Um..  The div has a fixed width set.  So it's MES is actually smaller than the
MES of the image.

We keep running into this problem.  In cases when a box is constrained and
overflows, should the MES include the overflowing content?

Comment 13

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

No.  (There's no backwards compatibility argument here, since other browsers
don't support 'width' correctly to begin with.)  Nor should tables clip their
overflow.

What behavior are these patches intended to enforce?  Comment 5 and comment 6
seemed to be the beginning of a consensus as to the correct behavior, but the
patches seem to be moving away from that consensus rather than towards it.  If
you disagree, please discuss, rather than just attaching patches.

Comment 14

[Bernd]

David: I don't see a consensus. I disagree with bz's comment: To summarize,
since table cells do not allow overflow we should be sizing to include the
overflow area of the kid, not just its MES. 

This would IMHO exactly mean that table cells should implement NS_BLOCKWRAPSIZE.
I thought we agreed in  bug 172896 that this flag is evil.

Comment 15

[karnaze (gone)]

If the "no" in comment #14 is saying that mes shouldn't contain the overflow,
then we could add another data member to the reflow metrics dealing with mes
overflow as a companion to mOverflowArea. Just using mes would be a lot simpler,
but maybe that would cause problems for blocks (does it?). 

Comment 16

[karnaze (gone)]

I meant to say comment #13.

Comment 17

[karnaze (gone)]

Attachment: patch to include overflow in mes

The only regression test that differs is block/bugs/6674.html, but the patch
makes us more compatible with IE and Opera on that test.

Comment 18

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

Oh, I misunderstood Jerry's description in comment 2.  I think Opera's behavior
is incorrect, but it may be the closest to a correct implementation of 'width'
we'll be able to do on the web today.  If we'd done it correctly 4 years ago I
suspect that wouldn't be the case, though.

Beware the changes I'm making in bug 172896.   |mOverflowArea| will soon have no
effect on layout, and that's needed to fix a whole group of bugs (see the
dependency tree of bug 172896 and also the dependencies and comments in bug
145141).  Does this depend on mOverflowArea affecting layout?  If it does, you
need to add something new.

Comment 19

[karnaze (gone)]

The patch doesn't use mOverflowArea directly, so I don't think there will be a
problem. 

Does anybody on the CC list want to sr/r the patch?

Comment 20

[Jerry]

yep, the patch is good.
change component to layout/block-inline, since it is not a table bug.

Comment 21

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

Comment on attachment 105220 [details] [diff] [review]
patch to include overflow in mes

So, it makes sense to me that max width should never be smaller than max
element size.  However, it seems like this still allows that to happen if
overflow is hidden.  Or do we trim the maxelementsize for the hidden overflow
case somewhere else?

Comment 22

[karnaze (gone)]

Good point. Maybe I should set max width appropriately and see what happens.

Comment 23

[kinmoz]

Comment on attachment 105220 [details] [diff] [review]
patch to include overflow in mes

Should we be taking into account alignment? I ask because this

  borderPadding.left + aState.mMaxElementSize.width + borderPadding.right

seems like it would make maxWidth too big in some cases. You can see what I'm
talking about by increasing the right border or right padding for the div in
the test case.

Comment 24

[karnaze (gone)]

To address kin's comments, does the following make sense?

Define the alignment of the block's boxes contributing to the mes width as
either left, right, or center. A left aligment may not need to include the right
border width in the mes width. A right alignment may not need to include the
left border width. A center aligment may not need to include either border.
 
If there are no floaters (and only one box) then the alignment is the text-align
of the block if the box is an inline, otherwise it is left.

If the boxes contain a left floater and a right floater, then the alignment is
center.

If the boxes contain left floater(s), no right floater, and the non floating box
is a block, then the alignment is left.

If the boxes contain left floater(s), no right floater, and the non floating box
is an inline, then the alignment is (1) left if the block's text-align is left
or center, (2) center if the block's text-align is right.

If the boxes contain right floater(s), no left floater, and the non floating box
is a block, then the alignment is right.

If the boxes contain right floater(s), no left floater, and the non floating box
is an inline, then the alignment is (1) right if the block's text-align is right
or center, (2) center if the block's text-align is left.

Of course, this assumes a direction of ltr.

Comment 25

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

What does the alignment have to do with whether the border should be included?

Comment 26

[karnaze (gone)]

If the div has a left borderpadding of 50px, a right borderpadding of 100px, and
a width of 0px, and aState.mMaxElementSize.width = 500px, then with the patch,
maxWidth will be 650px, but should only be 550px because the overflow went
beyond the right border and the right border width shouldn't have been counted.

But since the block lays out starting at its left content edge (for direction
ltr), my last comment is wrong, and I think we can always just ignore the right
border width in the calculation. I'll try it later, but I think the following
handwritten patch will take care of this.


+  maxWidth = (NS_STYLE_OVERFLOW_HIDDEN == aReflowState.mStyleDisplay->mOverflow)
+             ? aMetrics.width
+             : PR_MAX(borderPadding.left + aState.mMaxElementSize.width, 
+                      aMetrics.width);

Comment 27

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

How do we solve this problem for the mes calculation itself?  (It looks like mes
calculation does account for descendants even when those descendants can't
actually influence the size of the parent.  This isn't the way I would think it
ought to work (since a 'width' is a width, period).  However, it seems to be the
way legacy table layout works -- i.e., not based on a true minimum intrinsic
width concept.)  Or does the mes calculation do this correctly?  (If not, should
we just imitate here what it does do?)

Comment 28

[karnaze (gone)]

I agree with your comments, except I'm not sure what you are asking in the last
two questions. Since it looks like aState.mMaxElementSize.width was computed by
ignoring the style width on the block (and thus considering descendants which
can't influence the size of the block), it makes it possible to accomodate
tables in this case.

Comment 29

[karnaze (gone)]

Attachment: revised patch addressing comments

Comment 30

[Alexandru Savulov]

Comment on attachment 107123 [details] [diff] [review]
revised patch addressing comments

try to think of cases when the max element size is used outside the tables and
how this patch affects them. right now i don't see what could regress. so based
on the talk at the office r=

Comment 31

[Kathleen :Brade]

Comment on attachment 105220 [details] [diff] [review]
patch to include overflow in mes

obsolete patch; adding -

Comment 32

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

Comment on attachment 107123 [details] [diff] [review]
revised patch addressing comments

sr=dbaron, although kin may want to have a look at this as well.

Comment 33

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

Comment on attachment 107123 [details] [diff] [review]
revised patch addressing comments

Actually, now I'm not so sure.	I'm wondering whether this weird behavior we're
trying to do with allowing the size of descendants of tables to affect the
table size through other mechanisms than affecting size straight up the
ancestor chain is just a halfway-copying of MSIE's buggy implementation of the
'width' property.  Given the amount that 'width' is used on the web today, I
wonder whether it would be better to just avoid this in *both*
max-element-width and unconstrained-width computation.

KHTML does this, as demonstrated by this testcase:
http://www.people.fas.harvard.edu/~dbaron/css/test/2002/overflow-in-table
I'm curious what MacIE does.  If MacIE does the same thing (making the aqua
table wrap around the green div but not the purple div), then perhaps we should
move towards eliminating this strange behavior?  Also, what does WinIE6 do in
its strict mode?

Comment 34

[Simon Fraser [no longer active]]

MacIE does what mozilla is doing currently (wrap the aqua table around the
purple div).

Minor nits on the patch:
     maxWidth = aMetrics.width;
+    if (NS_STYLE_OVERFLOW_HIDDEN != aReflowState.mStyleDisplay->mOverflow)

Is the less readable "const != variable" still the preferred ordering for
conditionals?

+      maxWidth = PR_MAX(inset + aState.mMaxElementSize.width, aMetrics.width);

PR_MAX will evaluate "inset + aState.mMaxElementSize.width" twice (depending on
compiler optimizations). Might be better to precalculate it.
Also at:
+    aMetrics.mMaximumWidth = PR_MAX(maxWidth, aState.mMaximumWidth +
borderPadding.right);

Comment 35

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

kin never superreviewed this patch (at least not according to the bug), but it
was just checked in with sr=kin.  I'm still opposed (see comment 33).

Comment 36

[karnaze (gone)]

Sorry about that, I'll back it out, and reassign the bug.

Comment 37

[karnaze (gone)]

->kin

Comment 38

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

->me

Comment 39

[Martijn Wargers (dead)]

The testcase seems identical to Opera7.23 now, using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/2004-06-23
Firefox/0.8.0+

And I can see the bug, using Mozilla1.4:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4.1) Gecko/2003-10-08

So I guess this one can be resolved, marking WORKSFORME?

Comment 40

[Bernd]

Yes this has been fixed by the implementing the overflow area in tables.

Comment 41

[Bernd]

Comment on attachment 107123 [details] [diff] [review]
revised patch addressing comments

this patch is not necessary any more