[FIX]DIV content is rendered twice

RESOLVED FIXED in mozilla1.8rc1



DOM: Core & HTML
12 years ago
9 years ago


(Reporter: Miroslav Juhos, Assigned: bz)


({fixed1.8, regression, testcase})

fixed1.8, regression, testcase
Dependency tree / graph
Bug Flags:
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)



(6 attachments)



12 years ago
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:
     onload = function() {
      document.getElementById("message").style.display = "block";
      document.getElementById("message").innerHTML =

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

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

Expected Results:  
Firefox will display text "content".


12 years ago
Version: unspecified → 1.5 Branch
Created attachment 199796 [details]
reporter's testcase
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
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?

Comment 5

12 years ago
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
Created attachment 199837 [details]
Modified testcase that also shows the problem in builds from before restyle batching
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
Created attachment 199855 [details] [diff] [review]
Proposed patch

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

Attachment #199855 - Flags: superreview?(jst)
Attachment #199855 - Flags: superreview+
Attachment #199855 - Flags: review?(jst)
Attachment #199855 - Flags: review+
Fixed on trunk.
Last Resolved: 12 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?


12 years ago
Attachment #199855 - Flags: approval1.8rc1? → approval1.8rc1+
Fixed on 1.8 branch.
Keywords: fixed1.8
Depends on: 312847

Comment 15

12 years ago
making the blocking flag reflect reality for this bug.
Flags: blocking1.8rc1? → blocking1.8rc1+


12 years ago
Resolution: FIXED → ---

Comment 16

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

Comment 17

12 years ago
Created attachment 201627 [details]
insertBefore test case

Comment 18

12 years ago
Created attachment 201628 [details]
replaceChild test case

Comment 19

12 years ago
Created attachment 201630 [details]
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.
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 21

12 years ago
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).
Er, sorry, wrong bug.
Depends on: 314776, 315999
Depends on: 315189


9 years ago
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.