Closed Bug 412679 Opened 17 years ago Closed 17 years ago

iframe not using intrinsic height

Categories

(Core :: Layout: Positioned, defect)

Product:
Core
Component:
Layout: Positioned
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dennis.j.bell, Assigned: bzbarsky)

References

Details

(Whiteboard: [really duplicate of bug 363675])

Attachments

(2 files, 1 obsolete file)

testcase from comment #0
851 bytes, application/xhtml+xml
Details
Fix XUL stuff
15.30 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

If FF2, an absolutely positioned iframe with CSS styles of width set to auto and left and right set to 0 will resize to fill the containing absolutely-positioned element (in this case, a div).  This does not work in FF3.  However, both FF2 and FF3 expand the height when top and bottom CSS styles are specified.

Reproducible: Always

Steps to Reproduce:
Use the following code snippet as the source, and open in FF2 and FF3 to compare:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
  <head>
    <style type="text/css"> /*<![CDATA[*/
      body {background-color: #888;}
      iframe {
        position:absolute;
        top:0px;
        left:0px;
        right:0px;
        bottom:0px;
        border: 2px inset wheat;
      }
      div#container {
        position:absolute;
        top: 5em;
        left: 5em;
        right: 5em;
        bottom:5em;
        border: 2px solid black;
        background-color: wheat;
      }
    /*]]>*/
    </style>
    <title></title>
  </head>
  <body>
    <div id="container">
      <iframe id="content" src="http://www.getfirefox.com" name="content"></iframe>
    </div>
  </body>
</html>

Actual Results:  
The iframe responds to CSS style settings for top and bottom by filling the containing div from top to bottom in both browsers, but only filled the container from left to right in the FF2 browser, not the FF3 browser.  Resizing the browser's windows result in the height and width of the iframe to match that of the containing div in FF2, but only the height of the iframe and not the width resize to match the containing div in FF3

Expected Results:  
The FF3 browser's rendering of the page should look like that of the FF2's rendering,

    
  
Component: General → Layout: R & A Pos
Product: Firefox → Core
QA Contact: general → layout.r-and-a-pos
Version: unspecified → Trunk

    
  
URL: internal site - but a representitive ...
OS: Windows XP → All

    
    

    
  
Confirming with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011504 Minefield/3.0b3pre
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
The same thing happens in Opera 9.25.  And I think it's rare that both Mozilla and Opera are wrong.  

If I'm interpreting CSS 2.1 correctly then I think this bug is invalid.  Remember an iframe is a replaced element.  And the result in the testcase is an iframe that is 300 pixels wide.

from 10.3.8 Absolutely positioned, replaced elements:
"1. The used value of 'width' is determined as for inline replaced elements. "

I'm not sure but I think this is what's happening here:

from 10.3.2 Inline, replaced elements:
"Otherwise, if 'width' has a computed value of 'auto', but none of the conditions above are met, then the used value of 'width' becomes 300px. If 300px is too wide to fit the device, UAs should use the width of the largest rectangle that has a 2:1 ratio and fits the device instead."

Setting width:100%; on the iframe does get the desired behavior.  

    
    

    
  
(In reply to comment #4)

I was about to comment the same as you. With the following addition:
1. WebKit behaves nearly the same as Opera 9.25 (shrink-wrapped width).
2. Opera 9.5b and WebKit: the height of the iframe is different: about 150px tall (they don't take the 'bottom' value into consideration): a 2:1 ratio.

WebKit: latest nightly build and Safari 3.04, running on 10.4.11.


    
    

    
  
I was wondering about the height.  But I have no idea if there is an intrinsic ratio for an iframe. The 2:1 given only applies if device is less than 300pxs wide.  (Like perhaps a handheld device) 

    
    

    
  
(In reply to comment #6)
10.6.5 applies here, which points to 10.6.2 (as 'height' of the iframe is auto), where you'll see a paragraph mentioning the 2:1 ratio, and 150px.


    
    

    
  
The width thing is a bug in Fx2.  Comment 4 is spot on about what the spec says here.  That part is basically a duplicate of bug 410146.

The height part is a bug, just like bug 385870.

The 2:1 thing is interesting; it wasn't there when I last looked at the spec.  Yay mutating specs!  But it's not relevant: iframes have a 150px intrinsic height.

    
    

    
  
(In reply to comment #4)
> The same thing happens in Opera 9.25.  And I think it's rare that both Mozilla
> and Opera are wrong.  
> 
> If I'm interpreting CSS 2.1 correctly then I think this bug is invalid. 
> Remember an iframe is a replaced element.  And the result in the testcase is an
> iframe that is 300 pixels wide.
> 
> from 10.3.8 Absolutely positioned, replaced elements:
> "1. The used value of 'width' is determined as for inline replaced elements. "
> 
> I'm not sure but I think this is what's happening here:
> 
> from 10.3.2 Inline, replaced elements:
> "Otherwise, if 'width' has a computed value of 'auto', but none of the
> conditions above are met, then the used value of 'width' becomes 300px. If
> 300px is too wide to fit the device, UAs should use the width of the largest
> rectangle that has a 2:1 ratio and fits the device instead."
> 
> Setting width:100%; on the iframe does get the desired behavior.  
> 

There appears to be a conflict in section 10.3, so I think section 9.2 takes
over as authoritative.  Let me explain:

First, section 10.3.8 specifies: 
  "This situation is similar to the previous one, except that the element has
an intrinsic width."

Then, in section 10.3.2, we can only get to the clause "Otherwise, if 'width'
has a computed value of 'auto', but none of the conditions above are met, then
the used value of 'width' becomes 300px", is if the element DOES NOT have an
intrinsic value (as the case where there is an intrinsic width is handled above
this clause)

For years, naked iframes have been rendered as 150x300, but that is due to this
misinterpretation.  Iframes, basically HTML pages, do not have intrinsic
dimensions (if it did, you shouldn't be able to resize your browser).  Infact,
the W3C standard for HTML 4.01 does not specify a default height or width.

Therefore, since we cannot follow the rules in 10.3.8 to generate a width (no
intrisic dimensions), we can follow the rules as if it was a non-replacement
element and follow the rules of 9.2 for positioned elements.

    
    

    
  
> For years, naked iframes have been rendered as 150x300

Yes.  That's the intrinsic size of an iframe in all existing layout engines.

Even if you assume that an iframe has no intrinsic width, it certainly has no intrinsic ratio.  So you get to the "otherwise" clause in section 10.3.2, as you point out.  So the used value still ends up 300px wide, modulo this new "fits the device" text, whatever it means.

I don't see a contradiction.  More importantly, 9.2 doesn't actually specify layout in detail; that's the job of chapter 10.

I can guarantee that iframe is in fact a replaced element, and that the 300px stuff in the CSS spec is there precisely because that's what all UAs do with unstyled iframes.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        This fixes it (obsolete)
        — 
          — Splinter Review
      

    


  
This also fixes checkboxes and radios, which were also not using their intrinsic height.  I haven't done any real testing on this past checking that it in fact fixes this bug.  I assume this is OK for the various XUL stuff inheriting nsLeafFrame, but it would be nice to double-check.  It would also be nice if someone has time to create some reftests...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED

        Attachment #297494 -
        Flags: superreview?(dbaron)

        Attachment #297494 -
        Flags: review?(dbaron)

    
    

    
  
(In reply to comment #10)
> > For years, naked iframes have been rendered as 150x300
> 
> Yes.  That's the intrinsic size of an iframe in all existing layout engines.
> 
> Even if you assume that an iframe has no intrinsic width, it certainly has no
> intrinsic ratio.  So you get to the "otherwise" clause in section 10.3.2, as
> you point out.  So the used value still ends up 300px wide, modulo this new
> "fits the device" text, whatever it means.
> 
> I don't see a contradiction.  More importantly, 9.2 doesn't actually specify
> layout in detail; that's the job of chapter 10.
> 
> I can guarantee that iframe is in fact a replaced element, and that the 300px
> stuff in the CSS spec is there precisely because that's what all UAs do with
> unstyled iframes.
> 

The conflict, which I grant is subtle, is that 10.3.8 is specifying how to layout replaced elemnts *with* intrinsic dimentions.  It states that it is "similar to the previous (10.3.7), except that the element has an intrinsic width".  

Since we can establish that iframes, though they be replaced elements, have *no intrinsic width*, I suggest the logical course of action is to use 10.3.7. -- the title of section 10.3.8 is erroneous, it should be "10.3.8 Absolutely positioned, replaced elements with intrinsic width" because that's what it says in the text.

As for following 9.2, I simply meant that we should take the box offsets of positioned elements into consideration to calculate the width, as is done in 10.3.7 - sorry for the miscommunication.

    
    

    
  
Please don't quote the whole text.  It makes things harder to read for no good reason.

The "except ..." line is informative, not normative.  You raise a good point, which is that it's confusing.  I'll raise the issue of removing it with the working group.  The title of the section is quite correct.

    
    

    
  
(In reply to comment #13)
Sorry about the quoting thing - I was just clicking on the reply link.

Thanks for the explanation and logic.  While I disagree with the standard's stance on rendering replaced elements by ignoring the box offsets, I respect that they are the standards we're meant to follow. People ask me why use Firefox, and I say because it's standards-compliant, so I'd be a major hypocrite if I suggested we should ignore the standard because "it's wrong' in my opinion.  

Perhaps in a future CSS standard, they may revisit the 'perceived need' to differentiate rendering based on the presence of intrinsic (constant?) dimension rather than whether the element is replaced or not.  Perhaps they'll determine that the abitrary width/height/ratio assignment is, well, arbitrary, and if it comes to that point in the decision tree, render it as in 10.3.7 instead.  

Until that day, if it ever comes, the 'work-around' for being standards-compliant is just wrap the iframe  element in a positioned div that does respond to CSS left/right and set the iframe's width to 100%.  This also has to be done for img elements, because like iframes, they're replaced elements and should no longer stretch like they did in FF2.

I'm satisfied that this has been looked into sufficiently.


    
    

    
  
Mutating this bug for the height issue.  Mutating bugs are better than mutating specs. :)
Summary: FF3: iframe does not resize due to CSS style using left and right attribute → iframe not using intrinsic height

    
    

    
  
Sounds like this bug is basically a duplicate of bug 363675.

    
    

    
  
Indeed.  Given that the patches are basically identical, shall we just mark this duplicate?

    
    

    
  
Comment on attachment 297494 [details] [diff] [review]
This fixes it

r+sr=dbaron, although I don't see how this differs from my patch for bug 363675 that didn't work.

Please add some tests as well...

        Attachment #297494 -
        Flags: superreview?(dbaron)

        Attachment #297494 -
        Flags: superreview+

        Attachment #297494 -
        Flags: review?(dbaron)

        Attachment #297494 -
        Flags: review+
Flags: blocking1.9?
Whiteboard: [really duplicate of bug 363675]

    
    

    
  
> although I don't see how this differs from my patch for bug 363675
> that didn't work.

I don't either... This patch definitely works.

I'll try to steal the tests in bug 363675, I guess.  Though again, if someone (Brian?) is willing to cook up some reftests that would be great.  I won't have time to work on the tests until sometime next week, most likely...


    
    

    
  
Comment on attachment 297494 [details] [diff] [review]
This fixes it

Requesting approval.  This gives us IE/Safari/Opera compat for iframe sizing in this case.

        Attachment #297494 -
        Flags: approval1.9?

    
  

        Attachment #297494 -
        Flags: approval1.9? → approval1.9+

    
  
Flags: in-testsuite?
Flags: blocking1.9?
Flags: blocking1.9+

    
    

    
  
Checked in.  I added some tests too.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED

    
    

    
  
I backed this out on suspicion that it regressed a reftest on Mac and Windows (although it's not clear how an image can change by -1 pixels):

REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/bugs/321402-3.xul

REFTEST   IMAGE 1 (TEST): [image/png;base64 data: URL]
REFTEST   IMAGE 2 (REFERENCE): [image/png;base64 data: URL]
REFTEST number of differing pixels: -1

Mac: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1201299512.1201300654.22189.gz#err0
Windows: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1201298142.1201300512.21761.gz#err0

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
The results from the ensuing builds are coming in now, and they confirm that backing out this change fixed the regression.


    
    

    
  
Uh...  That test hits:

###!!! ASSERTION: Someone didn't override Reflow: 'Not Reached', file ../../../mozilla/layout/generic/nsLeafFrame.cpp, line 120

I guess I'll need to hunt down and kill said someone...

    
    

    
  
Ah, I bet the problem is some subclass that does override Reflow() but not ComputeAutoSize().

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix XUL stuff
        — 
          — Splinter Review
      

    


  
Just needed to make nsLeafBoxFrame not call the new ComputeAutoSize, since it has an intrinsic width and height of 0....

        Attachment #297494 -
        Attachment is obsolete: true

        Attachment #299369 -
        Flags: superreview?(dbaron)

        Attachment #299369 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 299369 [details] [diff] [review]
Fix XUL stuff

r+sr=dbaron

        Attachment #299369 -
        Flags: superreview?(dbaron)

        Attachment #299369 -
        Flags: superreview+

        Attachment #299369 -
        Flags: review?(dbaron)

        Attachment #299369 -
        Flags: review+

    
    

    
  
+  NS_ASSERTION(aReflowState.ComputedWidth() != NS_UNCONSTRAINEDSIZE,
...
+  NS_ASSERTION(NS_INTRINSICSIZE != aReflowState.ComputedHeight(),

Really ... :-)

    
    

    
  
Relanded on trunk.  It stuck this time.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED

    
  
Depends on: 414321

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: