iFrame with percent width in table too wide

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: mark.slater, Assigned: roc)

Tracking

({regression, testcase})

Trunk
x86
All
regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

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

15 years ago
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.
Status: UNCONFIRMED → NEW
Component: General → Browser-General
Ever confirmed: true
Product: Firebird → Browser
Version: unspecified → Trunk

Updated

15 years ago
Assignee: blake → general
QA Contact: general

Comment 2

15 years ago
Created attachment 140350 [details]
testcase

Comment 3

15 years ago
Moving to Layout: Tables.
Assignee: general → nobody
Component: Browser-General → Layout: Tables
Keywords: testcase
QA Contact: general → core.layout.tables
That's the default width for an iframe with no width specified, right?

Comment 5

15 years ago
regression between linux trunk 2004010907 and 2004011007
Keywords: regression
OS: Windows 2000 → All
(Reporter)

Comment 7

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

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

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

Updated

15 years ago
Summary: bizarre iFrame width → iFrame with percent width in table too wide

Comment 10

15 years ago
Created attachment 141947 [details]
reflow log

Comment 11

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

15 years ago
Created attachment 141998 [details]
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.

Updated

15 years ago
Attachment #141998 - Attachment is patch: false
Attachment #141998 - Attachment mime type: text/plain → text/html
Okay, thanks Bernd!
Assignee: nobody → roc
Priority: -- → P2
Created attachment 143769 [details] [diff] [review]
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).
Created attachment 143770 [details]
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 on attachment 143769 [details] [diff] [review]
fix

Another layout regression fix ... another candidate for 1.7b
Attachment #143769 - Flags: superreview?(dbaron)
Attachment #143769 - Flags: review?(dbaron)
Attachment #143769 - Flags: approval1.7b?
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?
> 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.
Actually, why shouldn't we honor borders and padding on FRAME elements?
(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?)
If you want to start honoring border and padding on FRAME elements, the beta
freeze isn't the time to do it. :-)
> 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?
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.
Created attachment 143841 [details] [diff] [review]
patch v2

as requested
Attachment #143769 - Attachment is obsolete: true
Attachment #143841 - Flags: superreview?(dbaron)
Attachment #143841 - Flags: review?(dbaron)
Attachment #143841 - Flags: approval1.7b?
Attachment #143769 - Flags: superreview?(dbaron)
Attachment #143769 - Flags: review?(dbaron)
Attachment #143769 - Flags: approval1.7b?
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.
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?
(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.
> 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).
Oh, _that_ minimum. Yeah, not really something CSS2.1 is going to address. (Are
we emulating IE here?)

Comment 30

15 years ago
Isn't our MEW similiar to the minimum content width (MCW) of CSS2.1?
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.
Attachment #143841 - Flags: superreview?(dbaron)
Attachment #143841 - Flags: superreview+
Attachment #143841 - Flags: review?(dbaron)
Attachment #143841 - Flags: review+
Attachment #143841 - Flags: approval1.7b?
Attachment #143841 - Flags: approval1.7?

Comment 32

15 years ago
Comment on attachment 143841 [details] [diff] [review]
patch v2

a=chofmann for 1.7 after we get beta builds done.
Attachment #143841 - Flags: approval1.7? → approval1.7+

Comment 33

15 years ago
>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.
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."
marking fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.