Closed Bug 312695 Opened 19 years ago Closed 19 years ago

[FIX]DIV content is rendered twice

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8rc1

People

(Reporter: mjuhos, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8, regression, testcase)

Attachments

(6 files)

reporter's testcase
330 bytes, text/html
Details
Modified testcase that also shows the problem in builds from before restyle batching
291 bytes, text/html
Details
Proposed patch
4.53 KB, patch
jst
: review+
jst
: superreview+
asa
: approval1.8rc1+
Details | Diff | Splinter Review
insertBefore test case
466 bytes, text/html
Details
replaceChild test case
466 bytes, text/html
Details
appendChild test case
408 bytes, text/html
Details
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
Attached file 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
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 patch — — Splinter 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 ago → 19 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

Creator:
Created:
Updated:
Size: