If you create a form text input field via javascript, and set its contents via javascript, the form is laid out twice: once in the correct location, and once at the top of the page. See the attached URL for a small example (internally, it's at http://warp/employees/akkana/bugs/jsformbug.html or /u/akkana/public_html/bugs/jsformbug.html to make it easier to see the JS code).

Eric, forms related

Linux specific, or all platforms?

All platforms.

Redistributing to M8...

With the June 30th build (Mac, Win 95, Linux), only one text field appears in the test case. This appears to be fixed.

Didn't make M8

This bug has been fixed (it was caused by a malformed content model, so I believe thanks go to Harish or Rickg).

Opps, I meant to say: This bug is now hidden by another bug - form[0] is not defined at the time that the value setting operation is executed. I don't think this is feasible to get done for M10.

After careful consideration, I've decided that I probably won't get this bug in for M12. Currently I have nearly 50 bugs scheduled for M13, so there is a possibility that this bug may need to be moved out farther still.

Moving to M16. Ugly but doesn't prevent use of page.

Hey Vidur, here's another backwards compatability issue - wondered if you had any thoughts on it?

Rescheduling

I keep seeing this bug moved out, milestone after milestone. But I'm not seeing it any more, on the test page, and haven't for quite a while now. I thought it was fixed; is there reason to believe it's still happening?

Nominating nsbeta2. We have to start drawing a line on DOM0 backward compatibility; these bugs are supposed to be a high priority for nsbeta2 per the beta2 criteria.

Text field no longer shows up twice.

This bug is not for the general document.forms[x] case, but only if the form is written out by a script. We should get this working, but it is less severe than if the general case had been broken.

I have a fix for this in my tree - it is a small change to stop caching the list of forms in the document. We not traverse the document looking for forms whenever someone calls document.mforms. I'll attach a preliminary diff, though I'll also have to go and nuke all other references to mForms before I can check this in.

Caching the forms in a document should work just fine even if the document is dynamically changed, the nsContentList that holds the form elements in the document is a and observer of the document and it should be notified when the document changes and it should update itself to contain new form elements even if the elements are dynamically added to the document. I think we should fix the content list to work properly with dynmically added content in stead of removing the caching, since that could really hurt performance on large documents that contain form elements that are accessed through document.forms[x]. Eric, please don't remove the caching, either have a look at the document observer methods in layout/base/src/nsContentList.cpp and see if you find the problem, or reassign this to me and I'll have a look.

Hm, the problem seems harder to reproduce now - did fixes recently go in that would have helped this? I noticed that if printStuff is called in the onload method on the body, this is still reproducible. The problem appears to be that we write out <form>, then try to access the forms array before writing out </form>. We don't get a content appended call in the content list until after the forms array is accessed. The stack for the content appended is: nsContentList::ContentAppended(nsContentList * const 0x0378dd0c, nsIDocument * 0x0377a860, nsIContent * 0x03752648, int 0) line 249 nsDocument::ContentAppended(nsDocument * const 0x0377a860, nsIContent * 0x03752648, int 0) line 1641 nsHTMLDocument::ContentAppended(nsHTMLDocument * const 0x0377a860, nsIContent * 0x03752648, int 0) line 1147 HTMLContentSink::NotifyAppend(nsIContent * 0x03752648, int 0) line 4151 SinkContext::FlushTags(int 1) line 1901 HTMLContentSink::Notify(HTMLContentSink * const 0x03787978, nsITimer * 0x0378dfc0) line 2326 nsTimer::Fire() line 200 nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x01562120, unsigned int 0) line 117 nsAppShell::Run(nsAppShell * const 0x015164e0) line 116 nsAppShellService::Run(nsAppShellService * const 0x01515c30) line 372 Should we flush the sink after every document.write(ln)? Attaching a test case that still shows the bug.

Hmm, flushing the sink after every document.write() would probably also be quite bad for performance, perhaps a fair compromise would bee to drop the cached mForms in the HTML document every time document.write() (and writeln) is called? Something similar probably needs to be done for mImages, mApplets, mEmbeds, mLinks, mAnchors, mForms and mLayers (whow, why do we still have a mLayers?). This should be fairly straight forward to do in nsHTMLDocument::WriteCommon() and nsHTMLDocument::ScriptWriteCommon() (those two functions should probably be one, but that's a later problem). Eric, I haveb't tried this but do you think this sounds reasonable?

> Hmm, flushing the sink after every document.write() would probably also be > quite bad for performance Ah, true, it could be - haven't tested. It might also lead to some nasty things like adding selects with half of their options and other badness. :( > perhaps a fair compromise would bee to drop the cached mForms in the HTML > document every time document.write() (and writeln) is called? Will this have the desired effect? What I noticed was that the form wasn't added to the content model until the "</form>" was written out (causing the sink to flush). So even if we drop mForms, and regenerate it, won't it still be out of date? Or does creating a content list flush the sink? Also, what about a case like this: ... frm = document.forms document.write("<form><input type=text name=foo>") frm[0].foo.value = "Set the value!" document.write("</form>"); ... This is very close to what the original test case tries, but won't even hit the documents GetForms method to regenerate the cached data in time to set the input's value (So neither of our fixes will have any effect, I think). I originally didn't know that the content list was a document observer - it's great that it is! Shouldn't being a document observer be sufficient to solve all these cases, provided that the document is updated (sink flushed) soon enough after document.write's? What kinds of bad side effects could this cause beyond performance problems?

See also bug 32022, which is related.

Sent over to rickg for PDT call.

I think it's time to nail this one down; approving for nsbeta2. Eric: flushing the sink after every document.write() would be unwise.

Removing [NEED INFO] from Status Whiteboard.

As an experiment, I changed this so we do flush the sink after every document.write. As you might have expected, this actually improves perceived performance for some document.write cases (content is created synchronously). I am now in the process of cooking up some long devious test that might be slower or incorrect due to the flushing to compare it with not flushing after every document.write. Can any of you (particularly those opposed to flushing) pitch in test cases here, or descriptions thereof, in case I'm overlooking something where performance would suffer because of this? I'll report back with data after this is done. One other possibility that might also work for the case mentioned above on 2000-05-22 17:47: throwing away mForms after every document.write as suggested by Johnny, then also flushing the sink every time just before mForms's content list is generated - sound better?

<html> <head> <script> function writeTable() { document.open(); document.write("<table>"); for (i=0; i<300; i++) { document.write("<tr><td>test"+i+"</td></tr>"); } document.write("</table>"); document.close(); } </script> </head> <body onload="writeTable();"> fail </body> </html> The above test case takes 6 seconds to display if we flush after every document.write, but just over 1 second to display if we take the second approach mentioned above. The performance hit is obviously unacceptable. :S I'll test the second approach for correctness and go that route.

Came up with another fix for this that doesn't cause performance to suffer like the above, and does work for the tricky test case: No changes to nsHTMLDocument in nsContentList::Item and nsContentList::NamedItem, flush pending notifications on the document before getting the item or named item. This will cause any pending content changes to go through (and the contentList will be rebuild as needed), but acts largely as a noop if there are no pending notifications.

That sounds lile a pretty good solution and I'd say, go for it. There is a (probably) minor downside to that approach tho, we're slowing down all access to document.forms[x] a bit since we do the flushing of the document, which ends up flushing the sink, if ther is one, and the pres shell, IMO we only need to flush the sink, never the pres shell in this case, but doing that is AFAIK not straight forward in todays code since there's no easy way to get the content sink from the document in nsContentList. One possible solution to that problem could be to add an argument to nsIDocument::FlushPendingNotifications() that could tell if we want to flush pending content only or if we also want to flush pending reflows, but that could go into a separate bug...

New bug on the content/reflow flushing split is bug 42892. I have a rough cut at this, but it is probably not a big enough gain to worry about for nsbeta2.

Eric, I had a look at the patch and it looks ok to me, r=jst I agree, I wouldn't worry about the content/reflow flushing split for beta2 either...

I checked this in. Unfortunately, I also remembered after commiting that one other change is needed to make this do anything. Basically, FlushPendingNotifications doesn't do anything unless we're currently inside a <script> tag. That means that in the onLoad handler, as when this function is called, FlushPendingNotifications will have no effect (won't actually flush). I had put this hack in my tree to work around this: (comment out mInScript check) NS_IMETHODIMP HTMLContentSink::FlushPendingNotifications() { nsresult result = NS_OK; // Only flush tags if we're not doing the notification ourselves // (since we aren't reentrant) and if we're in a script (since // we only care to flush if this is done via script). if (mCurrentContext && !mInNotification) { // && mInScript) { result = mCurrentContext->FlushTags(); } return result; } It's probably wrong, but I haven't yet figured out what to do instead yet. So it's not done quite yet.

*** Bug 40019 has been marked as a duplicate of this bug. ***

Test against bug 42178

Fix checked in. To verify, view the final test case. You should see a text input that says "Set the value!" I you see this, the bug was fixed, thanks!

Fixed in the July 10th build.