image inside table get truncated

RESOLVED WORKSFORME

Status

()

P3
normal
RESOLVED WORKSFORME
16 years ago
14 years ago

People

(Reporter: jerry.tan, Assigned: dbaron)

Tracking

Trunk
mozilla1.5beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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:
(Reporter)

Comment 1

16 years ago
Created attachment 103949 [details]
testcase
(Reporter)

Comment 2

16 years ago
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

16 years ago
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.
I commented on a bug with identical content yesterday.  Please find and dup.
Whiteboard: DUPEME
I agree that Opera's behavior is correct.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: DUPEME
(Reporter)

Comment 7

16 years ago
assign it to me
Assignee: table → jerry.tan
(Reporter)

Updated

16 years ago
Blocks: 174665

Updated

16 years ago
Priority: -- → P3
(Reporter)

Updated

16 years ago
Blocks: 166758
(Reporter)

Comment 8

16 years ago
Created attachment 104709 [details] [diff] [review]
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.
(Reporter)

Comment 9

16 years ago
Created attachment 104834 [details] [diff] [review]
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.
Attachment #104709 - Attachment is obsolete: true
(Reporter)

Comment 10

16 years ago
Comment on attachment 104834 [details] [diff] [review]
patch 0.2

patch 0.2 doesnot pass regression test, so mark it as obsolete
Attachment #104834 - Attachment is obsolete: true

Comment 11

16 years ago
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
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?
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

16 years ago
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

16 years ago
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?). 
Assignee: jerry.tan → karnaze
Target Milestone: --- → mozilla1.3beta

Comment 16

16 years ago
I meant to say comment #13.

Comment 17

16 years ago
Created attachment 105220 [details] [diff] [review]
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.

Updated

16 years ago
Status: NEW → ASSIGNED
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

16 years ago
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?
(Reporter)

Comment 20

16 years ago
yep, the patch is good.
change component to layout/block-inline, since it is not a table bug.
Component: Layout: Tables → Layout: Block & Inline

Updated

16 years ago
Attachment #105220 - Flags: superreview?(kin)
Attachment #105220 - Flags: review?(alexsavulov)

Updated

16 years ago
Attachment #105220 - Flags: review?(alexsavulov) → review+

Updated

16 years ago
Blocks: 102100

Updated

16 years ago
No longer blocks: 102100
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

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

Comment 23

16 years ago
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

16 years ago
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.
What does the alignment have to do with whether the border should be included?

Comment 26

16 years ago
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);
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

16 years ago
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

16 years ago
Created attachment 107123 [details] [diff] [review]
revised patch addressing comments
Attachment #105220 - Attachment is obsolete: true

Updated

16 years ago
Attachment #107123 - Flags: superreview?(kin)
Attachment #107123 - Flags: review?(alexsavulov)

Comment 30

16 years ago
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=
Attachment #107123 - Flags: review?(alexsavulov) → review+

Comment 31

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

obsolete patch; adding -
Attachment #105220 - Flags: superreview?(kin) → superreview-
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.
Attachment #107123 - Flags: superreview?(kin) → superreview+
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?
Attachment #107123 - Flags: superreview+ → superreview?(kin)

Comment 34

16 years ago
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);
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

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

Comment 37

16 years ago
->kin
Assignee: karnaze → kin
Status: ASSIGNED → NEW

Updated

16 years ago
Target Milestone: mozilla1.3beta → mozilla1.5alpha

Updated

16 years ago
Target Milestone: mozilla1.5alpha → mozilla1.5beta
->me
Assignee: kin → dbaron
Attachment #107123 - Flags: superreview?(kinmoz) → superreview?(dbaron)
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

15 years ago
Yes this has been fixed by the implementing the overflow area in tables.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WORKSFORME

Comment 41

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

this patch is not necessary any more
Attachment #107123 - Flags: superreview?(dbaron)
You need to log in before you can comment on or make changes to this bug.