Margins not respected when dynamically adding nodes in body.onload

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: devbits, Assigned: roc)

Tracking

({fixed1.8, testcase})

Trunk
fixed1.8, testcase
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 184734 [details]
Test case

Updated

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

Comment 3

13 years ago
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).
Created attachment 191309 [details] [diff] [review]
fix

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?

Updated

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

Updated

13 years ago
Flags: blocking1.8b4+
checked in on branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.