231264 - iFrame with percent width in table too wide

Status: RESOLVED FIXED

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

Description

[Mark Slater]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031207 Firebird/0.7+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031207 Firebird/0.7+

When clicking on the link "Foobar" in the URL above, the link calls a JS
function to set a div's visibility to block. In that div is an iFrame, set to
100% width. The td holding the div is set to 250px width. Theoretically, the
iframe should make itself as wide as that (minus the space used for padding
etc.), so about 200px. However, the iFrame takes up more space than that,
resulting in the td growing wider than 250px.

See also discussion here:
http://forums.mozillazine.org/viewtopic.php?p=334057

Reproducible: Always

Steps to Reproduce:
1. Load site
2. Click "Foobar"

Actual Results:  
The div changed visibility, showing the iframe, but the iframe changed the width
of the containing td

Expected Results:  
Made the iframe 100% in the defined space (250px), not add width to it.

I tested on the following builds (all with new profiles) and I saw the problem:

- Firebird (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a)
Gecko/20040110 Firebird/0.8.0+ (scragz), Win2k)

- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040115
Firebird/0.8.0+


Going back to this version made the div appear as intended:

- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031207
Firebird/0.7+

Comment 1

[Jason Barnabe (np)]

I see this on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a)
Gecko/20040118.

Confirming and moving to Browser General.

Comment 2

[Jason Barnabe (np)]

Attachment: testcase

Comment 3

[Jason Barnabe (np)]

Moving to Layout: Tables.

Comment 4

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

That's the default width for an iframe with no width specified, right?

Comment 5

[Andrew Schultz]

regression between linux trunk 2004010907 and 2004011007

Comment 6

[Bernd]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=01%2F09%2F2004+05%3A00%3A00&maxdate=01%2F10%2F2004+07%3A00%3A00&cvsroot=%2Fcvsroot

Comment 7

[Mark Slater]

Using Firefox (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6)
Gecko/20040206 Firefox/0.8), I do not see this bug any more, neither in the
testcase URL nor elsewhere. Unless anybody else (Jason?) is still seeing this, I
vote to verify as fixed and close this bug.

Comment 8

[Andrew Schultz]

Mark: the nightly I tested in comment 5 have later Gecko version than Firefox 0.8.
it still occurs with linux trunk 2004021408

Comment 9

[Andrew Schultz]

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

Comment 10

[Bernd]

Attachment: reflow log

Comment 11

[Bernd]

This looks like a regression from bug 225820, the reflow logic for the
incremental reflow where the subdoc gets its initial reflow looks broken to me.

Comment 12

[Bernd]

Attachment: testcase without incr. reflow

Robert changed the MEW computation,
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/html/document/src&command=DIFF&root=/cvsroot&file=nsFrameFrame.cpp&rev1=3.241&rev2=3.242#45
.
So what happens now is that if we make a unconstrained reflow then we cant
compute the percent width,  so we use the default width instead
(http://lxr.mozilla.org/mozilla/source/layout/html/document/src/nsFrameFrame.cpp#310
). But later we use that default width as the MEW and this is just wrong.  We
are of course able to shrink the iframe further if the available width is
smaller then our desired size. Before that change we used the MEW from the
iframe content to make sure that it fits the width in that auto case.

Comment 13

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

Okay, thanks Bernd!

Comment 14

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

Attachment: fix

Set MEW to our total desired width if our 'width' style is a length, otherwise
set it to just the size of our borders (i.e., allow the contained document to
shrink to nothing).

Comment 15

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

Attachment: enhanced testcase

This testcase elaborates the earlier testcase (for which, much thanks!). It
shows that we're changing behaviour a little bit: the IFRAME's borders now end
up overlapping the table border. But I think this is correct; we're setting the
content width to 100% of the cell width, so borders must overflow. This
testcase shows that's what DIVs do, too.

Comment 16

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

Comment on attachment 143769 [details] [diff] [review]
fix

Another layout regression fix ... another candidate for 1.7b

Comment 17

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

Comment on attachment 143769 [details] [diff] [review]
fix

>-    vm->MoveViewTo(mInnerView, offset.x, offset.y);
>+    vm->MoveViewTo(mInnerView, border.left, border.top);

Are you sure this doesn't do something weird if CSS specifies a border on a
FRAME element?

>+    if (NS_UNCONSTRAINEDSIZE != aReflowState.mComputedWidth
>+        && GetStylePosition()->mWidth.GetUnit() != eStyleUnit_Percent) {

Why should it matter what aReflowState.mComputedWidth is?

Comment 18

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

> Why should it matter what aReflowState.mComputedWidth is?

If someone does <iframe style="width:100px;"> then shouldn't the MEW be 100px?
(This is what we used to do in nsHTMLFrameInnerFrame::GetDesiredSize, see the
diff URL bernd posted.)

> Are you sure this doesn't do something weird if CSS specifies a border on a
> FRAME element?

No. I should just set border to 0 for FRAMEs. I'll fix that.

Comment 19

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

Actually, why shouldn't we honor borders and padding on FRAME elements?

Comment 20

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

(In reply to comment #18)
> > Why should it matter what aReflowState.mComputedWidth is?
> 
> If someone does <iframe style="width:100px;"> then shouldn't the MEW be 100px?
> (This is what we used to do in nsHTMLFrameInnerFrame::GetDesiredSize, see the
> diff URL bernd posted.)

But you're already getting that information from the unit check -- and
mComputedWidth can vary for percentage widths depending on the type of reflow --
and you don't want the MEW to vary depending on the type of reflow.  (I think
you want to drop the aReflowState.mComputedWidth check and just check the unit
on the width in the style struct.  Does that make sense?)

Comment 21

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

If you want to start honoring border and padding on FRAME elements, the beta
freeze isn't the time to do it. :-)

Comment 22

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

> If you want to start honoring border and padding on FRAME elements, the beta
> freeze isn't the time to do it. :-)

Fair enough. I'll copy the old behaviour then.

> (I think you want to drop the aReflowState.mComputedWidth check and just check
> the unit on the width in the style struct.  Does that make sense?)

I believe you're suggesting this code:

if (aDesiredSize.mComputeMEW) {
-    aDesiredSize.mMaxElementWidth = aDesiredSize.width;
+    // If our width is set by style to some fixed length,
+    // then our actual width is our minimum width
+    if (GetStylePosition()->mWidth.GetUnit() != eStyleUnit_Percent) {
+      aDesiredSize.mMaxElementWidth = aDesiredSize.width;
+    } else {
+      // if our width is not fixed by style, then we can shrink until
+      // there's nothing left but our borders
+      aDesiredSize.mMaxElementWidth = border.left + border.right;
+    }
   }

... but what about width:auto? It seems to me that a width:auto IFRAME is going
to get aDesiredSize.width == 300 (our default) and then we will set MEW to 300
... which will give rise to this bug again! It seems to me that we should only
set MEW to aDesiredSize.width when 'width' was an explicit length.

By the way, am I the only person who thinks "MaxElementWidth" is horribly
confusing, since it really means the *minimum* width of the frame?

Comment 23

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

Yes, it's confusing.

Maybe check 'auto' explicitly, if that's what you intended.  (I wasn't sure how
you wanted 'auto' to behave.)  But you shouldn't make percents behave
differently depending on whether it's an unconstrained-width or fixed-width reflow.

Comment 24

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

Attachment: patch v2

as requested

Comment 25

[Hixie (not reading bugmail)]

Based on the comments above, this is presumably a case that is not explained by
the CSS specs. Could someone explain what the issue is so I can raise it in the
working group when appropriate? I couldn't really work out what the issue was.

Comment 26

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

Ian, it's a table layout issue, which boils down to "what should the minimum
size of a width:auto IFRAME be?" I thought the spec is deliberately silent on
issues with the default table layout strategy.

BTW should we be applying box-sizing to IFRAMEs to make this testcase keep the
IFRAMEs borders inside the table cell?

Comment 27

[Hixie (not reading bugmail)]

(In reply to comment #26)
> Ian, it's a table layout issue, which boils down to "what should the minimum
> size of a width:auto IFRAME be?"

Ah. Wouldn't that be the same inside and outside of a table? (and based on the
min-width property?)


> I thought the spec is deliberately silent on issues with the default table 
> layout strategy.

Yup, in CSS2.1 it is.


> BTW should we be applying box-sizing to IFRAMEs to make this testcase keep 
> the IFRAMEs borders inside the table cell?

What our UA stylesheet gives as the value for box-sizing is up to us, I don't
really have an opinion either way so long as we do the right thing if the author
changes the value.

Comment 28

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

> What our UA stylesheet gives as the value for box-sizing is up to us,

Right, that was a Web compatibility question, not a standards question :-).

> Wouldn't that be the same inside and outside of a table?

Well, MEW doesn't mean exactly "minimum size" ... it's more like "preferred
minimum size". E.g. the MEW of a DIV containing only text is the width of its
longest word, even though the block *can* be narrower than that. AFAIK only we
only use this metric for laying out tables (with the default strategy).

Comment 29

[Hixie (not reading bugmail)]

Oh, _that_ minimum. Yeah, not really something CSS2.1 is going to address. (Are
we emulating IE here?)

Comment 30

[Bernd]

Isn't our MEW similiar to the minimum content width (MCW) of CSS2.1?

Comment 31

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

Yeah, it is, but CSS 2.1 doesn't explain how to calculate the MCW :-). Maybe it
tries to in this sentence:

> Calculate the minimum content width (MCW) of each cell: the formatted content
> may span any number of lines but may not overflow the cell box.

But, even if we're generous to the writers of CSS2.1, and take this to mean "the
minimum width such that the cell contents do not overflow the cell box", that
doesn't actually work as a definition. For example using position:relative and
other tricks one can have cells that always overflow the cell box.

Comment 32

[chris hofmann]

Comment on attachment 143841 [details] [diff] [review]
patch v2

a=chofmann for 1.7 after we get beta builds done.

Comment 33

[Bernd]

>may not overflow the cell box.
yeah we have been soooo standards compliant that tables did not even have a
overflow area, but then I feeded the gremlins and now these critters (bugs) are
all over.

Comment 34

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

That sentence is within a section described by "UAs are not required to
implement this algorithm to determine the table layout in the case that
'table-layout' is 'auto'; they can use any other algorithm."

Comment 35

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

checked in 

Comment 36

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

marking fixed.