Closed Bug 295767 Opened 19 years ago Closed 19 years ago

Margins not respected when dynamically adding nodes in body.onload

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: devbits, Assigned: roc)

Details

(Keywords: fixed1.8, testcase)

Attachments

(2 files)

Test case
599 bytes, text/html
Details
fix
16.65 KB, patch
dbaron
: review+
dbaron
: superreview+
asa
: approval1.8b4+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

There's a layout problem when adding nodes through the DOM after the page (see 
steps to reproduce) has finished loading.
The page renders fine when the function is called as part of the load process 
and before it finishes, but renders wrong when called as part of the 
body.onload event.
The included page includes the minimal markup required to reproduce the 
problem.

Reproducible: Always

Steps to Reproduce:
<html>
<head>
<style>
.clsTest {
	margin-top: 0.5em;
	border: 1px solid #000000;
}
</style>
</head>
<body onload="createSpan();">

<span>A static span</span>
<div class="clsTest">&nbsp;</div>

<p>Some paragraph text.</p>

<div class="clsTest" id="idDiv">&nbsp;</div>

<script type="text/javascript">
function createSpan() {
	var elemSpan = document.createElement("span");
	elemSpan.appendChild(document.createTextNode("A dynamic span"));
	
	var elemDiv = document.getElementById("idDiv");
	elemDiv.parentNode.insertBefore(elemSpan, elemDiv);
}
</script>
</body>
</html>
Actual Results:  
The 'static' span is displayed at the proper 0.5em margin above the div, the 
dynamically added one is displayed at a 1em margin above the div.
Note that calling 'createSpan()' from body.onload is what causes the problem, 
if the call is moved to within the script element and hence executed during 
page load, it'll render fine.

Also note that increasing and decreasing (Ctrl + followed by Ctrl -) will 
rectify the problem until reload.

Expected Results:  
It should have respected and properly rendered the requested margin.
Attached file Test case — 
Keywords: testcase

    
    

    
  
Looks like more margin-collapsing fun, roc...  Any idea what's up here?
Assignee: general → nobody
Status: UNCONFIRMED → NEW
Component: DOM: CSSOM → Layout: Block and Inline
Ever confirmed: true
OS: Windows XP → All
QA Contact: ian → layout.block-and-inline
Hardware: PC → All

    
    

    
  
I just ran into this in my own app and found this bug, but I was using
insertBefore() to add nodes based on user action (a little AJAX goodness).
Again, margins not applied correctly to the inserted node (a DIV in this case).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix
        — 
          — Splinter Review
      

    


  
Okay, here's a more principled approach to this whole MarkPreviousMarginDirty
decision. I think you can verify by inspection that this code will fire
strictly more often than the old code.
Assignee: nobody → roc
Status: NEW → ASSIGNED

        Attachment #191309 -
        Flags: superreview?(dbaron)

        Attachment #191309 -
        Flags: review?(dbaron)

    
    

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

I assume you intended to attach only the nsBlockFrame.cpp changes and that the
other changes are for other bugs.

It's not obvious to verify that by inspection.	Any chance you could briefly
explain how?

The last paragraph of the long comment looks like it's wrapped at a different
width than the rest?  Is it more than 80 characters?

The parenthesization here seems unnecessary:
+	 if (maybeReflowingForFirstTime /*1*/ ||
+	     (isEmpty || maybeWasEmpty) /*2/3/4*/) {
unless there's a particular reason for it.

    
    

    
  
(In reply to comment #5)
> (From update of attachment 191309 [details] [diff] [review] [edit])
> I assume you intended to attach only the nsBlockFrame.cpp changes and that the
> other changes are for other bugs.

Sorry!

> It's not obvious to verify that by inspection.	Any chance you could
> briefly explain how?

Well, that's because it's not quite true :-). It is true if you replace
"line->mBounds.height != 0" with "line->mBounds.height != 0 ||
!line->CachedIsEmpty()" in the old code (the former was approximating the
latter) and similarly, "line->mBounds.height == 0" with "line->mBounds.height ==
0 && line->CachedIsEmpty()". Then we can show that if
line.next()->MarkPreviousMarginDirty(); was called before, it will be called now.

Here's how. Let's assume it was called before. Then line.next() != end_lines()
so we get into the inner "if" in the new code. Then one of three cases holds:
-- line->mBounds.height == 0 && line->CachedIsEmpty() && previousMarginWasDirty:
then isEmpty is true in the new code and we call MarkPreviousMarginDirty.
-- maybeReflowingForFirstTime is true: then we call MarkPreviousMarginDirty.
-- oldY == 0 && line->mBounds.y == 0 && !(oldYMost != 0 && (line->mBounds.height
!= 0 || !line->CachedIsEmpty())): rewrite as oldY == 0 && line->mBounds.y == 0
&& (oldYMost == 0 || (line->mBounds.height == 0 && line->CachedIsEmpty()); then
there are two subcases:
  -- oldY == 0 && line->mBounds.y == 0 && oldYMost == 0; then maybeWasEmpty is
true in the new code and we call MarkPreviousMarginDirty.
  -- oldY == 0 && line->mBounds.y == 0 && line->mBounds.height == 0 &&
line->CachedIsEmpty(); then isEmpty is true in the new code and we call
MarkPreviousMarginDirty.

> The last paragraph of the long comment looks like it's wrapped at a different
> width than the rest?  Is it more than 80 characters?

I'll fix it.

> The parenthesization here seems unnecessary:
> +	 if (maybeReflowingForFirstTime /*1*/ ||
> +	     (isEmpty || maybeWasEmpty) /*2/3/4*/) {
> unless there's a particular reason for it.

I did it for clarity but it doesn't really matter. I'll remove it.

    
    

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

It seems like there will be many lines that have their previous margins marked
dirty on every reflow.	This seems like it could be bad for performance on long
pages.	But it seems like that's true both with and without the patch, so, even
though I don't follow the whole thing, r+sr=dbaron on the nsBlockFrame.cpp
changes.

        Attachment #191309 -
        Flags: superreview?(dbaron)

        Attachment #191309 -
        Flags: superreview+

        Attachment #191309 -
        Flags: review?(dbaron)

        Attachment #191309 -
        Flags: review+

    
    

    
  
checked in. I'll ask for branch approval

    
    

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

Only the nsBlockFrame part of this patch is relevant, the rest is unrelated. It
fixes an incremental reflow bug. No regressions have been observed on trunk.

        Attachment #191309 -
        Flags: approval1.8b4?

    
  

        Attachment #191309 -
        Flags: approval1.8b4? → approval1.8b4+

    
  
Flags: blocking1.8b4+

    
    

    
  
checked in on branch.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: