Closed Bug 312695 Opened 19 years ago Closed 19 years ago

[FIX]DIV content is rendered twice

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8rc1

People

(Reporter: mjuhos, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8, regression, testcase)

Attachments

(6 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Display hidden DIV element and set STYLE to this element.

If you change order of this operation, it works fine.

Reproducible: Always

Steps to Reproduce:
Try this code:
<html>
   <head>
     <script>
     onload = function() {
      document.getElementById("message").style.display = "block";
      document.getElementById("message").innerHTML =
"content<style>#message{color:blue}</style>";
     }
     </script>
  </head>

<body>
   <div id="message" style="display:none;"></div>
</body>
</html>

Tested on WinXP and on MacOS X too.
Actual Results:  
Firefox displays text "contentcontent".

Expected Results:  
Firefox will display text "content".
Version: unspecified → 1.5 Branch
Happens on trunk as well. Does not happen in Firefox 1.0.
Keywords: regression, testcase
Version: 1.5 Branch → Trunk
The regression range is between 2004-08-09-09 and 2004-08-10-15. Maybe bug 27382?

I'm confirming this in Core/DOM, which is my best guess.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → ian
Regression from bug 230170, looks like.  I thought I'd fixed this in bug 309986,
but clearly I didn't get all cases.  I'll debug tonight.
Blocks: 230170
Component: DOM → Layout: Misc Code
Flags: blocking1.8rc1?
Let us know what your debugging shows tonight (we might take a fix if it was
very low risk) and we're re-evaluate the status tomorrow. 
Sounds good.
Assignee: general → nobody
QA Contact: ian → layout.misc-code
So this is a DOM bug after all.  The problem is that when setting innerHTML we
use a document fragment.  The sequence of events here is:

1)  Insert text node into div
2)  Insert style node into div.  This triggers a parse of the sheet and a style
reresolve.  Since the style of the div changes, we rebuild the frames for the
subtree rooted at it, and construct a frame for the text.
3)  DOM notifies that the text got appended.  We construct _another_ frame for
the text.

I see three possible solutions offhand:

1)  Just notify for every single node in the fragment when inserting a fragment.
 This is probably safest from a correctness point of view, but might have
performance issues, both for setting .innerHTML and for editor.
2)  Handle the addition of a new sheet to the document asynchronously (that is,
when StyleSheetAdded comes in, don't do a reresolve immediately, or even at
EndUpdate, but rather just stick a pending reresolve in the frame constructor).
 This is what I think I'd like to do on trunk, in general.  But I'd not want to
do this on branch.
3)  Hook up an document observer for the duration of inserting a fragment and do
a simpler version what the HTML content sink does (notify on everything we've
inserted so far when BeginUpdate happens, and then make sure to not notify on
those nodes again).

For branch I think I like #3, actually.  It should be safe enough that I'd be ok
with doing it on branch even at this point...
Assignee: nobody → general
Component: Layout: Misc Code → DOM: Core
QA Contact: layout.misc-code → ian
Assignee: general → bzbarsky
Attached patch Proposed patchSplinter Review
This doesn't handle scripts in the fragment very well, just like the existing
code, but in the common cases (editor, innerHTML) they won't run in any case. 
This should handle anything that does non-content updates just dandy.  For 1.9
we really need to move toward not mutating things in any way from inside
BindToTree... or something.
Attachment #199855 - Flags: superreview?(jst)
Attachment #199855 - Flags: review?(bugmail)
I'll file followup bugs on the further work that needs doing here.
Priority: -- → P1
Summary: DIV content is rendered twice → [FIX]DIV content is rendered twice
Target Milestone: --- → mozilla1.8rc1
Attachment #199855 - Flags: review?(bugmail) → review?(jst)
Comment on attachment 199855 [details] [diff] [review]
Proposed patch

r+sr=jst
Attachment #199855 - Flags: superreview?(jst)
Attachment #199855 - Flags: superreview+
Attachment #199855 - Flags: review?(jst)
Attachment #199855 - Flags: review+
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 199855 [details] [diff] [review]
Proposed patch

Requesting branch approval.  This should be fairly safe -- all I'm doing in the
patch is notifying any time there's an update triggered by the content
addition, and notifying on what's left in the end, instead of notifying on
everything all together at the end.  This fixes various cases where innerHTML
adds stylesheets, basically.  There is some minor beneficial effect on actual
manipulations with document fragments (which no one does, really) that have
scripts in them -- for very simple scripts we might now behave better.	But the
main idea is fixing this innerHTML with stylesheet thing.
Attachment #199855 - Flags: approval1.8rc1?
Attachment #199855 - Flags: approval1.8rc1? → approval1.8rc1+
Fixed on 1.8 branch.
Keywords: fixed1.8
Depends on: 312847
making the blocking flag reflect reality for this bug.
Flags: blocking1.8rc1? → blocking1.8rc1+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
There is problem when is added HTML code to document by DOM methods:
 - appendChild
 - insertBefore
 - replaceChild
maybe by other methods.
See attached testcases.

Tested on Firefox 1.5.RC1
Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.8) Gecko/20051025 Firefox/1.5
Attached file insertBefore test case
Attached file replaceChild test case
Attached file appendChild test case
Please file a followup bug on that?  And cc me on it?  That's a subtly different problem, and one we're not going to be able to fix for 1.8 at this point...

This bug, as originally filed and with original testcase, is fixed.
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Yes, I made bug #314776.
The extra problems with button 5 went away on the trunk between 2005-11-06-04-trunk and 2005-11-07-04-trunk (Linux firefox).
Depends on: 315189
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: