Closed
Bug 176423
Opened 22 years ago
Closed 20 years ago
image inside table get truncated
Categories
(Core :: Layout: Block and Inline, defect, P3)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
WORKSFORME
mozilla1.5beta
People
(Reporter: jerry.tan, Assigned: dbaron)
References
Details
Attachments
(2 files, 3 obsolete files)
420 bytes,
text/html
|
Details | |
1.77 KB,
patch
|
alexsavulov
:
review+
|
Details | Diff | Splinter Review |
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:
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.
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•22 years ago
|
||
I commented on a bug with identical content yesterday. Please find and dup.
Whiteboard: DUPEME
Assignee | ||
Comment 5•22 years ago
|
||
I agree that Opera's behavior is correct.
Comment 6•22 years ago
|
||
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
Updated•22 years ago
|
Priority: -- → P3
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.
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•22 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•22 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
Comment 12•22 years ago
|
||
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?
Assignee | ||
Comment 13•22 years ago
|
||
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•22 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•22 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•22 years ago
|
||
I meant to say comment #13.
Comment 17•22 years ago
|
||
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•22 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 18•22 years ago
|
||
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•22 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•22 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•22 years ago
|
Attachment #105220 -
Flags: superreview?(kin)
Attachment #105220 -
Flags: review?(alexsavulov)
Updated•22 years ago
|
Attachment #105220 -
Flags: review?(alexsavulov) → review+
Assignee | ||
Comment 21•22 years ago
|
||
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•22 years ago
|
||
Good point. Maybe I should set max width appropriately and see what happens.
Comment 23•22 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•22 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.
Assignee | ||
Comment 25•22 years ago
|
||
What does the alignment have to do with whether the border should be included?
Comment 26•22 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);
Assignee | ||
Comment 27•22 years ago
|
||
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•22 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•22 years ago
|
||
Attachment #105220 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #107123 -
Flags: superreview?(kin)
Attachment #107123 -
Flags: review?(alexsavulov)
Comment 30•22 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•22 years ago
|
||
Comment on attachment 105220 [details] [diff] [review] patch to include overflow in mes obsolete patch; adding -
Attachment #105220 -
Flags: superreview?(kin) → superreview-
Assignee | ||
Comment 32•22 years ago
|
||
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+
Assignee | ||
Comment 33•22 years ago
|
||
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•22 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);
Assignee | ||
Comment 35•22 years ago
|
||
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•22 years ago
|
||
Sorry about that, I'll back it out, and reassign the bug.
Assignee | ||
Updated•21 years ago
|
Attachment #107123 -
Flags: superreview?(kinmoz) → superreview?(dbaron)
Comment 39•20 years ago
|
||
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•20 years ago
|
||
Yes this has been fixed by the implementing the overflow area in tables.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WORKSFORME
Comment 41•20 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.
Description
•