Closed Bug 149275 Opened 22 years ago Closed 22 years ago

Wrong <div> height (if overflow: auto)

Categories

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

Product:
Core
Component:
Layout: Tables
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: juarez_jc, Assigned: karnaze)

Details

(Whiteboard: [PATCH])

Attachments

(3 files, 2 obsolete files)

testcase from the reporter
550 bytes, text/html
Details
reduced test case from comment #2
430 bytes, text/html
Details
patch created from a recent build
27.62 KB, patch
karnaze
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.0rc3) Gecko/20020523
BuildID:    

In the code bellow the <DIV> has a zero px height. If the overflow:auto is
removed the height is correct (100% of the TD).

<html>
<body>
	<table border='1' width='100%' height='100%'>
		<tr>
			<td height='50px' valign=top>
				<center>abcdef
			</td>
		</tr>
		<tr>
			<td>
				<div id=dvFundos style='height:100%;width:750px;overflow:auto;border-style:solid;'>
					<center>
					<table width=900px>
						<tr>
							<td>
								Line 1<br>
								Line 2<br>
								Line 3<br>
								Line 4<br>
								Line 5<br>
								Line 6<br>
								Line 7<br>
								Line 8<br>
					</table>
				</div>
			</td>
		</tr>
	</table>
</body>
</html>


Reproducible: Always
Steps to Reproduce:
1.Save the code and load it in the 1.0RC3
Confirming the bug with branch build: 2002061708 on WIN2K.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
<html>
<body>
	<table border='1' height='100'>		
		<tr>
			<td>
				<div id=dvFundos style='height:80%;overflow:auto;border-style:solid;'>					
					Text
				</div>
			</td>
		</tr>
	</table>
</body>
</html>

More reduced testcase for it.

if set div's height to 80, it is ok.
So I think it is about percent height of element inside table cell.

    
    

    
  


  
The patch allocates the nsHTMLReflowState for the block and passes it into
nsBlockReflowContext::ReflowBlock where it gets initialized. This allows the
correct reflow state to be passed into DidReflow (so that if the block is
inside a table cell and has a percent height but no computed height, the table
cell will be notified and its table will do a 3rd pass height reflow).

    
  
Status: NEW → ASSIGNED
Whiteboard: [PATCH]
Target Milestone: --- → mozilla1.0.1

    
    

    
  
Comment on attachment 89323 [details] [diff] [review]
patch to pass correct nsHTMLReflowState to DidReflow

r= alexsavulov

there is some point in 

nsBlockReflowContext::ReflowBlock

where mFrame = aFrameRS.frame; and then nsIFrame* frame = aFrameRS.frame;

is there a reason for not using mFrame directly? just wondering.

        Attachment #89323 -
        Flags: review+

    
    

    
  
At first glance, it looks like this would unfix the bug for which buster
originally split ReflowBlock and DoReflowBlock.  It also makes it look like you
should recombine them since there's no need for them to be separate and there's
no *logical* distinction.

I'd like to look at the patch more closely, though.

    
    

    
  
It doesn't unfix or break anything in the regression tests. It basically makes 
sure that the correct nsHTMLReflowState is passed into DidReflow (which I think 
only tables care about). It also does some cleanup with regards to parameter 
passing, but maybe it could have done more by combining the two methods. If the 
two methods should be combined, maybe we could open another bug on that.

    
    

    
  
Oh, now I see what you did with that PR_FALSE and the Init calls.  I find that
ugly and confusing, but I guess it will work.  (Please rename the shadowing
variable, though -- you have two variables called floaterRS in the same
function.)  It would also make sense to recombine ReflowBlock and DoReflowBlock,
not because I like big functions (I don't), but because there's no logical
separation between the functions and splitting a function into "part1" and
"part2" doesn't really fix the problem with large functions, but just adds to
the confusion.

    
    

    
  
So which part of this fixes the bug, and which part is the cleanup?

    
    

    
  
dbaron, the notion of allocating an nsHTMLReflowState and then intitializing it 
later on was introduced by tables some time ago. I don't think this is ugly. 
Perhaps it is a bit confusing, which is why I added comments explaining it and 
saying who will call init.

waterson, I don't want to make a big point about the cleanup aspect. For 
example, by passing in an nsHTMLReflowState, the following params can be removed 
because the relevant info is in the nsHTMLReflowState. The net result is one 
less param. No big deal. I think there are other similar cases.

-nsBlockReflowContext::ReflowBlock(nsIFrame* aFrame,
-                                  const nsRect& aSpace,

    
    

    
  


  

    
  

        Attachment #89323 -
        Attachment is obsolete: true

    
  

        Attachment #90826 -
        Flags: review+

    
  
Summary: Wrong <div> height → Wrong <div> height (if overflow: auto)

    
    

    
  
Bug 57512, bug 129643, bug 148847, bug 157099, and bug 165760 may all be
duplicates of this bug. QA, please check each of those bugs when verifying this bug.

    
    

    
  


  

        Attachment #90826 -
        Attachment is obsolete: true

    
  

        Attachment #97726 -
        Flags: review+

    
    

    
  
Comment on attachment 97726 [details] [diff] [review]
patch created from a recent build

sr=bzbarsky if you get rid of the "frame" temporary and just use mFrame and
make the comment changes we discussed on AIM.

        Attachment #97726 -
        Flags: superreview+

    
    

    
  
The patch is in. I forgot to make a new patch with the latest comments and 
attach it to the bug.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: